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

On Fri, Apr 17, 2026 at 1:14 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Thu, Apr 16, 2026 at 4:59 AM Paul A Jungwirth
> <pj(at)illuminatedcomputing(dot)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(at)illuminatedcomputing(dot)com