Re: MERGE ... RETURNING

On Fri, Jul 14, 2023 at 1:55 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Thu, 13 Jul 2023 at 20:14, Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > On Thu, 2023-07-13 at 18:01 +0100, Dean Rasheed wrote:
> > > For some use cases, I can imagine allowing OLD/NEW.colname would mean
> > > you wouldn't need pg_merge_action() (if the column was NOT NULL), so
> > > I
> > > think the features should work well together.
> >
> > For use cases where a user could do it either way, which would you
> > expect to be the "typical" way (assuming we supported the new/old)?
> >
> >   MERGE ... RETURNING pg_merge_action(), id, val;
> >
> > or
> >
> >   MERGE ... RETURNING id, OLD.val, NEW.val;
> >
> > ?
> >
>
> I think it might depend on whether OLD.val and NEW.val were actually
> required, but I think I would still probably use pg_merge_action() to
> get the action, since it doesn't rely on specific table columns being
> NOT NULL.

+1. It would be better to expose the action explicitly, rather than
asking the user to deduce it based on the old and new values of a
column. The server providing that value is better than letting users
rely on error-prone methods.

> I found a 10-year-old thread discussing adding support for OLD/NEW to
> RETURNING [1],

Thanks for digging up that thread. An important concern brought up in
that thread was how the use of names OLD and NEW will affect plpgsql
(an possibly other PLs) trigger functions, which rely on specific
meaning for those names. The names BEFORE and AFTER, proposed there
are not as intuitive as OLD/NEW for the purpose of identifying old and
new versions of the row, but I don't have a better proposal. Perhaps
PREVIOUS and CURRENT?

> but it doesn't look like anything close to a
> committable solution was developed, or even a design that might lead
> to one. That's a shame, because there seemed to be a lot of demand for
> the feature,

+1

> > I am still bothered that pg_merge_action() is so context-sensitive.
> > "SELECT pg_merge_action()" by itself doesn't make any sense, but it's
> > allowed in the v8 patch. We could make that a runtime error, which
> > would be better, but it feels like it's structurally wrong. This is not
> > an objection, but it's just making me think harder about alternatives.
> >
> > Maybe instead of a function it could be a special table reference like:
> >
> >   MERGE ... RETURNING MERGE.action, MERGE.action_number, id, val?

I believe Jeff meant s/action_number/when_number/. Not that we've
settled on a name for this virtual column.

> Well, that's a little more concise, but I'm not sure that it really
> buys us that much, to be worth the extra complication.

After considering the options, and their pros and cons (ease of
implementation, possibility of conflict with SQL spec, intuitiveness
of syntax), I'm now strongly leaning towards the SQL syntax variant.
Exposing the action taken via a context-sensitive function feels
kludgy, when compared to Jeff's proposed SQL syntax. Don't get me
wrong, I still feel it was very clever how you were able to make the
function context sensitive, and make it work in expressions deeper in
the subqueries.

Plus, if we were able to make it work as SQL syntax, it's very likely
we can use the same technique to implement BEFORE and AFTER behaviour
in UPDATE ... RETURNING that the old thread could not accomplish a
decade ago.

> Presumably
> something in the planner would turn that into something the executor
> could handle, which might just end up being the existing functions
> anyway.

If the current patch's functions can serve the needs of the SQL syntax
variant, that'd be a neat win!

Best regards,
Gurjeet
http://Gurje.et