Re: MERGE ... RETURNING

On Sat, Jul 1, 2023 at 4:08 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Mon, 13 Mar 2023 at 13:36, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > And another rebase.
> >
>
> I ran out of cycles to pursue the MERGE patches in v16, but hopefully
> I can make more progress in v17.
>
> Looking at this one with fresh eyes, it looks mostly in good shape.

+1

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`.

> To
> recap, this adds support for the RETURNING clause in MERGE, together
> with new support functions pg_merge_action() and
> pg_merge_when_clause() that can be used in the RETURNING list of MERGE

nit: s/can be used in/can be used only in/

> to retrieve the kind of action (INSERT/UPDATE/DELETE), and the index
> of the WHEN clause executed for each row merged. In addition,
> RETURNING support allows MERGE to be used as the source query in COPY
> TO and WITH queries.
>
> 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.

> Attached is an updated patch with some cosmetic updates, plus updated
> ruleutils support.

With each iteration of your patch, it is becoming increasingly clear
that this will be a documentation-heavy patch :-)

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

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'.


+    /*
+     * 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.

--- 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.

--- 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.

+BEGIN;
+COPY (
+    MERGE INTO sq_target t
+    USING v
+    ON tid = sid
+    WHEN MATCHED AND tid > 2 THEN
+        UPDATE SET balance = t.balance + delta
+    WHEN NOT MATCHED THEN
+        INSERT (balance, tid) VALUES (balance + delta, sid)
+    WHEN MATCHED AND tid < 2 THEN
+        DELETE
+    RETURNING pg_merge_action(), t.*
+) TO stdout;
+DELETE  1   100
+ROLLBACK;

I expected the .out file to have captured the stdout. I'm gradually,
and gladly, re-learning bits of the test infrastructure.

The DELETE command tag in the output does not feel appropriate for a
COPY command that's using MERGE as the source of the data.

+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.

Best regards,
Gurjeet
http://Gurje.et