Re: MERGE ... RETURNING

On Sun, 22 Jan 2023 at 19:08, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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