[PATCH v1] [BUG #19516] Skip whole-row projection shortcut for OLD/NEW returning type

On Wed, 10 Jun 2026 at 11:48, Marko Grujic
<marko.grujic@enterprisedb.com> wrote:

Hi, submitting a patch for bug #19516 (held in moderation queue atm).

Looks like the root cause is that the shortcut taken in ParseComplexProjection
loses the `varreturningtype` flag, causing it to default to `VAR_RETURNING_DEFAULT`.

Consequently, the default RETURNING behavior is applied, which is strictly wrong
when a user typed (old).col on INSERT/UPDATE or (new).col on DELETE.

To fix this simply skip the shortcut when varreturningtype is set to VAR_RETURNING_OLD/VAR_RETURNING_NEW.

Nice catch!

I actually think that the root cause of the problem is
ParseComplexProjection()'s use of GetNSItemByRangeTablePosn(), which
returns the wrong nsitem because it only compares varno and
varlevelsup, not varreturningtype.

So I think a better solution would be to add a new function
GetNSItemByVar(), similar to GetNSItemByRangeTablePosn() except that
it would take a Var and return the matching nsitem, taking into
account varreturningtype. Then ParseComplexProjection() could use that
to return a Var with varreturningtype set correctly.

This makes me wonder if there are any other users of
GetNSItemByRangeTablePosn() that have a similar problem.

Regards,
Dean

Good point; the patch I sent is a minimal bandaid, while the actual root
cause is GetNSItemByRangeTablePosn not handling varreturningtype.

So I think a better solution would be to add a new function

GetNSItemByVar(), similar to GetNSItemByRangeTablePosn()

Agreed that this is a better solution than the present patch.

That said what do you think about retrofitting GetNSItemByRangeTablePosn to
accept VarReturningType too (other callers can pass VAR_RETURNING_DEFAULT)?

Thanks,
Marko

On Wed, Jun 10, 2026 at 1:54 PM Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:

On Wed, 10 Jun 2026 at 13:23, Marko Grujic
<marko.grujic@enterprisedb.com> wrote:

Agreed that this is a better solution than the present patch.

That said what do you think about retrofitting GetNSItemByRangeTablePosn to accept VarReturningType too (other callers can pass VAR_RETURNING_DEFAULT)?

I don't like changing GetNSItemByRangeTablePosn() in back-branches
because some external code might be using it, though I didn't find any
examples on PGXN.

Some users of GetNSItemByRangeTablePosn() look fine, so there's no
need to change them -- for example, the MERGE parsing code, which
isn't starting from a Var, and isn't processing something that could
be in a RETURNING list.

OTOH, I think ExpandRowReference() does need updating -- at least the
comment suggests that (old.*).* will go through it, rather than
ParseComplexProjection(), so wouldn't be fixed by your original patch.

The one I'm unsure about is coerce_record_to_complex(). I can't manage
to come up with an example that breaks it, but it certainly looks like
it should be updated.

Regards,
Dean

Agreed; there are 4 callers of GetNSItemByRangeTablePosn, 3 of which are
Var-shaped, so adding a new GetNSItemByVar makes sense.

I don't like changing GetNSItemByRangeTablePosn() in back-branches

Given that the remaining caller (the MERGE parsing code) hard-codes
the sublevels_up arg to 0, there's also potential to simplify
GetNSItemByRangeTablePosn or even just inline it.,
but probably best to avoid for the sake of back-portability.

This makes me wonder if there are any other users of

GetNSItemByRangeTablePosn() that have a similar problem.

OTOH, I think ExpandRowReference() does need updating

Indeed, that seems to be the case

postgres=# insert into t values (2, 'two') returning (old).*, old.*,
(new).*, new.*;
a | b | a | b | a | b | a | b
---+-----+---+---+---+-----+---+-----
2 | two | | | 2 | two | 2 | two
(1 row)

Thanks,
Marko

On Wed, Jun 10, 2026 at 2:47 PM Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:

So I think a better solution would be to add a new function

GetNSItemByVar(), similar to GetNSItemByRangeTablePosn()

Alright, produced a v2 patch with this approach now (attached).

It also fixes the star expansion when RETURNING parenthesized OLD/NEW rows
(added test cases to exercise that as well).

Let me know how it looks.

Thanks,
Marko

On Wed, Jun 10, 2026 at 2:53 PM Marko Grujic <markoog@gmail.com> wrote:

On Wed, 10 Jun 2026 at 15:08, Marko Grujic
<marko.grujic@enterprisedb.com> wrote:

So I think a better solution would be to add a new function

GetNSItemByVar(), similar to GetNSItemByRangeTablePosn()

Alright, produced a v2 patch with this approach now (attached).

It also fixes the star expansion when RETURNING parenthesized OLD/NEW rows (added test cases to exercise that as well).

Let me know how it looks.

Looks good, thanks.

Pushed and back-patched to v18.

I condensed the tests down a bit, and added a comment note to
GetNSItemByRangeTablePosn() suggesting that callers should consider
GetNSItemByVar() instead, in case any new code is written that might
need it.

Regards,
Dean