MIME-Version: 1.0
References: <ddc34dbd-1efc-4710-824c-e101e7eb63e6@postgrespro.ru>
 <CAEze2Wh+_C8LtmiMRb58df=A1PrBVmMnYMOfbBUk9c=m99CN+w@mail.gmail.com>
 <2546907.1774898070@sss.pgh.pa.us>
In-Reply-To: <2546907.1774898070@sss.pgh.pa.us>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 30 Mar 2026 22:14:25 +0200
Message-ID: 
 <CAEze2WhrYY6J=0X54EQbTH0POWPTSnWeAwNJd01vcoWoXhtiig@mail.gmail.com>
Subject: Re: [BUG] Excessive memory usage with update on STORED generated
 columns.
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "Anton A. Melnikov" <a.melnikov@postgrespro.ru>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Peter Eisentraut <peter@eisentraut.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAEze2WhrYY6J%3D0X54EQbTH0POWPTSnWeAwNJd01vcoWoXhtiig%40mail.gmail.com>
Precedence: bulk

On Mon, 30 Mar 2026 at 21:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Matthias van de Meent <boekewurm+postgres@gmail.com> 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)