Re: Adding OLD/NEW support to RETURNING

On Sat, 24 Feb 2024 at 17:52, Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> Presumably the 2013 thread went nowhere because of some implementation
> problems, not simply because the author lost interest and disappeared?
> Would it be helpful for this new patch to briefly summarize what the
> main issues were and how this new approach deals with that? (It's hard
> to say if reading the old thread is necessary/helpful for understanding
> this new patch, and time is a scarce resource.)

Thanks for looking!

The 2013 patch got fairly far down a particular implementation path
(adding a new kind of RTE called RTE_ALIAS) before Robert reviewed it
[1]. He pointed out various specific issues, as well as questioning
the overall approach, and suggesting a different approach that would
have involved significant rewriting (this is essentially the approach
that I have taken, adding a new field to Var nodes).

[1] https://www.postgresql.org/message-id/CA%2BTgmoY5EXE-YKMV7CsdSFj-noyZz%3D2z45sgyJX5Y84rO3RnWQ%40mail.gmail.com

The thread kind-of petered out shortly after that, with the conclusion
that the patch needed a pretty significant redesign and rewrite.


> No opinion on whether varreturningtype is the right approach - it sounds
> like it's working better than the 2013 patch, but I won't pretend my
> knowledge of this code is sufficient to make judgments beyond that.
>
> > For the most part, the rewriter and parser are then untouched, except
> > for a couple of places necessary to ensure that the new field makes it
> > through correctly. In particular, none of this affects the shape of
> > the final plan produced. All of the work to support the new Var
> > returning type is done in the executor.

(Of course, I meant the rewriter and the *planner* are largely untouched.)

I think this is one of the main advantages of this approach. The 2013
design, adding a new RTE kind, required changes all over the place,
including lots of hacking in the planner.


> > This turns out to be relatively straightforward, except for
> > cross-partition updates, which was a little trickier since the tuple
> > format of the old row isn't necessarily compatible with the new row,
> > which is in a different partition table and so might have a different
> > column order.
>
> So we just "remap" the attributes, right?

Right. That's what the majority of the new code in ExecDelete() and
ExecInsert() is for. It's not that complicated, but it did require a
bit of care.


> What are the challenges for supporting OLD/NEW for foreign tables?

I didn't really look at that in any detail, but I don't think it
should be too hard. It's not something I want to tackle now though,
because the patch is big enough already.


> I think OLD/NEW with a way to define a custom alias when needed seems
> acceptable. Or at least I can't think of a clearly better solution. Yes,
> using some other name might not have this problem, but I guess we'd have
> to pick an existing keyword or add one. And Tom didn't seem thrilled
> with reserving a keyword in 2013 ...
>
> Plus I think there's value in consistency, and OLD/NEW seems way more
> natural that BEFORE/AFTER.

Yes, I think OLD/NEW are much nicer too.

Attached is a new patch, now with docs (no other code changes).

Regards,
Dean