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

On Thu, May 7, 2026 at 12:06 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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(at)illuminatedcomputing(dot)com