| From: | Marko Grujic <markoog(at)gmail(dot)com> |
|---|---|
| To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Cc: | Marko Grujic <marko(dot)grujic(at)enterprisedb(dot)com>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
| Subject: | Re: [PATCH v1] [BUG #19516] Skip whole-row projection shortcut for OLD/NEW returning type |
| Date: | 2026-06-10 12:53:43 |
| Message-ID: | CAKQrOnoebK_P9oiTW_3Ni+pSZhh2DPNczvqYBpcNJXoqGZDM0A@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| 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(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:
> On Wed, 10 Jun 2026 at 13:23, Marko Grujic
> <marko(dot)grujic(at)enterprisedb(dot)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
>
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nazir Bilal Yavuz | 2026-06-10 12:59:20 | ci: Generate crashlogs on Windows |
| Previous Message | Ashutosh Bapat | 2026-06-10 12:49:49 | Re: [Bug] Add the missing RTE_GRAPH_TABLE case to transformLockingClause() |