Re: MERGE ... RETURNING

On Fri, Jul 21, 2023 at 7:17 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Mon, 17 Jul 2023 at 20:43, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> >
> > > > Maybe instead of a function it could be a special table reference
> > > > like:
> > > >
> > > > MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val?
> > > >
> > The benefits are:
> >
> > 1. It is naturally constrained to the right context. It doesn't require
> > global variables and the PG_TRY/PG_FINALLY, and can't be called in the
> > wrong contexts (like SELECT).
> >
> > 2. More likely to be consistent with eventual support for NEW/OLD
> > (actually BEFORE/AFTER for reasons the prior thread discussed).
> >
>
> Thinking about this some more, I think that the point about
> constraining these functions to the right context is a reasonable one,
> and earlier versions of this patch did that better, without needing
> global variables or a PG_TRY/PG_FINALLY block.
>
> Here is an updated patch that goes back to doing it that way. This is
> more like the way that aggregate functions and GROUPING() work, in
> that the parser constrains the location from which the functions can
> be used, and at execution time, the functions rely on the relevant
> context being passed via the FunctionCallInfo context.
>
> It's still possible to use these functions in subqueries in the
> RETURNING list, but attempting to use them anywhere else (like a
> SELECT on its own) will raise an error at parse time. If they do
> somehow get invoked in a non-MERGE context, they will elog an error
> (again, just like aggregate functions), because that's a "shouldn't
> happen" error.
>
> This does nothing to be consistent with eventual support for
> BEFORE/AFTER, but I think that's really an entirely separate thing,

+1

> From a user perspective, writing something like "BEFORE.id" is quite
> natural, because it's clear that "id" is a column, and "BEFORE" is the
> old state of the table. Writing something like "MERGE.action" seems a
> lot more counter-intuitive, because "action" isn't a column of
> anything (and if it was, I think this syntax would potentially cause
> even more confusion).
>
> So really, I think "MERGE.action" is an abuse of the syntax,
> inconsistent with any other SQL syntax, and using functions is much
> more natural, akin to GROUPING(), for example.

There seems to be other use cases which need us to invent a method to
expose a command-specific alias. See Tatsuo Ishii's call for help in
his patch for Row Pattern Recognition [1].

<quote>
I was not able to find a way to implement expressions like START.price
(START is not a table alias). Any suggestion is greatly appreciated.
</quote>

It looks like the SQL standard has started using more of such
context-specific keywords, and I'd expect such keywords to only
increase in future, as the SQL committee tries to introduce more
features into the standard.

So if MERGE.action is not to your taste, perhaps you/someone can
suggest an alternative that doesn't cause a confusion, and yet
implementing it would open up the way for more such context-specific
keywords.

I performed the review vo v9 patch by comparing it to v8 and v7
patches, and then comparing to HEAD.

The v9 patch is more or less a complete revert to v7 patch, plus the
planner support for calling the merge-support functions in subqueries,
parser catching use of merge-support functions outside MERGE command,
and name change for one of the support functions.

But reverting to v7 also means that some of my gripes with that
version also return; e.g. invention of EXPR_KIND_MERGE_RETURNING. And
as noted in v7 review, I don't have a better proposal.

Function name changed from pg_merge_when_clause() to
pg_merge_when_clause_number(). That's better, even though it's a bit
of mouthful.

Doc changes (compared to v7) look good.

The changes made to tests (compared to v7) are for the better.

- * Uplevel PlaceHolderVars and aggregates are replaced, too.
+ * Uplevel PlaceHolderVars, aggregates, GROUPING() expressions and merge
+ * support functions are replaced, too.

Needs Oxford comma: s/GROUPING() expressions and/GROUPING() expressions, and/

+pg_merge_action(PG_FUNCTION_ARGS)
+{
...
+ relaction = mtstate->mt_merge_action;
+ if (relaction)
+ {
..
+ }
+
+ PG_RETURN_NULL();
+}

Under what circumstances would the relaction be null? Is it okay to
return NULL from this function if relaction is null, or is it better
to throw an error? These questions apply to the
pg_merge_when_clause_number() function as well.

[1]: Row pattern recognition
https://www.postgresql.org/message-id/flat/20230625.210509.1276733411677577841.t-ishii%40sranhm.sra.co.jp

Best regards,
Gurjeet
http://Gurje.et