Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Small terminology fixes in comments

  2. Require UPDATE permission on FOR PORTION OF column

  1. FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-04-07T08:42:42Z

    Hi hackers,

It appears that this is a bug where UPDATE FOR ... PORTION OF fails to
recompute GENERATED ALWAYS AS ... STORED columns whose expressions
reference the range column being narrowed. Please find the repro below.

postgres=# CREATE TABLE t (id int, valid_at int4range NOT NULL, val int,
    range_len int GENERATED ALWAYS AS (upper(valid_at) - lower(valid_at))
STORED);
INSERT INTO t VALUES (1, '[1,100)', 10);
UPDATE t FOR PORTION OF valid_at FROM 30 TO 70 SET val = 99;
SELECT *, upper(valid_at) - lower(valid_at) AS expected FROM t ORDER BY
valid_at;
CREATE TABLE
INSERT 0 1
UPDATE 1
 id | valid_at | val | range_len | expected
----+----------+-----+-----------+----------
  1 | [1,30)   |  10 |        29 |       29
  1 | [30,70)  |  99 |        99 |       40
  1 | [70,100) |  10 |        30 |       30
(3 rows)


The updated row [30,70) retains the stale range_len = 99 from the original
[1,100) range. The leftover inserts are correct because CMD_INSERT
unconditionally recomputes all generated columns. Virtual generated columns
are not affected and are computed correctly because they're evaluated at
read time from the actual stored valid_at value.

Further looking at the code it appears, In transformForPortionOfClause(),
the range column is intentionally not added to perminfo->updatedCols. Since
the range column is absent from updatedCols, any generated stored column
whose expression depends solely on the range column (e.g., upper(valid_at)
- lower(valid_at)) is skipped. Therefore, its expression is never prepared
and never recomputed during the FPO update.

Attached a draft patch that has the test scenario and a fix to address this
issue. In ExecInitGenerated, after retrieving updatedCols, the patch
additionally checks whether the owning ModifyTableState contains an FPO
clause. If it does, the attribute number (attno) of the range column is
added to updatedCols.

Thanks,
Satya

  2. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-04-10T09:10:30Z

    Hi.

ExecUpdate->ExecUpdateEpilogue->ExecForPortionOfLeftovers
In ExecForPortionOfLeftovers, we have
"""
if (!resultRelInfo->ri_forPortionOf)
    {
        /*
         * If we don't have a ForPortionOfState yet, we must be a partition
         * child being hit for the first time. Make a copy from the root, with
         * our own tupleTableSlot. We do this lazily so that we don't pay the
         * price of unused partitions.
         */
        ForPortionOfState *leafState = makeNode(ForPortionOfState);
}
"""
We reached the end of ExecUpdate, and then suddenly initialized
resultRelInfo->ri_forPortionOf.
That seems wrong; we should initialize resultRelInfo->ri_forPortionOf
earlier so other places can use that information, such as
ForPortionOfState->fp_rangeAttno.

We can initialize ForPortionOfState right after ExecModifyTable:
"""
/* If it's not the same as last time, we need to locate the rel */
if (resultoid != node->mt_lastResultOid)
    resultRelInfo = ExecLookupResultRelByOid(node, resultoid,
                                                false, true);
"""

In ExecForPortionOfLeftovers, we should use ForPortionOfState more and
ForPortionOfExpr less.
(ForPortionOfExpr and ForPortionOfState share some overlapping information;
maybe we can eliminate some common fields or put ForPortionOfExpr into
ForPortionOfState).


As noted in [1], the FOR PORTION OF column is physically modified,
even though we didn't require explicit UPDATE privileges,
we failed to track this column in ExecGetUpdatedCols and
ExecGetExtraUpdatedCols.
This omission directly impacts the ExecInsertIndexTuples ->
index_unchanged_by_update -> ExecGetExtraUpdatedCols execution path.
We should ensure ExecGetExtraUpdatedCols also accounts for this column.
Otherwise, we need a clearer explanation for why
index_unchanged_by_update can safely ignore a column that is being
physically modified.

I have added regression test cases for CREATE TRIGGER UPDATE OF column_name.

The attached patch also addressed the table inheritance issue in
https://postgr.es/m/CAHg+QDcsXsUVaZ+JwM02yDRQEi=cL_rTH_ROLDYgOx004sQu7A@mail.gmail.com

I've combined all these changes into a single patch for now, as they
seem closely related.

[1]: https://postgr.es/m/CACJufxHALFKca5SMn5DNnbrX2trPamVL6napn_nm35p15yw+rg@mail.gmail.com




--
jian
https://www.enterprisedb.com/

  3. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-04-10T22:00:57Z

    Hi Jian,

On Fri, Apr 10, 2026 at 2:11 AM jian he <jian.universality@gmail.com> wrote:

> Hi.
>
> ExecUpdate->ExecUpdateEpilogue->ExecForPortionOfLeftovers
> In ExecForPortionOfLeftovers, we have
> """
> if (!resultRelInfo->ri_forPortionOf)
>     {
>         /*
>          * If we don't have a ForPortionOfState yet, we must be a partition
>          * child being hit for the first time. Make a copy from the root,
> with
>          * our own tupleTableSlot. We do this lazily so that we don't pay
> the
>          * price of unused partitions.
>          */
>         ForPortionOfState *leafState = makeNode(ForPortionOfState);
> }
> """
> We reached the end of ExecUpdate, and then suddenly initialized
> resultRelInfo->ri_forPortionOf.
> That seems wrong; we should initialize resultRelInfo->ri_forPortionOf
> earlier so other places can use that information, such as
> ForPortionOfState->fp_rangeAttno.
>
> We can initialize ForPortionOfState right after ExecModifyTable:
> """
> /* If it's not the same as last time, we need to locate the rel */
> if (resultoid != node->mt_lastResultOid)
>     resultRelInfo = ExecLookupResultRelByOid(node, resultoid,
>                                                 false, true);
> """
>
> In ExecForPortionOfLeftovers, we should use ForPortionOfState more and
> ForPortionOfExpr less.
> (ForPortionOfExpr and ForPortionOfState share some overlapping information;
> maybe we can eliminate some common fields or put ForPortionOfExpr into
> ForPortionOfState).
>
>
> As noted in [1], the FOR PORTION OF column is physically modified,
> even though we didn't require explicit UPDATE privileges,
> we failed to track this column in ExecGetUpdatedCols and
> ExecGetExtraUpdatedCols.
> This omission directly impacts the ExecInsertIndexTuples ->
> index_unchanged_by_update -> ExecGetExtraUpdatedCols execution path.
> We should ensure ExecGetExtraUpdatedCols also accounts for this column.
> Otherwise, we need a clearer explanation for why
> index_unchanged_by_update can safely ignore a column that is being
> physically modified.
>
> I have added regression test cases for CREATE TRIGGER UPDATE OF
> column_name.
>
> The attached patch also addressed the table inheritance issue in
>
> https://postgr.es/m/CAHg+QDcsXsUVaZ+JwM02yDRQEi=cL_rTH_ROLDYgOx004sQu7A@mail.gmail.com
>
> I've combined all these changes into a single patch for now, as they
> seem closely related.
>
> [1]:
> https://postgr.es/m/CACJufxHALFKca5SMn5DNnbrX2trPamVL6napn_nm35p15yw+rg@mail.gmail.com


I applied your patch and tested. The following scenarios are now passing:
(1) table inheritance issue I reported in [1], (2) issue reported in this
thread.

Following are still failing:

(1) instead of triggers + views, mentioned in the thread [2], it has both
the test case and the fix.




(2) For Portion Of DELETE loses rows when a BEFORE INSERT trigger returns
NULL

DROP TABLE IF EXISTS subscriptions CASCADE;
CREATE TABLE subscriptions (
    sub_id   int,
    period   int4range NOT NULL,
    plan     text
);

