Thread
-
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 -
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.
-
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 -
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". -
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/ -
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/ -
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/ -
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 -
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 -
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/ -
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 -
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/ -
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); } } } `````` -
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 -
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 -
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 -
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 -
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 -
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)".