Horton

Thread

Commits

  1. Fix parsing of parenthesised OLD/NEW in RETURNING list.

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

    Marko Grujic <marko.grujic@enterprisedb.com> — 2026-06-10T10:48:20Z

    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.

Also added tests that exercise this.

Thanks,
Marko

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

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2026-06-10T11:54:06Z

    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

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

    Marko Grujic <marko.grujic@enterprisedb.com> — 2026-06-10T12:23:14Z

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

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

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2026-06-10T12:47:32Z

    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

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

    Marko Grujic <markoog@gmail.com> — 2026-06-10T12:53:43Z

    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
>

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

    Marko Grujic <marko.grujic@enterprisedb.com> — 2026-06-10T14:08:38Z

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

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

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2026-06-11T11:24:14Z

    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