Re: MERGE ... RETURNING

On Thu, 6 Jul 2023 at 06:13, Gurjeet Singh <gurjeet@singh.im> wrote:
>
> Most of the review was done with the v6 of the patch, minus the doc
> build step. The additional changes in v7 of the patch were eyeballed,
> and tested with `make check`.
>

Thanks for the review!

> > One minor annoyance is that, due to the way that pg_merge_action() and
> > pg_merge_when_clause() require access to the MergeActionState, they
> > only work if they appear directly in the RETURNING list. They can't,
> > for example, appear in a subquery in the RETURNING list, and I don't
> > see an easy way round that limitation.
>
> I believe that's a serious limitation, and would be a blocker for the feature.
>

Yes, that was bugging me for quite a while.

Attached is a new version that removes that restriction, allowing the
merge support functions to appear anywhere. This requires a bit of
planner support, to deal with merge support functions in subqueries,
in a similar way to how aggregates and GROUPING() expressions are
handled. But a number of the changes from the previous patch are no
longer necessary, so overall, this version of the patch is slightly
smaller.

> I think the name of function pg_merge_when_clause() can be improved.
> How about pg_merge_when_clause_ordinal().
>

That's a bit of a mouthful, but I don't have a better idea right now.
I've left the names alone for now, in case something better occurs to
anyone.

> In the doc page of MERGE, you've moved the 'with_query' from the
> bottom of Parameters section to the top of that section. Any reason
> for this? Since the Parameters section is quite large, for a moment I
> thought the patch was _adding_ the description of 'with_query'.
>

Ah yes, I was just making the order consistent with the
INSERT/UPDATE/DELETE pages. That could probably be committed
separately.

> +    /*
> +     * Merge support functions should only be called directly from a MERGE
> +     * command, and need access to the parent ModifyTableState.  The parser
> +     * should have checked that such functions only appear in the RETURNING
> +     * list of a MERGE, so this should never fail.
> +     */
> +    if (IsMergeSupportFunction(funcid))
> +    {
> +        if (!state->parent ||
> +            !IsA(state->parent, ModifyTableState) ||
> +            ((ModifyTableState *) state->parent)->operation != CMD_MERGE)
> +            elog(ERROR, "merge support function called in non-merge context");
>
> As the comment says, this is an unexpected condition, and should have
> been caught and reported by the parser. So I think it'd be better to
> use an Assert() here. Moreover, there's ERROR being raised by the
> pg_merge_action() and pg_merge_when_clause() functions, when they
> detect the function context is not appropriate.
>
> I found it very innovative to allow these functions to be called only
> in a certain part of certain SQL command. I don't think  there's a
> precedence for this in  Postgres; I'd be glad to learn if there are
> other functions that Postgres exposes this way.
>
> -    EXPR_KIND_RETURNING,        /* RETURNING */
> +    EXPR_KIND_RETURNING,        /* RETURNING in INSERT/UPDATE/DELETE */
> +    EXPR_KIND_MERGE_RETURNING,  /* RETURNING in MERGE */
>
> Having to invent a whole new ParseExprKind enum, feels like a bit of
> an overkill. My only objection is that this is exactly like
> EXPR_KIND_RETURNING, hence EXPR_KIND_MERGE_RETURNING needs to be dealt
> with exactly in as many places. But I don't have any alternative
> proposals.
>

That's all gone now from the new patch, since there is no longer any
restriction on where these functions can appear.

> --- a/src/include/catalog/pg_proc.h
> +++ b/src/include/catalog/pg_proc.h
> +/* Is this a merge support function?  (Requires fmgroids.h) */
> +#define IsMergeSupportFunction(funcid) \
> +    ((funcid) == F_PG_MERGE_ACTION || \
> +     (funcid) == F_PG_MERGE_WHEN_CLAUSE)
>
> If it doesn't cause recursion or other complications, I think we
> should simply include the fmgroids.h in pg_proc.h. I understand that
> not all .c files/consumers that include pg_proc.h may need fmgroids.h,
> but #include'ing it will eliminate the need to keep the "Requires..."
> note above, and avoid confusion, too.
>

There's now only one place that uses this macro, whereas there are a
lot of places that include pg_proc.h and not fmgroids.h, so I don't
think forcing them all to include fmgroids.h is a good idea. (BTW,
this approach and comment is borrowed from IsTrueArrayType() in
pg_type.h)

> --- a/src/test/regress/expected/merge.out
> +++ b/src/test/regress/expected/merge.out
>
> -WHEN MATCHED AND tid > 2 THEN
> +WHEN MATCHED AND tid >= 2 THEN
>
> This change can be treated as a bug fix :-)
>
> +-- COPY (MERGE ... RETURNING) TO ...
> +BEGIN;
> +COPY (
> +    MERGE INTO sq_target t
> +    USING v
> +    ON tid = sid
> +    WHEN MATCHED AND tid > 2 THEN
>
> For consistency, the check should be tid >= 2, like you've fixed in
> command referenced above.
>
> +CREATE FUNCTION merge_into_sq_target(sid int, balance int, delta int,
> +                                     OUT action text, OUT tid int,
> OUT new_balance int)
> +LANGUAGE sql AS
> +$$
> +    MERGE INTO sq_target t
> +    USING (VALUES ($1, $2, $3)) AS v(sid, balance, delta)
> +    ON tid = v.sid
> +    WHEN MATCHED AND tid > 2 THEN
>
> Again, for consistency, the comparison operator should be >=. There
> are a few more occurrences of this comparison in the rest of the file,
>  that need the same treatment.
>

I changed the new tests to use ">= 2" (and the COPY test now returns 3
rows, with an action of each type, which is easier to read), but I
don't think it's really necessary to change all the existing tests
from "> 2". There's nothing wrong with the "= 2" case having no
action, as long as the tests give decent coverage.

Thanks again for all the feedback.

Regards,
Dean