Обсуждение: [BUG] Excessive memory usage with update on STORED generated columns.

Hi!


My colleagues found that a queries like that:

\timing

DROP TABLE IF EXISTS t;

CREATE TABLE t (
     id int,
     a int,
     b int,
     g text GENERATED ALWAYS AS (
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0') ||
         lpad((CASE WHEN a <> 0 THEN a ELSE b END)::text, 10, '0')
     ) STORED
);

INSERT INTO t
SELECT 1, 100, 0
FROM generate_series(1, 1000000);

UPDATE t SET id = 2; -- < problem query

lead to excessive memory consumption up to 10Gb in this example and
query execution time up to ~1,5min.

Bisect shows that the problem appeared after commit 83ea6c540
(Virtual generated columns).

Before this commit the update query took only ~8s and the memory
consumption did not exceed 150Mb for this backend.
MemoryContextStats reports only a small amount of memory usage, while
malloc_stats() confirms large allocations outside PostgreSQL memory
contexts.

With help of massif tool i found repeated allocations originating from:

     ExecInitGenerated
       → build_column_default
         → stringToNode

This indicates that generated expressions are reparsed multiple times,
once per row to be updated instead of being reused.

There is a problem call stack during UPDATE t SET id = 2;
execution: see attached bt.txt, please.

Before the above-mentioned commit, ExecInitGenerated() was effectively
invoked once per ResultRelInfo, so this behavior was not observable.

I would like to propose a fix that add a caching of the the parsed
expression trees (Node *) in ResultRelInfo, so that build_column_default()
and stringToNode() are executed at most once per attribute per query.

With this fix, the query execution time
and memory consumption return to normal:

postgres=# UPDATE t SET id = 2;
UPDATE 1000000
Time: 11522,621 ms (00:11,523)


A patch for this approach for current master is attached here.


Would be glad for any feedback.

Best regards,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, 30 Mar 2026 at 16:25, Anton A. Melnikov
<a.melnikov@postgrespro.ru> wrote:
>
> Hi!
>
>
> My colleagues found that a queries like that:
[...]
> lead to excessive memory consumption up to 10Gb in this example and
> query execution time up to ~1,5min.
>
> Bisect shows that the problem appeared after commit 83ea6c540
> (Virtual generated columns).

Yep, that looks about accurate. Thanks for the report and initial triage!

> This indicates that generated expressions are reparsed multiple times,
> once per row to be updated instead of being reused.
[...]
> I would like to propose a fix that add a caching of the the parsed
> expression trees (Node *) in ResultRelInfo, so that build_column_default()
> and stringToNode() are executed at most once per attribute per query.
>
> With this fix, the query execution time
> and memory consumption return to normal:

I think that fixes the wrong issue. Specifically, ExecInitGenerated
mentions that it expects to be called just once per ResultRelInfo, and
your code adds code to allow it to be called more than once without
causing more problems. While it does solve the complaint, it's still
executing more work than it should. Additionally, though it's not a
failing of the patch per se, it changes the layout of an existing
struct, and so it isn't backportable.

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.

So, I think the attached patch is a more appropriate fix, it avoids
calling into ExecInitGenerated at all when no column included in
expressions was updated. It also adds an assertion that the function
isn't called again once the field has been initialized. It also has
the benefit of being backportable.


Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

Hi!

On 30.03.2026 18:26, Matthias van de Meent wrote:
> So, I think the attached patch is a more appropriate fix, it avoids
> calling into ExecInitGenerated at all when no column included in
> expressions was updated. It also adds an assertion that the function
> isn't called again once the field has been initialized. It also has
> the benefit of being backportable.
> 

Agreed that this "one floor higher" fix is rather ​​simpler
and much more native than my variant with caching.

Thanks a lot for reply!

Best regards,

-- 
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

"Anton A. Melnikov" <a.melnikov@postgrespro.ru> writes:
> With help of massif tool i found repeated allocations originating from:
>      ExecInitGenerated
>        → build_column_default
>          → stringToNode
> This indicates that generated expressions are reparsed multiple times,
> once per row to be updated instead of being reused.

Hm.

> I would like to propose a fix that add a caching of the the parsed
> expression trees (Node *) in ResultRelInfo, so that build_column_default()
> and stringToNode() are executed at most once per attribute per query.

That seems pretty brute-force.  Both ExecInitGenerated and its callers
are still of the opinion that it's only done once per query; what is
broken about that?

[ pokes at it... ]  It looks like the problem in this example is
that the trivial-case path

    if (ri_NumGeneratedNeeded == 0)
    {
        /* didn't need it after all */
        pfree(ri_GeneratedExprs);
        ri_GeneratedExprs = NULL;
    }

results in storing NULL into ri_GeneratedExprsU, so that the
next time through ExecComputeStoredGenerated, we still think
we need to do ExecInitGenerated:

        if (resultRelInfo->ri_GeneratedExprsU == NULL)
            ExecInitGenerated(resultRelInfo, estate, cmdtype);

So I think the correct fix is that there needs to be a separate
boolean tracking whether this work has been done, akin to
ExecGetExtraUpdatedCols's use of ri_extraUpdatedCols_valid.

            regards, tom lane

On Mon, 30 Mar 2026 at 20:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> So I think the correct fix is that there needs to be a separate
> boolean tracking whether this work has been done, akin to
> ExecGetExtraUpdatedCols's use of ri_extraUpdatedCols_valid.

Why would it need a new boolean? ri_extraUpdatedCols_valid tracks
exactly whether we've already gone through ExecInitGenerated(...,
CMD_UPDATE), and in doing so if both ri_extraUpdatedCols,
ri_GeneratedExprsU, and ri_NumGeneratedNeededU are valid or whether
they still need to be populated. Adding a new boolean would therefore
be rather duplicative.

See also the patch of a few hours ago at [0].

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)


[0] https://postgr.es/m/CAEze2Wh%2B_C8LtmiMRb58df%3DA1PrBVmMnYMOfbBUk9c%3Dm99CN%2Bw%40mail.gmail.com

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.

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 think the right fix is to have three
booleans specifically reflecting the validity of ri_GeneratedExprsU,
ri_GeneratedExprsI, and ri_extraUpdatedCols.

> It also has the benefit of being backportable.

Backportability is definitely a concern, but in this case it
looks like we could get away with squeezing the additional
fields into what is now padding space:

typedef struct ResultRelInfo
{
...
    /* For UPDATE, attnums of generated columns to be computed */
    Bitmapset  *ri_extraUpdatedCols;
    /* true if the above has been computed */
    bool        ri_extraUpdatedCols_valid;

    /* Projection to generate new tuple in an INSERT/UPDATE */
    ProjectionInfo *ri_projectNew;

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.

            regards, tom lane

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)