CREATE OR REPLACE FUNCTION reject_new_subscriptions() RETURNS trigger AS $$
BEGIN
    -- Business rule: no new subscription rows allowed via INSERT.
    RETURN NULL;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER no_new_subs
    BEFORE INSERT ON subscriptions
    FOR EACH ROW EXECUTE FUNCTION reject_new_subscriptions();

-- Pre-existing row (bypass trigger to seed it).
ALTER TABLE subscriptions DISABLE TRIGGER no_new_subs;
INSERT INTO subscriptions VALUES (1, '[1,100)', 'premium');
ALTER TABLE subscriptions ENABLE TRIGGER no_new_subs;

SELECT * FROM subscriptions;
-- 1 row: (1, [1,100), premium)

-- Delete just the [40,60) slice.
DELETE FROM subscriptions FOR PORTION OF period FROM 40 TO 60;

SELECT * FROM subscriptions ORDER BY period;
-- Should be two rows: [1,40) and [60,100)
-- Actually: 0 rows.  The whole subscription vanished.

SELECT count(*) AS remaining FROM subscriptions;
-- Expected 2, got 0.

(3) FPO UPDATE loses leftovers the same way

DROP TABLE IF EXISTS room_bookings CASCADE;
CREATE TABLE room_bookings (
    booking_id int,
    slot       int4range NOT NULL,
    note       text
);

CREATE OR REPLACE FUNCTION block_booking_inserts() RETURNS trigger AS $$
BEGIN
    -- Business rule: bookings created only through an API layer.
    RETURN NULL;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER booking_guard
    BEFORE INSERT ON room_bookings
    FOR EACH ROW EXECUTE FUNCTION block_booking_inserts();

ALTER TABLE room_bookings DISABLE TRIGGER booking_guard;
INSERT INTO room_bookings VALUES (1, '[1,100)', 'team meeting');
ALTER TABLE room_bookings ENABLE TRIGGER booking_guard;

SELECT * FROM room_bookings;
-- 1 row: (1, [1,100), team meeting)

-- Shorten the meeting to only [40,60).
UPDATE room_bookings FOR PORTION OF slot FROM 40 TO 60 SET note =
'shortened';

SELECT * FROM room_bookings ORDER BY slot;
-- Should be three rows:
--   [1,40)   team meeting
--   [40,60)  shortened
--   [60,100) team meeting
-- Actually: only the [40,60) row survives.

SELECT count(*) AS remaining FROM room_bookings;
-- Expected 3, got 1.


[1]:
https://postgr.es/m/CAHg+QDcsXsUVaZ+JwM02yDRQEi=cL_rTH_ROLDYgOx004sQu7A@mail.gmail.com
[2]:
https://www.postgresql.org/message-id/flat/CACJufxHALFKca5SMn5DNnbrX2trPamVL6napn_nm35p15yw%2Brg%40mail.gmail.com#ab4216dc6828fb0d7ada48aab9e330d0

Thanks,
Satya

  4. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-04-11T08:36:51Z

    On Sat, Apr 11, 2026 at 6:01 AM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:
>

> Following are still failing:
>
> (1) instead of triggers + views, mentioned in the thread [2], it has both the test case and the fix.
>
I will check and reply in that thread.

>
> (2) For Portion Of DELETE loses rows when a BEFORE INSERT trigger returns NULL
>
> DROP TABLE IF EXISTS subscriptions CASCADE;
> CREATE TABLE subscriptions (
>     sub_id   int,
>     period   int4range NOT NULL,
>     plan     text
> );
>
> CREATE OR REPLACE FUNCTION reject_new_subscriptions() RETURNS trigger AS $$
> BEGIN
>     -- Business rule: no new subscription rows allowed via INSERT.
>     RETURN NULL;
> END;
> $$ LANGUAGE plpgsql;
>
> CREATE TRIGGER no_new_subs
>     BEFORE INSERT ON subscriptions
>     FOR EACH ROW EXECUTE FUNCTION reject_new_subscriptions();
>
> -- Pre-existing row (bypass trigger to seed it).
> ALTER TABLE subscriptions DISABLE TRIGGER no_new_subs;
> INSERT INTO subscriptions VALUES (1, '[1,100)', 'premium');
> ALTER TABLE subscriptions ENABLE TRIGGER no_new_subs;
>
> SELECT * FROM subscriptions;
> -- 1 row: (1, [1,100), premium)
>
> -- Delete just the [40,60) slice.
> DELETE FROM subscriptions FOR PORTION OF period FROM 40 TO 60;
>
> SELECT * FROM subscriptions ORDER BY period;
> -- Should be two rows: [1,40) and [60,100)
> -- Actually: 0 rows.  The whole subscription vanished.
>
> SELECT count(*) AS remaining FROM subscriptions;
> -- Expected 2, got 0.
>

I think this is expected.
https://www.postgresql.org/docs/devel/sql-delete.html says
<<>>
When FOR PORTION OF is used, this can result in users who don't have INSERT
privileges firing INSERT triggers. This should be considered when using SECURITY
DEFINER trigger functions.
<<>>

We first tried inserting [1,40) and [60,100), but they were rejected
and not inserted
because the trigger function reject_new_subscriptions returned NULL.

See ExecInsert:
``````
    if (resultRelInfo->ri_TrigDesc &&
        resultRelInfo->ri_TrigDesc->trig_insert_before_row)
    {
        /* Flush any pending inserts, so rows are visible to the triggers */
        if (estate->es_insert_pending_result_relations != NIL)
            ExecPendingInserts(estate);
        if (!ExecBRInsertTriggers(estate, resultRelInfo, slot))
            return NULL;        /* "do nothing" */
    }
``````

> (3) FPO UPDATE loses leftovers the same way
>
> -- Shorten the meeting to only [40,60).
> UPDATE room_bookings FOR PORTION OF slot FROM 40 TO 60 SET note = 'shortened';
>
> SELECT * FROM room_bookings ORDER BY slot;
> -- Should be three rows:
> --   [1,40)   team meeting
> --   [40,60)  shortened
> --   [60,100) team meeting
> -- Actually: only the [40,60) row survives.
>

For the same reason as above, I think the current behavior is correct.



--
jian
https://www.enterprisedb.com/

  5. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-15T20:59:21Z

    On Fri, Apr 10, 2026 at 3:01 PM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:
>
>> I've combined all these changes into a single patch for now, as they
>> seem closely related.
>>
>> [1]: https://postgr.es/m/CACJufxHALFKca5SMn5DNnbrX2trPamVL6napn_nm35p15yw+rg@mail.gmail.com
>
> I applied your patch and tested. The following scenarios are now passing:  (1) table inheritance issue I reported in [1], (2) issue reported in this thread.

They look good to me too. I read through the patch and made some
stylistic changes, as well as some grammar/typo fixes. In
ExecInitForPortionOf I tried to group things for each case more
closely, instead of separate disconnected branches. I also added/moved
some comments. Please see the attached.

There's something I think we could still improve: we omit the
valid-time in updatedCols, since that bitmapset is for permissions
checking (at least originally), but now other features are using it as
well. Our fix adds special logic to consider the valid-at column for
GENERATED column dependencies and more special logic for UPDATE OF
triggers. Perhaps we should add the attno to updatedCols after all,
and put the special logic in the permissions check instead? That seems
simpler and more robust. Or maybe it's time to have separate
bitmapsets, one for permissions and another for everything else? What
do you think?

I'm not sure we should be consolidating these fixes into one patch.
But I tried to call out all the issues in my commit message.

> Following are still failing:
>
> (1) instead of triggers + views, mentioned in the thread [2], it has both the test case and the fix.

I'll follow up there.

> (2) For Portion Of DELETE loses rows when a BEFORE INSERT trigger returns NULL
>
> ...
>
> (3) FPO UPDATE loses leftovers the same way

