Re: Bug: Rule actions see wrong values for generated columns (NEW.gen reads OLD value)

On Tue, 14 Apr 2026 at 07:29, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
> > On Apr 14, 2026, at 11:27, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> >
> > It could be made to work by pre-resolving the generation expressions'
> > base column Vars before adding them to the UPDATE's targetlist. For
> > each generated column, we'd call ReplaceVarsFromTargetList on the
> > generation expression to resolve its base column Vars, then add the
> > fully resolved expression to the targetlist. But this seems to add
> > code complexity. And I'm not sure about the performance difference
> > between these two approaches. I expect that rule action trees are
> > typically small.
> >
> My implementation has pre-resolved the generation expressions, that’s why all tests passed. But I agree my change is heavier as I had to add a new static helper function.
>
> If we think rule actions are usually small enough that the extra full-tree pass would not be an issue, then v1 may be preferable for simplicity.
>
> My only comment on v1 is the typo in generated_virtual.sql where “STORED” should be “VIRTUAL”.
>

I don't quite buy the argument that the rule action is typically
small. We have no idea how big it might be. Note that, at that point
in the code, sub_action is the combination of both the original query
and the rule action.

I do accept though that rules are not widely used, and that it's not
worth optimising too much, if it means a lot of extra complexity.
However, IMO, it is slightly simpler and neater to put the expanded
generated columns in the replacement list used by
ReplaceVarsFromTargetList() on sub_action.

In the attached v2 patch, I've done that by refactoring
expand_generated_columns_internal(), renaming it to
get_generated_columns(), and making it just return the list of
generated column expressions, rather than doing the rewrite -- I never
particularly liked the separation of concerns between
expand_generated_columns_internal() and
expand_generated_columns_in_expr(), especially after the rest of the
code expanding virtual generated columns was moved out of the
rewriter, so that expand_generated_columns_in_expr() became the only
caller of expand_generated_columns_internal(). Doing this simplifies
the function, since it's no longer necessary to pass it node, rte, and
result_relation.

With that change, all rewriteRuleAction() needs to do is get the
generated columns, rewrite any new.attribute references in them, and
then use that list plus the original target list as the replacement
list when rewriting sub_action.

It is a slightly bigger patch overall, but it feels a little neater
and more logical to me, but I accept that that's a subjective thing.

Note: I included a minor comment update needed for
build_generation_expression(), and the test fix noted above.

Regards,
Dean