Обсуждение: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
SATYANARAYANA NARLAPURAM
Дата:
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.
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
Вложения
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/
Вложения
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
SATYANARAYANA NARLAPURAM
Дата:
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.
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.
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.
Thanks,
Satya
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/
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
Вложения
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/
Вложения
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
Вложения
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/
Вложения
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
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.
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
Вложения
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
Вложения
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Peter Eisentraut
Дата:
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.
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
Вложения
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".
> 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-tablebug. 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-inheritancecase.
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-tablebug.
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/
> 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
storedcolumns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for
theinheritance-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
themulti-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-tablebug.
> 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
alsoadded 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
see0002 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
keepfocusing 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/
Вложения
> 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
storedcolumns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for
theinheritance-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
themulti-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-tablebug.
>> 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
alsoadded 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
see0002 for my changes.
>
> To make each commit self-contained, would you mind moving the code for the inheritance-table fix to 0002? Then you
cankeep 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/
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
storedcolumns. I just noticed that the patch also changes the inheritance-table path, and I posted another patch for
theinheritance-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
themulti-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-tablebug.
> > 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
alsoadded 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
see0002 for my changes.
>
> To make each commit self-contained, would you mind moving the code for the inheritance-table fix to 0002? Then you
cankeep 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
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
"makecheck". 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
Вложения
> 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
newbitmapset. 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
"makecheck". 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
thecopy.
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/
Re: FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column
От
Paul A Jungwirth
Дата:
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
makethe 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