I agree with jian that this is the correct behavior. The inserts are
supposed to fire triggers, and if a trigger signals to cancel the
insert, that's what we should do.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  6. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-04-17T08:13:28Z

    On Thu, Apr 16, 2026 at 4:59 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> There's something I think we could still improve: we omit the
> valid-time in updatedCols, since that bitmapset is for permissions
> checking (at least originally), but now other features are using it as
> well. Our fix adds special logic to consider the valid-at column for
> GENERATED column dependencies and more special logic for UPDATE OF
> triggers. Perhaps we should add the attno to updatedCols after all,
> and put the special logic in the permissions check instead? That seems
> simpler and more robust. Or maybe it's time to have separate
> bitmapsets, one for permissions and another for everything else? What
> do you think?
>

+ /*
+ * For UPDATE ... FOR PORTION OF, the range column is also being modified
+ * (narrowed via intersection), but it is not included in updatedCols
+ * because the user does not need UPDATE permission on it.  We must
+ * account for it here so that generated columns referencing the range
+ * column are recomputed.
+ */
+ if (resultRelInfo->ri_forPortionOf)
+ {
+ AttrNumber rangeAttno = resultRelInfo->ri_forPortionOf->fp_rangeAttno;
+
+ updatedCols = bms_add_member(bms_copy(updatedCols),
+ rangeAttno - FirstLowInvalidHeapAttributeNumber);
+ }
+
Putting the above into ExecGetUpdatedCols would be more neat.
InitPlan->ExecCheckPermissions happened earlier than ExecGetUpdatedCols,
So I think it will work.

Another reason to do it this way is that some places only call
ExecGetUpdatedCols, not ExecGetExtraUpdatedCols.
Put the above into ExecGetUpdatedCols, then we don't need to worry
about whether ExecGetExtraUpdatedCols is called.

ExecGetExtraUpdatedCols saves the ri_extraUpdatedCols to estate->es_query_cxt.
For ExecGetUpdatedCols, we can do the same: save the FOR PORTION OF
column to estate->es_query_cxt.
Please find the attached diff, which is based on your V3 patch.


ExecForPortionOfLeftovers
    /*
     * Get the range's type cache entry. This is worth caching for the whole
     * UPDATE/DELETE as range functions do.
     */
    typcache = fpoState->fp_leftoverstypcache;
    if (typcache == NULL)
    {
        typcache = lookup_type_cache(forPortionOf->rangeType, 0);
        fpoState->fp_leftoverstypcache = typcache;
    }
It seems fp_leftoverstypcache is never being used?
place it to ExecInitModifyTable would be better, i think.

    /*
     * Get the ranges to the left/right of the targeted range. We call a SETOF
     * support function and insert as many temporal leftovers as it gives us.
     * Although rangetypes have 0/1/2 leftovers, multiranges have 0/1, and
     * other types may have more.
     */
Currently, we only support anymultirange and anyrange. so here
"and other types may have more." is kind of wrong?



--
jian
https://www.enterprisedb.com/

  7. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-19T20:10:06Z

    On Fri, Apr 17, 2026 at 1:14 AM jian he <jian.universality@gmail.com> wrote:
>
> On Thu, Apr 16, 2026 at 4:59 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > There's something I think we could still improve: we omit the
> > valid-time in updatedCols, since that bitmapset is for permissions
> > checking (at least originally), but now other features are using it as
> > well. Our fix adds special logic to consider the valid-at column for
> > GENERATED column dependencies and more special logic for UPDATE OF
> > triggers. Perhaps we should add the attno to updatedCols after all,
> > and put the special logic in the permissions check instead? That seems
> > simpler and more robust. Or maybe it's time to have separate
> > bitmapsets, one for permissions and another for everything else? What
> > do you think?
> >
>
> + /*
> + * For UPDATE ... FOR PORTION OF, the range column is also being modified
> + * (narrowed via intersection), but it is not included in updatedCols
> + * because the user does not need UPDATE permission on it.  We must
> + * account for it here so that generated columns referencing the range
> + * column are recomputed.
> + */
> + if (resultRelInfo->ri_forPortionOf)
> + {
> + AttrNumber rangeAttno = resultRelInfo->ri_forPortionOf->fp_rangeAttno;
> +
> + updatedCols = bms_add_member(bms_copy(updatedCols),
> + rangeAttno - FirstLowInvalidHeapAttributeNumber);
> + }
> +
> Putting the above into ExecGetUpdatedCols would be more neat.
> InitPlan->ExecCheckPermissions happened earlier than ExecGetUpdatedCols,
> So I think it will work.

Okay, I like this approach. Thank you for the patch! I'm not sure
about mutating perminfo though. It saves repeated bms allocations for
multi-row updates, but it seems a bit surprising.

> Please find the attached diff, which is based on your V3 patch.

This fix doesn't work for partitioned tables, because in
ExecGetUpdatedCols ri_forPortionOf->fp_rangeAttno has already been
mapped to the child table, but then we add it to the unmapped
bitmapset and map it again. I updated an existing FOR PORTION OF test
on partitioned tables to show this, by adding a GENERATED STORED
column. So instead you need to add to the bitmapset *after* mapping.
That's the approach I've taken here. And then mutating perminfo is
definitely wrong, because it has the parent attnos.

> ExecForPortionOfLeftovers
>     /*
>      * Get the range's type cache entry. This is worth caching for the whole
>      * UPDATE/DELETE as range functions do.
>      */
>     typcache = fpoState->fp_leftoverstypcache;
>     if (typcache == NULL)
>     {
>         typcache = lookup_type_cache(forPortionOf->rangeType, 0);
>         fpoState->fp_leftoverstypcache = typcache;
>     }
> It seems fp_leftoverstypcache is never being used?
> place it to ExecInitModifyTable would be better, i think.

You're right; that must have been left over from an earlier patch. But
actually the fix about preventing BEFORE triggers from changing
valid_at winds up using it. So I left it in for now.

>     /*
>      * Get the ranges to the left/right of the targeted range. We call a SETOF
>      * support function and insert as many temporal leftovers as it gives us.
>      * Although rangetypes have 0/1/2 leftovers, multiranges have 0/1, and
>      * other types may have more.
>      */
> Currently, we only support anymultirange and anyrange. so here
> "and other types may have more." is kind of wrong?

That's all we support for now, but my goal is to allow user-defined
functions to support other types. So this comment is looking forward
to that future.

v5 attached.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  8. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-04-21T03:57:59Z

    On Mon, Apr 20, 2026 at 4:10 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> v5 attached.
>
v5 will cause a segfault.

+ /*
+ * For UPDATE ... FOR PORTION OF, the range column is being modified
+ * (narrowed via intersection), but it is not included in updatedCols
+ * because the user does not need UPDATE permission on it. Now manualy
+ * add it to updatedCols. Since ri_forPortionOf->fp_rangeAttno is already
+ * mapped for the child partition, we have to add it after the mapping just
+ * above. Also that makes it unsafe to mutate perminfo. XXX: Always add the
+ * unmapped attno instead (before mapping), and mutate perminfo, to avoid
+ * repeated allocations?
+ */
+ if (relinfo->ri_forPortionOf)
+ {
+ AttrNumber rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
+
+ if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
+   updatedCols))
+ {
+ MemoryContext oldContext;
+
+ oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+ updatedCols =
+ bms_add_member(updatedCols,
+   rangeAttno - FirstLowInvalidHeapAttributeNumber);
+
+ MemoryContextSwitchTo(oldContext);
+ }
+ }
+
+ return updatedCols;

+ updatedCols =
+ bms_add_member(updatedCols,
+   rangeAttno - FirstLowInvalidHeapAttributeNumber);

Here, use "perminfo->updatedCols" not "updatedCols", otherwise segfault.
The attached diff based on v5, fixes this issue.

+ALTER TABLE temporal_partitioned_3 ADD COLUMN range_len int GENERATED
ALWAYS AS (upper(valid_at) - lower(valid_at)) STORED;
Slightly refactoring the tests will allow for easier comparison of
range_len values.



--
jian
https://www.enterprisedb.com/

  9. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-21T14:59:56Z

    On Mon, Apr 20, 2026 at 8:58 PM jian he <jian.universality@gmail.com> wrote:
>
> + updatedCols =
> + bms_add_member(updatedCols,
> +   rangeAttno - FirstLowInvalidHeapAttributeNumber);
>
> Here, use "perminfo->updatedCols" not "updatedCols", otherwise segfault.
> The attached diff based on v5, fixes this issue.
>
> +ALTER TABLE temporal_partitioned_3 ADD COLUMN range_len int GENERATED
> ALWAYS AS (upper(valid_at) - lower(valid_at)) STORED;
> Slightly refactoring the tests will allow for easier comparison of
> range_len values.

I can't reproduce a segfault. I don't see how that code would lead to
one. Can you share what you're doing to cause it? Simply running the
regression tests doesn't do it for me.

The v5 patch has quite a lot of code shared between both branches. I
was trying to avoid that in the v5 patch. Is there any way to clean it
up?

I like the how the test change makes it easy to verify the range_len column.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  10. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-04-22T01:11:18Z

    On Tue, Apr 21, 2026 at 11:00 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Mon, Apr 20, 2026 at 8:58 PM jian he <jian.universality@gmail.com> wrote:
> >
> > + updatedCols =
> > + bms_add_member(updatedCols,
> > +   rangeAttno - FirstLowInvalidHeapAttributeNumber);
> >
> > Here, use "perminfo->updatedCols" not "updatedCols", otherwise segfault.
> > The attached diff based on v5, fixes this issue.
> >
> > +ALTER TABLE temporal_partitioned_3 ADD COLUMN range_len int GENERATED
> > ALWAYS AS (upper(valid_at) - lower(valid_at)) STORED;
> > Slightly refactoring the tests will allow for easier comparison of
> > range_len values.
>
> I can't reproduce a segfault. I don't see how that code would lead to
> one. Can you share what you're doing to cause it? Simply running the
> regression tests doesn't do it for me.
>

Sorry for the noise. After cleaning the cached build directory and
rebuilding, there is now no issue.

+#include "nodes/print.h"
This should be removed from v5.

  11. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-22T18:03:18Z

    On Tue, Apr 21, 2026 at 6:11 PM jian he <jian.universality@gmail.com> wrote:
>
> > I can't reproduce a segfault. I don't see how that code would lead to
> > one. Can you share what you're doing to cause it? Simply running the
> > regression tests doesn't do it for me.
> >
>
> Sorry for the noise. After cleaning the cached build directory and
> rebuilding, there is now no issue.
>
> +#include "nodes/print.h"
> This should be removed from v5.

Good catch! I removed that line in v7 (attached). I also included your
test change to compute the range len by hand. Also a rebase was
necessary after d3bba04154.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  12. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-05T21:50:05Z

    On Wed, Apr 22, 2026 at 11:03 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Good catch! I removed that line in v7 (attached). I also included your
> test change to compute the range len by hand. Also a rebase was
> necessary after d3bba04154.

This needed a rebase. v8 attached.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  13. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Peter Eisentraut <peter@eisentraut.org> — 2026-05-06T11:39:18Z

    On 05.05.26 23:50, Paul A Jungwirth wrote:
> On Wed, Apr 22, 2026 at 11:03 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
>>
>> Good catch! I removed that line in v7 (attached). I also included your
>> test change to compute the range len by hand. Also a rebase was
>> necessary after d3bba04154.
> 
> This needed a rebase. v8 attached.

This patch fails the injection_points/isolation test for me.  It looks 
like it causes a server crash.  Check please.

  14. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-06T17:13:39Z

    On Wed, May 6, 2026 at 4:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 05.05.26 23:50, Paul A Jungwirth wrote:
> > On Wed, Apr 22, 2026 at 11:03 AM Paul A Jungwirth
> > <pj@illuminatedcomputing.com> wrote:
> >>
> >> Good catch! I removed that line in v7 (attached). I also included your
> >> test change to compute the range len by hand. Also a rebase was
> >> necessary after d3bba04154.
> >
> > This needed a rebase. v8 attached.
>
> This patch fails the injection_points/isolation test for me.  It looks
> like it causes a server crash.  Check please.

Sorry, I didn't have injection_points enabled, but now I see it too.
The attached v9 fixes it.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  15. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-05-07T01:14:47Z

    On Thu, May 7, 2026 at 1:13 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Sorry, I didn't have injection_points enabled, but now I see it too.
> The attached v9 fixes it.
>

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 1eb6b9f1f40..363830f0158 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -1408,6 +1408,7 @@ Bitmapset *
 ExecGetUpdatedCols(ResultRelInfo *relinfo, EState *estate)
 {
  RTEPermissionInfo *perminfo = GetResultRTEPermissionInfo(relinfo, estate);
+ Bitmapset *updatedCols = perminfo->updatedCols;

  if (perminfo == NULL)

----------------------------------------------------
v8 crashes because in some cases, `perminfo` is NULL and we are
``perminfo->updatedCols;``

        /*
         * If we don't have a ForPortionOfState yet, we must be a partition
         * child being hit for the first time. Make a copy from the root, with
         * our own TupleTableSlot. We do this lazily so that we don't pay the
         * price of unused partitions.
         */
        if ((((ModifyTable *) context.mtstate->ps.plan)->forPortionOf) &&
            !resultRelInfo->ri_forPortionOf)
        {
            ExecInitForPortionOf(context.mtstate, estate, resultRelInfo);
        }

the comment "partition child" seems not 100% accurate.
Since we also need to consider table inheritance.
Maybe replace it with "child table".

  16. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Chao Li <li.evan.chao@gmail.com> — 2026-05-07T04:34:43Z

    
> On May 7, 2026, at 01:13, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
> 
> On Wed, May 6, 2026 at 4:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> 
>> On 05.05.26 23:50, Paul A Jungwirth wrote:
>>> On Wed, Apr 22, 2026 at 11:03 AM Paul A Jungwirth
>>> <pj@illuminatedcomputing.com> wrote:
>>>> 
>>>> Good catch! I removed that line in v7 (attached). I also included your
>>>> test change to compute the range len by hand. Also a rebase was
>>>> necessary after d3bba04154.
>>> 
>>> This needed a rebase. v8 attached.
>> 
>> This patch fails the injection_points/isolation test for me.  It looks
>> like it causes a server crash.  Check please.
> 
> Sorry, I didn't have injection_points enabled, but now I see it too.
> The attached v9 fixes it.
> 
> Yours,
> 
> -- 
> Paul              ~{:-)
> pj@illuminatedcomputing.com
> <v9-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch>

Hi Paul,

I didn’t review this patch earlier because, from the subject, I thought it was only about recomputing generated stored columns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for the inheritance-table bug. Please see [1].

I tried applying the new tests from my patch on top of this patch, and it looks like this patch still does not fix the multi-inheritance case.

So I’d like to check with you how we should proceed. I think there are two options:

1. Keep this patch focused on the generated-column issue described in the subject, and use my patch to fix the inheritance-table bug.
2. I can continue from this patch and extend it to fix the multi-inheritance case as well.

Please let me know what you prefer.

[1] https://www.postgresql.org/message-id/4245F94D-84F1-4E05-BF81-C458A6CF9901%40gmail.com

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  17. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Chao Li <li.evan.chao@gmail.com> — 2026-05-07T05:54:52Z

    
> On May 7, 2026, at 12:34, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> 
> 
>> On May 7, 2026, at 01:13, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
>> 
>> On Wed, May 6, 2026 at 4:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>> 
>>> On 05.05.26 23:50, Paul A Jungwirth wrote:
>>>> On Wed, Apr 22, 2026 at 11:03 AM Paul A Jungwirth
>>>> <pj@illuminatedcomputing.com> wrote:
>>>>> 
>>>>> Good catch! I removed that line in v7 (attached). I also included your
>>>>> test change to compute the range len by hand. Also a rebase was
>>>>> necessary after d3bba04154.
>>>> 
>>>> This needed a rebase. v8 attached.
>>> 
>>> This patch fails the injection_points/isolation test for me.  It looks
>>> like it causes a server crash.  Check please.
>> 
>> Sorry, I didn't have injection_points enabled, but now I see it too.
>> The attached v9 fixes it.
>> 
>> Yours,
>> 
>> -- 
>> Paul              ~{:-)
>> pj@illuminatedcomputing.com
>> <v9-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch>
> 
> Hi Paul,
> 
> I didn’t review this patch earlier because, from the subject, I thought it was only about recomputing generated stored columns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for the inheritance-table bug. Please see [1].
> 
> I tried applying the new tests from my patch on top of this patch, and it looks like this patch still does not fix the multi-inheritance case.
> 
> So I’d like to check with you how we should proceed. I think there are two options:
> 
> 1. Keep this patch focused on the generated-column issue described in the subject, and use my patch to fix the inheritance-table bug.
> 2. I can continue from this patch and extend it to fix the multi-inheritance case as well.
> 
> Please let me know what you prefer.
> 
> [1] https://www.postgresql.org/message-id/4245F94D-84F1-4E05-BF81-C458A6CF9901%40gmail.com
> 

I just looked into v9 and made a fix in ExecInitForPortionOf() that resolves the bug with multi-inheritance tables. I also added a test case for that.

The inheritance-table bug affects not only UPDATE, but also DELETE, so I added test cases for DELETE as well. Please see 0002 for my changes.

To make each commit self-contained, would you mind moving the code for the inheritance-table fix to 0002? Then you can keep focusing on 0001, and I can continue working on 0002.

PFA v10 - 0001 the same as v9. 0002 fixed a bug with multi-inheritance tables.

(Note, in 0002, there is a comment format change around line 1496, that was done by pgindent.)

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  18. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Chao Li <li.evan.chao@gmail.com> — 2026-05-07T07:05:34Z

    
> On May 7, 2026, at 13:54, Chao Li <li.evan.chao@gmail.com> wrote:
> 
> 
> 
>> On May 7, 2026, at 12:34, Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>> 
>> 
>>> On May 7, 2026, at 01:13, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
>>> 
>>> On Wed, May 6, 2026 at 4:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>>> 
>>>> On 05.05.26 23:50, Paul A Jungwirth wrote:
>>>>> On Wed, Apr 22, 2026 at 11:03 AM Paul A Jungwirth
>>>>> <pj@illuminatedcomputing.com> wrote:
>>>>>> 
>>>>>> Good catch! I removed that line in v7 (attached). I also included your
>>>>>> test change to compute the range len by hand. Also a rebase was
>>>>>> necessary after d3bba04154.
>>>>> 
>>>>> This needed a rebase. v8 attached.
>>>> 
>>>> This patch fails the injection_points/isolation test for me.  It looks
>>>> like it causes a server crash.  Check please.
>>> 
>>> Sorry, I didn't have injection_points enabled, but now I see it too.
>>> The attached v9 fixes it.
>>> 
>>> Yours,
>>> 
>>> -- 
>>> Paul              ~{:-)
>>> pj@illuminatedcomputing.com
>>> <v9-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch>
>> 
>> Hi Paul,
>> 
>> I didn’t review this patch earlier because, from the subject, I thought it was only about recomputing generated stored columns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for the inheritance-table bug. Please see [1].
>> 
>> I tried applying the new tests from my patch on top of this patch, and it looks like this patch still does not fix the multi-inheritance case.
>> 
>> So I’d like to check with you how we should proceed. I think there are two options:
>> 
>> 1. Keep this patch focused on the generated-column issue described in the subject, and use my patch to fix the inheritance-table bug.
>> 2. I can continue from this patch and extend it to fix the multi-inheritance case as well.
>> 
>> Please let me know what you prefer.
>> 
>> [1] https://www.postgresql.org/message-id/4245F94D-84F1-4E05-BF81-C458A6CF9901%40gmail.com
>> 
> 
> I just looked into v9 and made a fix in ExecInitForPortionOf() that resolves the bug with multi-inheritance tables. I also added a test case for that.
> 
> The inheritance-table bug affects not only UPDATE, but also DELETE, so I added test cases for DELETE as well. Please see 0002 for my changes.
> 
> To make each commit self-contained, would you mind moving the code for the inheritance-table fix to 0002? Then you can keep focusing on 0001, and I can continue working on 0002.
> 
> PFA v10 - 0001 the same as v9. 0002 fixed a bug with multi-inheritance tables.
> 
> (Note, in 0002, there is a comment format change around line 1496, that was done by pgindent.)
> 
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
> 
> 
> 
> 
> <v10-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch><v10-0002-Fix-FOR-PORTION-OF-on-inherited-children-with-di.patch>

A few comments for 0001:

1 - execUtils.c
```
+	updatedCols = perminfo->updatedCols;
+
 	/* Map the columns to child's attribute numbers if needed. */
 	if (relinfo->ri_RootResultRelInfo)
 	{
 		TupleConversionMap *map = ExecGetRootToChildMap(relinfo, estate);
 
 		if (map)
-			return execute_attr_map_cols(map->attrMap, perminfo->updatedCols);
+			updatedCols = execute_attr_map_cols(map->attrMap, updatedCols);
+	}
+
+	/*
+	 * For UPDATE ... FOR PORTION OF, the range column is being modified
+	 * (narrowed via intersection), but it is not included in updatedCols
+	 * because the user does not need UPDATE permission on it. Now manualy
+	 * add it to updatedCols. Since ri_forPortionOf->fp_rangeAttno is already
+	 * mapped for the child partition, we have to add it after the mapping just
+	 * above. Also that makes it unsafe to mutate perminfo. XXX: Always add the
+	 * unmapped attno instead (before mapping), and mutate perminfo, to avoid
+	 * repeated allocations?
+	 */
+	if (relinfo->ri_forPortionOf)
+	{
+		AttrNumber	rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
+
+		if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
+						   updatedCols))
+		{
+			MemoryContext oldContext;
+
+			oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+			updatedCols =
+				bms_add_member(updatedCols,
+							   rangeAttno - FirstLowInvalidHeapAttributeNumber);
```

The comment explicitly says that it is unsafe to mutate perminfo, but bms_add_member() does not always allocate a new bitmapset. So if updatedCols still points to perminfo->updatedCols, then bms_add_member() may mutate perminfo->updatedCols.

To verify that, I added Assert(updatedCols != perminfo->updatedCols); right after the bms_add_member(), then ran "make check". A lot of tests failed, which seems to confirm that perminfo->updatedCols was being mutated.

So, I think we should make a copy the bitmapset before bms_add_member when needed, to make sure perminfo is not mutated, something like:
```
			if (updatedCols == perminfo->updatedCols)
            			updatedCols = bms_copy(updatedCols);

			updatedCols =
				bms_add_member(updatedCols,
							   rangeAttno - FirstLowInvalidHeapAttributeNumber);
```

2 - execUtils.c
```
+  * because the user does not need UPDATE permission on it. Now manualy
```

Typo: manualy -> manually

3 - nodeModifyTable.c
```
+		/*
+		 * If we don't have a ForPortionOfState yet, we must be a partition
+		 * child being hit for the first time. Make a copy from the root, with
+		 * our own TupleTableSlot. We do this lazily so that we don't pay the
+		 * price of unused partitions.
+		 */
+		if ((((ModifyTable *) context.mtstate->ps.plan)->forPortionOf) &&
+			!resultRelInfo->ri_forPortionOf)
+		{
+			ExecInitForPortionOf(context.mtstate, estate, resultRelInfo);
+		}
```

I think this comment is stale. It could be a partition child or an inheritance child.

4 - nodeModifyTable.c
```
+	/* Each partition needs a slot matching its tuple descriptor */
+	leafState->fp_Existing =
+		table_slot_create(resultRelInfo->ri_RelationDesc,
+						  &mtstate->ps.state->es_tupleTable);
```

I think the comment should say "each child relation" rather than "each partition".

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  19. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-07T23:26:08Z

    On Wed, May 6, 2026 at 10:55 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > I didn’t review this patch earlier because, from the subject, I thought it was only about recomputing generated stored columns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for the inheritance-table bug. Please see [1].
> >
> > I tried applying the new tests from my patch on top of this patch, and it looks like this patch still does not fix the multi-inheritance case.
> >
> > So I’d like to check with you how we should proceed. I think there are two options:
> >
> > 1. Keep this patch focused on the generated-column issue described in the subject, and use my patch to fix the inheritance-table bug.
> > 2. I can continue from this patch and extend it to fix the multi-inheritance case as well.
> >
> > Please let me know what you prefer.

Thanks for your help on this! I agree that separating the patches
would be better.

> I just looked into v9 and made a fix in ExecInitForPortionOf() that resolves the bug with multi-inheritance tables. I also added a test case for that.
>
> The inheritance-table bug affects not only UPDATE, but also DELETE, so I added test cases for DELETE as well. Please see 0002 for my changes.
>
> To make each commit self-contained, would you mind moving the code for the inheritance-table fix to 0002? Then you can keep focusing on 0001, and I can continue working on 0002.
>
> PFA v10 - 0001 the same as v9. 0002 fixed a bug with multi-inheritance tables.

I'll post a v11 addressing the feedback in your other email and moving
the fixes for partitions/inheritance.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

  20. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-07T23:47:46Z

    On Thu, May 7, 2026 at 12:06 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > <v10-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch><v10-0002-Fix-FOR-PORTION-OF-on-inherited-children-with-di.patch>
>
> A few comments for 0001:
>
> 1 - execUtils.c
> The comment explicitly says that it is unsafe to mutate perminfo, but bms_add_member() does not always allocate a new bitmapset. So if updatedCols still points to perminfo->updatedCols, then bms_add_member() may mutate perminfo->updatedCols.
>
> To verify that, I added Assert(updatedCols != perminfo->updatedCols); right after the bms_add_member(), then ran "make check". A lot of tests failed, which seems to confirm that perminfo->updatedCols was being mutated.
>
> So, I think we should make a copy the bitmapset before bms_add_member when needed, to make sure perminfo is not mutated, something like:
> ```
>                         if (updatedCols == perminfo->updatedCols)
>                                 updatedCols = bms_copy(updatedCols);
>
>                         updatedCols =
>                                 bms_add_member(updatedCols,
>                                                            rangeAttno - FirstLowInvalidHeapAttributeNumber);
> ```

Ah, thanks for catching this! Fixed.

> 2 - execUtils.c
> ```
> +  * because the user does not need UPDATE permission on it. Now manualy
> ```
>
> Typo: manualy -> manually

Fixed.

> 3 - nodeModifyTable.c
> ```
> +               /*
> +                * If we don't have a ForPortionOfState yet, we must be a partition
> +                * child being hit for the first time. Make a copy from the root, with
> +                * our own TupleTableSlot. We do this lazily so that we don't pay the
> +                * price of unused partitions.
> +                */
> +               if ((((ModifyTable *) context.mtstate->ps.plan)->forPortionOf) &&
> +                       !resultRelInfo->ri_forPortionOf)
> +               {
> +                       ExecInitForPortionOf(context.mtstate, estate, resultRelInfo);
> +               }
> ```
>
> I think this comment is stale. It could be a partition child or an inheritance child.

Okay.

> 4 - nodeModifyTable.c
> ```
> +       /* Each partition needs a slot matching its tuple descriptor */
> +       leafState->fp_Existing =
> +               table_slot_create(resultRelInfo->ri_RelationDesc,
> +                                                 &mtstate->ps.state->es_tupleTable);
> ```
>
> I think the comment should say "each child relation" rather than "each partition".

Okay.

In these v11 patches I've tried to separate (1) the fix for GENERATED
STORED columns and UPDATE OF triggers (2) fixing inheritance and (and
partitions too, for the bugs in #1). I understand why jian he combined
these into one patch: there is some overlap. If you don't like my
separation, let me know.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  21. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Chao Li <li.evan.chao@gmail.com> — 2026-05-08T07:09:46Z

    
> On May 8, 2026, at 07:47, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
> 
> On Thu, May 7, 2026 at 12:06 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>> <v10-0001-Fix-some-problems-with-UPDATE-FOR-PORTION-OF.patch><v10-0002-Fix-FOR-PORTION-OF-on-inherited-children-with-di.patch>
>> 
>> A few comments for 0001:
>> 
>> 1 - execUtils.c
>> The comment explicitly says that it is unsafe to mutate perminfo, but bms_add_member() does not always allocate a new bitmapset. So if updatedCols still points to perminfo->updatedCols, then bms_add_member() may mutate perminfo->updatedCols.
>> 
>> To verify that, I added Assert(updatedCols != perminfo->updatedCols); right after the bms_add_member(), then ran "make check". A lot of tests failed, which seems to confirm that perminfo->updatedCols was being mutated.
>> 
>> So, I think we should make a copy the bitmapset before bms_add_member when needed, to make sure perminfo is not mutated, something like:
>> ```
>>                        if (updatedCols == perminfo->updatedCols)
>>                                updatedCols = bms_copy(updatedCols);
>> 
>>                        updatedCols =
>>                                bms_add_member(updatedCols,
>>                                                           rangeAttno - FirstLowInvalidHeapAttributeNumber);
>> ```
> 
> Ah, thanks for catching this! Fixed.
> 
>> 2 - execUtils.c
>> ```
>> +  * because the user does not need UPDATE permission on it. Now manualy
>> ```
>> 
>> Typo: manualy -> manually
> 
> Fixed.
> 
>> 3 - nodeModifyTable.c
>> ```
>> +               /*
>> +                * If we don't have a ForPortionOfState yet, we must be a partition
>> +                * child being hit for the first time. Make a copy from the root, with
>> +                * our own TupleTableSlot. We do this lazily so that we don't pay the
>> +                * price of unused partitions.
>> +                */
>> +               if ((((ModifyTable *) context.mtstate->ps.plan)->forPortionOf) &&
>> +                       !resultRelInfo->ri_forPortionOf)
>> +               {
>> +                       ExecInitForPortionOf(context.mtstate, estate, resultRelInfo);
>> +               }
>> ```
>> 
>> I think this comment is stale. It could be a partition child or an inheritance child.
> 
> Okay.
> 
>> 4 - nodeModifyTable.c
>> ```
>> +       /* Each partition needs a slot matching its tuple descriptor */
>> +       leafState->fp_Existing =
>> +               table_slot_create(resultRelInfo->ri_RelationDesc,
>> +                                                 &mtstate->ps.state->es_tupleTable);
>> ```
>> 
>> I think the comment should say "each child relation" rather than "each partition".
> 
> Okay.
> 
> In these v11 patches I've tried to separate (1) the fix for GENERATED
> STORED columns and UPDATE OF triggers (2) fixing inheritance and (and
> partitions too, for the bugs in #1). I understand why jian he combined
> these into one patch: there is some overlap. If you don't like my
> separation, let me know.
> 
> Yours,
> 
> -- 
> Paul              ~{:-)
> pj@illuminatedcomputing.com
> <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>

Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.

```
+	if (relinfo->ri_forPortionOf)
+	{
+		AttrNumber  rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
+
+		if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
+						   updatedCols))
+		{
+			MemoryContext oldContext;
+
+			oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+			updatedCols = bms_copy(updatedCols);
+			updatedCols =
+				bms_add_member(updatedCols,
+							   rangeAttno - FirstLowInvalidHeapAttributeNumber);
+
+			MemoryContextSwitchTo(oldContext);
+		}
 	}
```

1. I don’t think we should unconditionally do bms_copy, only if (updatedCols == perminfo->updatedCols), we need to make the copy.

2. I doubt if we need to switch to estate->es_query_cxt. Because ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its header comment says the function runs in per-tuple memory context:
```
/*
* Return columns being updated, including generated columns
*
* The bitmap is allocated in per-tuple memory context. It's up to the caller to
* copy it into a different context with the appropriate lifespan, if needed.
*/
Bitmapset *
ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
```

So I think bms_copy and bms_add_member should be just done in the current memory context.

3. "rangeAttno - FirstLowInvalidHeapAttributeNumber” appears twice, maybe add a local variable to avoid the duplication.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  22. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-08T15:25:22Z

    On Fri, May 8, 2026 at 12:10 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
>
> Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.
>
> ```
> +       if (relinfo->ri_forPortionOf)
> +       {
> +               AttrNumber  rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
> +
> +               if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
> +                                                  updatedCols))
> +               {
> +                       MemoryContext oldContext;
> +
> +                       oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
> +
> +                       updatedCols = bms_copy(updatedCols);
> +                       updatedCols =
> +                               bms_add_member(updatedCols,
> +                                                          rangeAttno - FirstLowInvalidHeapAttributeNumber);
> +
> +                       MemoryContextSwitchTo(oldContext);
> +               }
>         }
> ```
>
> 1. I don’t think we should unconditionally do bms_copy, only if (updatedCols == perminfo->updatedCols), we need to make the copy.

You're saying we can skip the copy if execute_attr_map_cols already
made a new bms above. That's true. Since we're going to just use the
current memory context (see below), that seems safe.

> 2. I doubt if we need to switch to estate->es_query_cxt. Because ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its header comment says the function runs in per-tuple memory context:
> ```
> /*
> * Return columns being updated, including generated columns
> *
> * The bitmap is allocated in per-tuple memory context. It's up to the caller to
> * copy it into a different context with the appropriate lifespan, if needed.
> */
> Bitmapset *
> ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
> ```
>
> So I think bms_copy and bms_add_member should be just done in the current memory context.

Okay. I think using the current memory context is more correct anyway.
There are other callers, and using the query memory context isn't
necessarily what they want. Also the bms (potentially) allocated by
execute_attr_map_cols is in the current memory context, so doing
something different feels surprising. And it's safer not to change the
behavior. Maybe there is a memory leak because of a long-lived
context, but then it exists already. I added a comment to
ExecGetUpdatedCols to call out that we use the current memory context.

> 3. "rangeAttno - FirstLowInvalidHeapAttributeNumber” appears twice, maybe add a local variable to avoid the duplication.

Okay.

v12 attached.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  23. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Chao Li <li.evan.chao@gmail.com> — 2026-05-09T06:38:03Z

    
> On May 8, 2026, at 23:25, Paul A Jungwirth <pj@illuminatedcomputing.com> wrote:
> 
> On Fri, May 8, 2026 at 12:10 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>> <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
>> 
>> Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.
>> 
>> ```
>> +       if (relinfo->ri_forPortionOf)
>> +       {
>> +               AttrNumber  rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
>> +
>> +               if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
>> +                                                  updatedCols))
>> +               {
>> +                       MemoryContext oldContext;
>> +
>> +                       oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
>> +
>> +                       updatedCols = bms_copy(updatedCols);
>> +                       updatedCols =
>> +                               bms_add_member(updatedCols,
>> +                                                          rangeAttno - FirstLowInvalidHeapAttributeNumber);
>> +
>> +                       MemoryContextSwitchTo(oldContext);
>> +               }
>>        }
>> ```
>> 
>> 1. I don’t think we should unconditionally do bms_copy, only if (updatedCols == perminfo->updatedCols), we need to make the copy.
> 
> You're saying we can skip the copy if execute_attr_map_cols already
> made a new bms above. That's true. Since we're going to just use the
> current memory context (see below), that seems safe.
> 
>> 2. I doubt if we need to switch to estate->es_query_cxt. Because ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its header comment says the function runs in per-tuple memory context:
>> ```
>> /*
>> * Return columns being updated, including generated columns
>> *
>> * The bitmap is allocated in per-tuple memory context. It's up to the caller to
>> * copy it into a different context with the appropriate lifespan, if needed.
>> */
>> Bitmapset *
>> ExecGetAllUpdatedCols(ResultRelInfo *relinfo, EState *estate)
>> ```
>> 
>> So I think bms_copy and bms_add_member should be just done in the current memory context.
> 
> Okay. I think using the current memory context is more correct anyway.
> There are other callers, and using the query memory context isn't
> necessarily what they want. Also the bms (potentially) allocated by
> execute_attr_map_cols is in the current memory context, so doing
> something different feels surprising. And it's safer not to change the
> behavior. Maybe there is a memory leak because of a long-lived
> context, but then it exists already. I added a comment to
> ExecGetUpdatedCols to call out that we use the current memory context.
> 
>> 3. "rangeAttno - FirstLowInvalidHeapAttributeNumber” appears twice, maybe add a local variable to avoid the duplication.
> 
> Okay.
> 
> v12 attached.
> 
> Yours,
> 
> -- 
> Paul              ~{:-)
> pj@illuminatedcomputing.com
> <v12-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v12-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>

Thanks for updating the patch. I have no more comment. V12 LGTM.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  24. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-05-11T12:03:17Z

    On Fri, May 8, 2026 at 11:25 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Fri, May 8, 2026 at 12:10 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > > <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
> >
> > Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.
> >
> > ```
> > +       if (relinfo->ri_forPortionOf)
> > +       {
> > +               AttrNumber  rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;
> > +
> > +               if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
> > +                                                  updatedCols))
> > +               {
> > +                       MemoryContext oldContext;
> > +
> > +                       oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
> > +
> > +                       updatedCols = bms_copy(updatedCols);
> > +                       updatedCols =
> > +                               bms_add_member(updatedCols,
> > +                                                          rangeAttno - FirstLowInvalidHeapAttributeNumber);
> > +
> > +                       MemoryContextSwitchTo(oldContext);
> > +               }
> >         }
> > ```
> >
> > 1. I don’t think we should unconditionally do bms_copy, only if (updatedCols == perminfo->updatedCols), we need to make the copy.
>
> You're saying we can skip the copy if execute_attr_map_cols already
> made a new bms above. That's true. Since we're going to just use the
> current memory context (see below), that seems safe.
>
> > 2. I doubt if we need to switch to estate->es_query_cxt. Because ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its header comment says the function runs in per-tuple memory context:
> > ```

Switching to estate->es_query_cxt can actually save some cycles.

See ExecGetExtraUpdatedCols->ExecInitGenerated
    /*
     * Make sure these data structures are built in the per-query memory
     * context so they'll survive throughout the query.
     */
    oldContext = MemoryContextSwitchTo(estate->es_query_cxt);


In ExecGetUpdatedCols, we can change it to the following to save some
unnecessary bms_add_member cycle.
``````
    if (relinfo->ri_forPortionOf)
    {
        AttrNumber    rangeAttno = relinfo->ri_forPortionOf->fp_rangeAttno;

        if (!bms_is_member(rangeAttno - FirstLowInvalidHeapAttributeNumber,
                           updatedCols))
        {
            MemoryContext oldContext;

            if (updatedCols != perminfo->updatedCols)
                updatedCols = bms_add_member(updatedCols, rangeAttno -
FirstLowInvalidHeapAttributeNumber);
            else
            {
                oldContext = MemoryContextSwitchTo(estate->es_query_cxt);

                updatedCols = bms_add_member(updatedCols, rangeAttno -
FirstLowInvalidHeapAttributeNumber);

               MemoryContextSwitchTo(oldContext);
            }
        }
    }
``````

  25. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-12T19:26:01Z

    On Mon, May 11, 2026 at 5:03 AM jian he <jian.universality@gmail.com> wrote:
>
> > > 2. I doubt if we need to switch to estate->es_query_cxt. Because ExecGetUpdatedCols() is called by ExecGetAllUpdatedCols(), and its header comment says the function runs in per-tuple memory context:
> > > ```
>
> Switching to estate->es_query_cxt can actually save some cycles.
>
> See ExecGetExtraUpdatedCols->ExecInitGenerated
>     /*
>      * Make sure these data structures are built in the per-query memory
>      * context so they'll survive throughout the query.
>      */
>     oldContext = MemoryContextSwitchTo(estate->es_query_cxt);

I agree that seems nice, but it doesn't seem correct if we sometimes
change the context and sometimes not (from execute_attr_map_cols).

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  26. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Nathan Bossart <nathandbossart@gmail.com> — 2026-05-12T20:34:18Z

    FOR PORTION OF doesn't seem to work well with virtual generated columns,
either.  The following example seg-faults on my machine:

    create table t (a int, b int4range generated always as (int4range(a, a + 1)) virtual);
    insert into t values (1);
    delete from t for portion of b from 1 to 2;

-- 
nathan

  27. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-13T00:05:03Z

    On Tue, May 12, 2026 at 1:34 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> FOR PORTION OF doesn't seem to work well with virtual generated columns,
> either.  The following example seg-faults on my machine:
>
>     create table t (a int, b int4range generated always as (int4range(a, a + 1)) virtual);
>     insert into t values (1);
>     delete from t for portion of b from 1 to 2;

Thanks for catching this!

Here is a patch forbidding both STORED and VIRTUAL columns here. There
is a follow-up patch (not for v19) to add SQL:2011 PERIODs, which will
be based on STORED columns, so we will eventually allow those (if they
belong to a PERIOD), but it seems right to forbid them for now.

I put the check in the analysis phase to match what we have already,
but based on [1] that is apparently premature. I think I'd like to
move all those things together in a single commit though.

I did experiment with putting just this check in ExecInitModifyTable.
But (1) the planner will already reject the UPDATE case with a
different error message, and (2) it doesn't really improve anything,
since rangeVar gets looked up during analysis anyway (until we address
the rest of [1]).

[1] https://www.postgresql.org/message-id/626986.1776785090%40sss.pgh.pa.us

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

  28. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-13T15:42:53Z

    On Tue, May 12, 2026 at 5:05 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Tue, May 12, 2026 at 1:34 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >
> > FOR PORTION OF doesn't seem to work well with virtual generated columns,
> > either.  The following example seg-faults on my machine:
> >
> >     create table t (a int, b int4range generated always as (int4range(a, a + 1)) virtual);
> >     insert into t values (1);
> >     delete from t for portion of b from 1 to 2;
>
> Thanks for catching this!
>
> Here is a patch forbidding both STORED and VIRTUAL columns here. There
> is a follow-up patch (not for v19) to add SQL:2011 PERIODs, which will
> be based on STORED columns, so we will eventually allow those (if they
> belong to a PERIOD), but it seems right to forbid them for now.
>
> I put the check in the analysis phase to match what we have already,
> but based on [1] that is apparently premature. I think I'd like to
> move all those things together in a single commit though.
>
> I did experiment with putting just this check in ExecInitModifyTable.
> But (1) the planner will already reject the UPDATE case with a
> different error message, and (2) it doesn't really improve anything,
> since rangeVar gets looked up during analysis anyway (until we address
> the rest of [1]).
>
> [1] https://www.postgresql.org/message-id/626986.1776785090%40sss.pgh.pa.us

I started a new thread for this issue and made a CF entry. Please see:

- https://www.postgresql.org/message-id/CA%2BrenyVRPyP5TNgEBe%3DhRT1ZR3%3DzWCZLXkAwp5xbWeS_TaMxOA%40mail.gmail.com
- https://commitfest.postgresql.org/patch/6764/

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  29. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-25T22:23:20Z

    On Fri, May 8, 2026 at 8:25 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Fri, May 8, 2026 at 12:10 AM Chao Li <li.evan.chao@gmail.com> wrote:
> > > <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
> >
> > Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.
> . . .
>
> v12 attached.

After discussing this as PGConf.dev, Peter and I agreed that we
*should* be checking for UPDATE permission on the application-time
column. So we need to add the attno to updatedCols on the
RTEPermissionInfo. That is great, because it fixes GENERATED column
dependency tracking and also UPDATE OF triggers, without the
complexity of changing ExecGetUpdatedCols.

Note we still do not require INSERT permission for the temporal
leftovers, which is what the SQL Standard says and also makes sense
semantically, since those leftovers represent already-existing
history.

No RLS changes are needed because RLS policies aren't checked based on
*column* changes.

We still need a fix for non-partitioning inheritance, but the patch
becomes a lot simpler.

Here is a patch adding the application-time column to updatedCols,
along with the tests we've written for GENERATED columns and UPDATE OF
triggers.

I'll submit a patch for inheritance on that thread.

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  30. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    jian he <jian.universality@gmail.com> — 2026-05-28T03:23:45Z

    On Tue, May 26, 2026 at 6:23 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Here is a patch adding the application-time column to updatedCols,
> along with the tests we've written for GENERATED columns and UPDATE OF
> triggers.
>

V13 looks good to me.

one minor issue:
V13 uses the wording "generated stored column(s)", but all other
places use "stored generated column(s)".

  31. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Peter Eisentraut <peter@eisentraut.org> — 2026-06-04T09:58:48Z

    On 26.05.26 00:23, Paul A Jungwirth wrote:
> On Fri, May 8, 2026 at 8:25 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
>>
>> On Fri, May 8, 2026 at 12:10 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>>> <v11-0001-Fix-FOR-PORTION-OF-column-dependency-tracking.patch><v11-0002-Fix-FOR-PORTION-OF-with-partitions-and-inheritan.patch>
>>>
>>> Thanks for updating the patch and making the separation. After reading v11, I still have a few comments for 0001.
>> . . .
>>
>> v12 attached.
> 
> After discussing this as PGConf.dev, Peter and I agreed that we
> *should* be checking for UPDATE permission on the application-time
> column. So we need to add the attno to updatedCols on the
> RTEPermissionInfo. That is great, because it fixes GENERATED column
> dependency tracking and also UPDATE OF triggers, without the
> complexity of changing ExecGetUpdatedCols.
> 
> Note we still do not require INSERT permission for the temporal
> leftovers, which is what the SQL Standard says and also makes sense
> semantically, since those leftovers represent already-existing
> history.
> 
> No RLS changes are needed because RLS policies aren't checked based on
> *column* changes.
> 
> We still need a fix for non-partitioning inheritance, but the patch
> becomes a lot simpler.
> 
> Here is a patch adding the application-time column to updatedCols,
> along with the tests we've written for GENERATED columns and UPDATE OF
> triggers.

committed

  32. Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

    Peter Eisentraut <peter@eisentraut.org> — 2026-06-04T10:04:25Z

    On 28.05.26 05:23, jian he wrote:
> On Tue, May 26, 2026 at 6:23 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
>>
>> Here is a patch adding the application-time column to updatedCols,
>> along with the tests we've written for GENERATED columns and UPDATE OF
>> triggers.
>>
> 
> V13 looks good to me.
> 
> one minor issue:
> V13 uses the wording "generated stored column(s)", but all other
> places use "stored generated column(s)".

I fixed that.  In one case the test actually included both kinds of 
generated columns, so I just removed the "stored".