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

> 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:

> 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:
>
>> 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
>>
>