public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matthias van de Meent <[email protected]>
To: Tom Lane <[email protected]>
Cc: Anton A. Melnikov <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: [BUG] Excessive memory usage with update on STORED generated columns.
Date: Mon, 30 Mar 2026 22:14:25 +0200
Message-ID: <CAEze2WhrYY6J=0X54EQbTH0POWPTSnWeAwNJd01vcoWoXhtiig@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAEze2Wh+_C8LtmiMRb58df=A1PrBVmMnYMOfbBUk9c=m99CN+w@mail.gmail.com>
<[email protected]>
On Mon, 30 Mar 2026 at 21:14, Tom Lane <[email protected]> wrote:
>
> Matthias van de Meent <[email protected]> writes:
> > The actual issue is that ExecComputeStoredGenerated uses
> > ri_GeneratedExprsU's NULL-ness to check whether the generated columns'
> > expressions have been initialized, whilst for UPDATE ResultRelInfos
> > the initialized-indicator is stored in ri_extraUpdatedCols_valid.
>
> Sorry, I missed that you'd already responded.
That's allright.
> I don't like using ri_extraUpdatedCols_valid here: it requires callers
> to know more than they should about how ExecInitGenerated works, and
> it does not fix the comparable performance problem that probably
> exists in the INSERT path.
I'm not sure which comparable performance problem you're referring to;
I don't see one mentioned, and INSERT doesn't have the same issue
because we never call into ExecInitGenerated for inserts unless 1.)
there are any generated stored columns, and 2.) it hasn't been called
already for this ResultRelInfo (*) by checking
nonnull-after-initialization ri_GeneratedExprsI.
(*) the issue here, of course, being that we *do* call
ExecInitGenerated many times in the same query for the same RRI when
UPDATE only changes columns that aren't referenced in generated
columns, this caused due to an incorrect check which checks the wrong
field.
> I think the right fix is to have three
> booleans specifically reflecting the validity of ri_GeneratedExprsU,
> ri_GeneratedExprsI, and ri_extraUpdatedCols.
I'll defer to Peter as primary author of this code, but personally I
think that it isn't needed:
In the insert case (ri_GeneratedExprsI) the field is always non-null
once the generated columns' exprstates are initialized, whilst in the
update case the current boolean is indicative of the fields having
been populated. Yes, it might benefit from better naming, but the
boolean itself is already sufficient to indicate that you can rely on
the update-related fields to be populated by ExecInitGenerated.
> There's at least three bytes free after ri_extraUpdatedCols_valid,
> so we could cram two more bools in there without an ABI break.
> Admittedly, it's not pretty that those bools wouldn't be close
> to the fields they describe. But we could improve that in HEAD
> with some more-substantial reordering of the contents of
> ResultRelInfo; it's not like the current ordering has much rhyme
> or reason to it.
I personally try to avoid adding new fields in backbranches if that's
possible (even in alignment gaps), so that if we actually must add
data to the struct we still have space to pick from.
Kind regards,
Matthias van de Meent
Databricks (https://www.databricks.com)
view thread (4+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [BUG] Excessive memory usage with update on STORED generated columns.
In-Reply-To: <CAEze2WhrYY6J=0X54EQbTH0POWPTSnWeAwNJd01vcoWoXhtiig@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox