> 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 <[email protected]> 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 <[email protected]> > wrote: > >> On Wed, 10 Jun 2026 at 13:23, Marko Grujic >> <[email protected]> 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 >> >
0001-Fix-mixup-when-RETURNING-parenthesized-OLD-NEW-from-.patch
Description: Binary data
