Re: [PATCH v1] [BUG #19516] Skip whole-row projection shortcut for OLD/NEW returning type
Marko Grujic <markoog@gmail.com>
From: Marko Grujic <markoog@gmail.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Marko Grujic <marko.grujic@enterprisedb.com>,
Dean Rasheed <dean.a.rasheed@gmail.com>
Date: 2026-06-10T12:53:43Z
Lists: pgsql-hackers
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 >
Commits
-
Fix parsing of parenthesised OLD/NEW in RETURNING list.
- 9108fed3eda9 18 (unreleased) landed
- 79c65b9d97fe 19 (unreleased) landed