MERGE ... RETURNING
I've been thinking about adding RETURNING support to MERGE in order to
let the user see what changed.
I considered allowing a separate RETURNING list at the end of each
action, but rapidly dismissed that idea. Firstly, it introduces
shift/reduce conflicts to the grammar. These can be resolved by making
the "AS" before column aliases non-optional, but that's pretty ugly,
and there may be a better way. More serious drawbacks are that this
syntax is much more cumbersome for the end user, having to repeat the
RETURNING clause several times, and the implementation is likely to be
pretty complex, so I didn't pursue it.
A much simpler approach is to just have a single RETURNING list at the
end of the command. That's much easier to implement, and easier for
the end user. The main drawback is that it's impossible for the user
to work out from the values returned which action was actually taken,
and I think that's a pretty essential piece of information (at least
it seems pretty limiting to me, not being able to work that out).
So playing around with it (and inspired by the WITH ORDINALITY syntax
for SRFs), I had the idea of allowing "WITH WHEN CLAUSE" at the end of
the returning list, which adds an integer column to the list, whose
value is set to the index of the when clause executed, as in the
attached very rough patch.
So, quoting an example from the tests, this allows things like:
WITH t AS (
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 t.* WITH WHEN CLAUSE
)
SELECT CASE when_clause
WHEN 1 THEN 'UPDATE'
WHEN 2 THEN 'INSERT'
WHEN 3 THEN 'DELETE'
END, *
FROM t;
case | tid | balance | when_clause
--------+-----+---------+-------------
INSERT | -1 | -11 | 2
DELETE | 1 | 100 | 3
(2 rows)
1 row is returned for each merge action executed (other than DO
NOTHING actions), and as usual, the values represent old target values
for DELETE actions, and new target values for INSERT/UPDATE actions.
It's also possible to return the source values, and a bare "*" in the
returning list expands to all the source columns, followed by all the
target columns.
The name of the added column, if included, can be changed by
specifying "WITH WHEN CLAUSE [AS] col_alias". I chose the syntax "WHEN
CLAUSE" and "when_clause" as the default column name because those
match the existing terminology used in the docs.
Anyway, this feels like a good point to stop playing around and get
feedback on whether this seems useful, or if anyone has other ideas.
Regards,
Dean
Attachments:
POC-support-MERGE-RETURNING.patchtext/x-patch; charset=US-ASCII; name=POC-support-MERGE-RETURNING.patchDownload+352-76
On Sun, 8 Jan 2023 at 07:28, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
So playing around with it (and inspired by the WITH ORDINALITY syntax
for SRFs), I had the idea of allowing "WITH WHEN CLAUSE" at the end of
the returning list, which adds an integer column to the list, whose
value is set to the index of the when clause executed, as in the
attached very rough patch.
Would it be useful to have just the action? Perhaps "WITH ACTION"? My idea
is that this would return an enum of INSERT, UPDATE, DELETE (so is "action"
the right word?). It seems to me in many situations I would be more likely
to care about which of these 3 happened rather than the exact clause that
applied. This isn't necessarily meant to be instead of your suggestion
because I can imagine wanting to know the exact clause, just an alternative
that might suffice in many situations. Using it would also avoid problems
arising from editing the query in a way which changes the numbers of the
clauses.
So, quoting an example from the tests, this allows things like:
WITH t AS (
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 t.* WITH WHEN CLAUSE
)
SELECT CASE when_clause
WHEN 1 THEN 'UPDATE'
WHEN 2 THEN 'INSERT'
WHEN 3 THEN 'DELETE'
END, *
FROM t;case | tid | balance | when_clause
--------+-----+---------+-------------
INSERT | -1 | -11 | 2
DELETE | 1 | 100 | 3
(2 rows)1 row is returned for each merge action executed (other than DO
NOTHING actions), and as usual, the values represent old target values
for DELETE actions, and new target values for INSERT/UPDATE actions.
Would it be feasible to allow specifying old.column or new.column? These
would always be NULL for INSERT and DELETE respectively but more useful
with UPDATE. Actually I've been meaning to ask this question about UPDATE …
RETURNING.
Actually, even with DELETE/INSERT, I can imagine wanting, for example, to
get only the new values associated with INSERT or UPDATE and not the values
removed by a DELETE. So I can imagine specifying new.column to get NULLs
for any row that was deleted but still get the new values for other rows.
It's also possible to return the source values, and a bare "*" in the
returning list expands to all the source columns, followed by all the
target columns.
Does this lead to a problem in the event there are same-named columns
between source and target?
The name of the added column, if included, can be changed by
Show quoted text
specifying "WITH WHEN CLAUSE [AS] col_alias". I chose the syntax "WHEN
CLAUSE" and "when_clause" as the default column name because those
match the existing terminology used in the docs.
On Sun, 8 Jan 2023 at 20:09, Isaac Morland <isaac.morland@gmail.com> wrote:
Would it be useful to have just the action? Perhaps "WITH ACTION"? My idea is that this would return an enum of INSERT, UPDATE, DELETE (so is "action" the right word?). It seems to me in many situations I would be more likely to care about which of these 3 happened rather than the exact clause that applied. This isn't necessarily meant to be instead of your suggestion because I can imagine wanting to know the exact clause, just an alternative that might suffice in many situations. Using it would also avoid problems arising from editing the query in a way which changes the numbers of the clauses.
Hmm, perhaps that's something that can be added as well. Both use
cases seem useful.
1 row is returned for each merge action executed (other than DO
NOTHING actions), and as usual, the values represent old target values
for DELETE actions, and new target values for INSERT/UPDATE actions.Would it be feasible to allow specifying old.column or new.column? These would always be NULL for INSERT and DELETE respectively but more useful with UPDATE. Actually I've been meaning to ask this question about UPDATE … RETURNING.
I too have wished for the ability to do that with UPDATE ...
RETURNING, though I'm not sure how feasible it is.
I think it's something best considered separately though. I haven't
given any thought as to how to make it work, so there might be
technical difficulties. But if it could be made to work for UPDATE, it
shouldn't be much more effort to make it work for MERGE.
It's also possible to return the source values, and a bare "*" in the
returning list expands to all the source columns, followed by all the
target columns.Does this lead to a problem in the event there are same-named columns between source and target?
Not really. It's exactly the same as doing "SELECT * FROM src JOIN tgt
ON ...". That may lead to duplicate column names in the result, but
that's not necessarily a problem.
Regards,
Dean
On 1/9/23 13:29, Dean Rasheed wrote:
On Sun, 8 Jan 2023 at 20:09, Isaac Morland <isaac.morland@gmail.com> wrote:
Would it be useful to have just the action? Perhaps "WITH ACTION"? My idea is that this would return an enum of INSERT, UPDATE, DELETE (so is "action" the right word?). It seems to me in many situations I would be more likely to care about which of these 3 happened rather than the exact clause that applied. This isn't necessarily meant to be instead of your suggestion because I can imagine wanting to know the exact clause, just an alternative that might suffice in many situations. Using it would also avoid problems arising from editing the query in a way which changes the numbers of the clauses.
Hmm, perhaps that's something that can be added as well. Both use
cases seem useful.
Bikeshedding here. Instead of Yet Another WITH Clause, could we perhaps
make a MERGING() function analogous to the GROUPING() function that goes
with grouping sets?
MERGE ...
RETURNING *, MERGING('clause'), MERGING('action');
Or something.
--
Vik Fearing
On Mon, 9 Jan 2023 at 16:23, Vik Fearing <vik@postgresfriends.org> wrote:
Bikeshedding here. Instead of Yet Another WITH Clause, could we perhaps
make a MERGING() function analogous to the GROUPING() function that goes
with grouping sets?MERGE ...
RETURNING *, MERGING('clause'), MERGING('action');
Hmm, possibly, but I think that would complicate the implementation quite a bit.
GROUPING() is not really a function (in the sense that there is no
pg_proc entry for it, you can't do "\df grouping", and it isn't
executed with its arguments like a normal function). Rather, it
requires special-case handling in the parser, through to the executor,
and I think MERGING() would be similar.
Also, it masks any user function with the same name, and would
probably require MERGING to be some level of reserved keyword.
I'm not sure that's worth it, just to have a more standard-looking
RETURNING list, without a WITH clause.
Regards,
Dean
On Mon, 9 Jan 2023 at 17:44, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Mon, 9 Jan 2023 at 16:23, Vik Fearing <vik@postgresfriends.org> wrote:
Bikeshedding here. Instead of Yet Another WITH Clause, could we perhaps
make a MERGING() function analogous to the GROUPING() function that goes
with grouping sets?MERGE ...
RETURNING *, MERGING('clause'), MERGING('action');Hmm, possibly, but I think that would complicate the implementation quite a bit.
GROUPING() is not really a function (in the sense that there is no
pg_proc entry for it, you can't do "\df grouping", and it isn't
executed with its arguments like a normal function). Rather, it
requires special-case handling in the parser, through to the executor,
and I think MERGING() would be similar.Also, it masks any user function with the same name, and would
probably require MERGING to be some level of reserved keyword.
I thought about this some more, and I think functions do make more
sense here, rather than inventing a special WITH syntax. However,
rather than using a special MERGING() function like GROUPING(), which
isn't really a function at all, I think it's better (and much simpler
to implement) to have a pair of normal functions (one returning int,
and one text).
The example from the tests shows the sort of thing this allows:
MERGE INTO sq_target t USING sq_source s 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_when_clause() AS when_clause,
pg_merge_action() AS merge_action,
t.*,
CASE pg_merge_action()
WHEN 'INSERT' THEN 'Inserted '||t
WHEN 'UPDATE' THEN 'Added '||delta||' to balance'
WHEN 'DELETE' THEN 'Removed '||t
END AS description;
when_clause | merge_action | tid | balance | description
-------------+--------------+-----+---------+---------------------
3 | DELETE | 1 | 100 | Removed (1,100)
1 | UPDATE | 2 | 220 | Added 20 to balance
2 | INSERT | 4 | 40 | Inserted (4,40)
(3 rows)
I think this is easier to use than the WITH syntax, and more flexible,
since the new functions can be used anywhere in the RETURNING list,
including in expressions.
There is one limitation though. Due to the way these functions need
access to the originating query, they need to appear directly in
MERGE's RETURNING list, not in subqueries, plpgsql function bodies, or
anything else that amounts to a different query. Maybe there's a way
round that, but it looks tricky. In practice though, it's easy to work
around, if necessary (e.g., by wrapping the MERGE in a CTE).
Regards,
Dean
Attachments:
v2-POC-support-MERGE-RETURNING.patchtext/x-patch; charset=US-ASCII; name=v2-POC-support-MERGE-RETURNING.patchDownload+313-98
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c new file mode 100644 index e34f583..aa3cca0 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -274,12 +274,6 @@ DoCopy(ParseState *pstate, const CopyStm { Assert(stmt->query);- /* MERGE is allowed by parser, but unimplemented. Reject for now */
- if (IsA(stmt->query, MergeStmt))
- ereport(ERROR,
- errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("MERGE not supported in COPY"));
Does this COPY stuff come from another branch where you're adding
support for MERGE in COPY? I see that you add a test that MERGE without
RETURNING fails, but you didn't add any tests that it works with
RETURNING. Anyway, I suspect these small changes shouldn't be here.
Overall, the idea of using Postgres-specific functions for extracting
context in the RETURNING clause looks acceptable to me. We can change
that to add support to whatever clauses the SQL committee offers, when
they get around to offering something. (We do have to keep our fingers
crossed that they will decide to use the same RETURNING syntax as we do
in this patch, of course.)
Regarding mas_action_idx, I would have thought that it belongs in
MergeAction rather than MergeActionState. After all, you determine it
once at parse time, and it is a constant from there onwards, right?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Sun, 22 Jan 2023 at 19:08, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c new file mode 100644 index e34f583..aa3cca0 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -274,12 +274,6 @@ DoCopy(ParseState *pstate, const CopyStm { Assert(stmt->query);- /* MERGE is allowed by parser, but unimplemented. Reject for now */
- if (IsA(stmt->query, MergeStmt))
- ereport(ERROR,
- errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("MERGE not supported in COPY"));Does this COPY stuff come from another branch where you're adding
support for MERGE in COPY? I see that you add a test that MERGE without
RETURNING fails, but you didn't add any tests that it works with
RETURNING. Anyway, I suspect these small changes shouldn't be here.
A few of the code changes I made were entirely untested (this was
after all just a proof-of-concept, intended to gather opinions and get
consensus about the overall shape of the feature). They serve as
useful reminders of things to test. In fact, since then, I've been
doing more testing, and so far everything I have tried has just
worked, including COPY (MERGE ... RETURNING ...) TO ... Thinking about
it, I can't see any reason why it wouldn't.
Still, there's a lot more testing to do. Just going through the docs
looking for references to RETURNING gave me a lot more ideas of things
to test.
Overall, the idea of using Postgres-specific functions for extracting
context in the RETURNING clause looks acceptable to me.
Cool.
We can change
that to add support to whatever clauses the SQL committee offers, when
they get around to offering something. (We do have to keep our fingers
crossed that they will decide to use the same RETURNING syntax as we do
in this patch, of course.)
Yes indeed. At least, done this way, the only non-SQL-standard syntax
is the RETURNING keyword itself, which we've already settled on for
INSERT/UPDATE/DELETE. Let's just hope they don't decide to use
RETURNING in an incompatible way in the future.
Regarding mas_action_idx, I would have thought that it belongs in
MergeAction rather than MergeActionState. After all, you determine it
once at parse time, and it is a constant from there onwards, right?
Oh, yes that makes sense (and removes the need for a couple of the
executor changes). Thanks for looking.
Regards,
Dean
On Mon, 23 Jan 2023 at 16:54, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Sun, 22 Jan 2023 at 19:08, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Regarding mas_action_idx, I would have thought that it belongs in
MergeAction rather than MergeActionState. After all, you determine it
once at parse time, and it is a constant from there onwards, right?Oh, yes that makes sense (and removes the need for a couple of the
executor changes). Thanks for looking.
Attached is a more complete patch, with that change and more doc and
test updates.
A couple of latest changes from this patch look like they represent
pre-existing issues with MERGE that should really be extracted from
this patch and applied to HEAD+v15. I'll take a closer look at that,
and start new threads for those.
Regards,
Dean
Attachments:
support-merge-returning-v3.patchtext/x-patch; charset=US-ASCII; name=support-merge-returning-v3.patchDownload+943-167
On Tue, 7 Feb 2023 at 10:56, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Attached is a more complete patch
Rebased version attached.
Regards,
Dean
Attachments:
support-merge-returning-v4.patchtext/x-patch; charset=US-ASCII; name=support-merge-returning-v4.patchDownload+903-163
On Fri, 24 Feb 2023 at 05:46, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Rebased version attached.
Another rebase.
Regards,
Dean
Attachments:
support-merge-returning-v5.patchtext/x-patch; charset=US-ASCII; name=support-merge-returning-v5.patchDownload+896-160
On Sun, 26 Feb 2023 at 09:50, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Another rebase.
And another rebase.
Regards,
Dean
Attachments:
support-merge-returning-v6.patchtext/x-patch; charset=US-ASCII; name=support-merge-returning-v6.patchDownload+904-162
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. 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
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.
Attached is an updated patch with some cosmetic updates, plus updated
ruleutils support.
Regards,
Dean
Attachments:
support-merge-returning-v7.patchtext/x-patch; charset=US-ASCII; name=support-merge-returning-v7.patchDownload+939-177
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 THENThis 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 THENFor 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 THENAgain, 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
On 2023-Jul-05, Gurjeet Singh wrote:
+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.
You misread this one :-) The COPY output is there, the tag is not. So
DELETE is the value from pg_merge_action(), and "1 100" correspond to
the columns in the the sq_target row that was deleted. The command tag
is presumably MERGE 1.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Thu, Jul 6, 2023 at 1:13 PM Gurjeet Singh <gurjeet@singh.im> wrote:
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 MERGEnit: 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().
I think the name of function pg_merge_when_clause() can be improved.
How about pg_merge_when_clause_ordinal().
another idea: pg_merge_action_ordinal()
On Thu, Jul 6, 2023 at 3:39 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Jul-05, Gurjeet Singh wrote:
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.You misread this one :-) The COPY output is there, the tag is not. So
DELETE is the value from pg_merge_action(), and "1 100" correspond to
the columns in the the sq_target row that was deleted. The command tag
is presumably MERGE 1.
:-) That makes more sense. It matches my old mental model. Thanks for
clarifying!
Best regards,
Gurjeet
http://Gurje.et
On Thu, Jul 6, 2023 at 4:07 AM jian he <jian.universality@gmail.com> wrote:
On Thu, Jul 6, 2023 at 1:13 PM Gurjeet Singh <gurjeet@singh.im> wrote:
I think the name of function pg_merge_when_clause() can be improved.
How about pg_merge_when_clause_ordinal().another idea: pg_merge_action_ordinal()
Since there can be many occurrences of the same action
(INSERT/UPDATE/DELETE) in a MERGE command associated with different
conditions, I don't think action_ordinal would make sense for this
function name.
e.g.
WHEN MATCHED and src.col1 = val1 THEN UPDATE col2 = someval1
WHEN MATCHED and src.col1 = val2 THEN UPDATE col2 = someval2
...
When looking at the implementation code, as well, we see that the code
in this function tracks and reports the lexical position of the WHEN
clause, irrespective of the action associated with that WHEN clause.
foreach(l, stmt->mergeWhenClauses)
{
...
action->index = foreach_current_index(l) + 1;
Best regards,
Gurjeet
http://Gurje.et
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 THENThis 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 THENFor 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 THENAgain, 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
Attachments:
support-merge-returning-v8.patchtext/x-patch; charset=US-ASCII; name=support-merge-returning-v8.patchDownload+978-170
On Fri, Jul 7, 2023 at 3:48 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Thu, 6 Jul 2023 at 06:13, Gurjeet Singh <gurjeet@singh.im> wrote:
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.
+1
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.
+1. How do we make sure we don't forget that it needs to be named
better. Perhaps a TODO item within the patch will help.
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.
I don't think that's necessary, if it improves consistency with related docs.
+ /* + * 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.
I believe this elog can be safely turned into an Assert.
+ switch (mergeActionCmdType)
{
- CmdType commandType = relaction->mas_action->commandType;
....
+ case CMD_INSERT:
....
+ default:
+ elog(ERROR, "unrecognized commandType: %d", (int)
mergeActionCmdType);The same treatment can be applied to the elog(ERROR) in pg_merge_action().
+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 THENAgain, 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.
I was just trying to drive these tests towards a consistent pattern.
As a reader, if I see these differences, the first and the
conservative thought I have is that these differences must be there
for a reason, and then I have to work to find out what those reasons
might be. And that's a lot of wasted effort, just in case someone
intends to change something in these tests.
I performed this round of review by comparing the diff between the v7
and v8 patches (after applying to commit 4f4d73466d)
-ExecProcessReturning(ResultRelInfo *resultRelInfo,
+ExecProcessReturning(ModifyTableContext *context,
+ ResultRelInfo *resultRelInfo,
...
+ TupleTableSlot *rslot;
...
+ if (context->relaction)
+ {
...
+ PG_TRY();
+ {
+ rslot = ExecProject(projectReturning);
+ }
+ PG_FINALLY();
+ {
+ mergeActionCmdType = saved_mergeActionCmdType;
+ mergeActionIdx = saved_mergeActionIdx;
+ }
+ PG_END_TRY();
+ }
+ else
+ rslot = ExecProject(projectReturning);
+
+ return rslot;In the above hunk, if there's an exception/ERROR, I believe we should
PG_RE_THROW(). If there's a reason to continue, we should at least set
rslot = NULL, otherwise we may be returning an uninitialized value to
the caller.
{ oid => '9499', descr => 'command type of current MERGE action',
- proname => 'pg_merge_action', provolatile => 'v',
+ proname => 'pg_merge_action', provolatile => 'v', proparallel => 'r',
....
{ oid => '9500', descr => 'index of current MERGE WHEN clause',
- proname => 'pg_merge_when_clause', provolatile => 'v',
+ proname => 'pg_merge_when_clause', provolatile => 'v', proparallel => 'r',
....I see that you've now set proparallel of these functions to 'r'. I'd
just like to understand how you got to that conclusion.
--- error when using MERGE support functions outside MERGE
-SELECT pg_merge_action() FROM sq_target;I believe it would be worthwhile to keep a record of the expected
outputs of these invocations in the tests, just in case they change
over time.
Best regards,
Gurjeet
http://Gurje.et
