| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Richard Guo <guofenglinux(at)gmail(dot)com> |
| Cc: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, SATYANARAYANA NARLAPURAM <satyanarlapuram(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org> |
| Subject: | Re: Bug: Rule actions see wrong values for generated columns (NEW.gen reads OLD value) |
| Date: | 2026-04-14 06:28:24 |
| Message-ID: | 8D1CD3EB-BF72-4C73-AF24-D88581AC01BE@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Apr 14, 2026, at 11:27, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Mon, Apr 13, 2026 at 8:04 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On Mon, 13 Apr 2026, 09:20 Richard Guo, <guofenglinux(at)gmail(dot)com> wrote:
>>> I think a simpler fix might be to expand generated column references
>>> in the NEW relation to their generation expressions before
>>> ReplaceVarsFromTargetList resolves NEW references, so that the base
>>> column Vars within the expressions can be correctly resolved.
>>> Something like attached.
>
>> One thing about that approach is that it leads to 2 full rewrites of the rule action using ReplaceVarsFromTargetList(). I think that could be avoided by using including generated column expressions in the targetlist passed to ReplaceVarsFromTargetList() by rewriteRuleAction(). I haven't tried it, but I imagine it could reuse some code from expand_generated_columns_internal().
>
> I considered it, but I'm afraid it doesn't work directly, because
> replace_rte_variables_mutator returns the callback's replacement node
> without recursing into its children.
>
> Take Satya's repro as an example. If we add the generation expression
> for gen to the UPDATE's targetlist, the list would be:
>
> TargetEntry 1: resno=2, expr=Const(100) -- a = 100
> TargetEntry 2: resno=3, expr=Var(3, 2) * 2 -- gen = NEW.a * 2
>
> When ReplaceVarsFromTargetList processes Var(3, 3) (NEW.gen) in the
> rule action, it finds resno=3 and substitutes Var(3, 2) * 2. However,
> replace_rte_variables_mutator returns this replacement directly to its
> caller; it does not recurse into the replacement's children to look
> for further matching Vars. So the inner Var(3, 2) (NEW.a) is never
> processed, even though resno=2 with Const(100) is right there in the
> targetlist. The Var(3, 2) survives into the planner and would cause
> problems.
>
> 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.
>
> - Richard
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”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Smith | 2026-04-14 06:28:37 | Re: Add missing period to DETAIL messages |
| Previous Message | Yugo Nagata | 2026-04-14 06:24:25 | Re: Infinite Autovacuum loop caused by failing virtual generated column expression |