Обсуждение: DELETE/UPDATE FOR PORTION OF with rule system is not working

Hi.

https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f
DELETE/UPDATE FOR PORTION OF with rule system is not working, and
there are no RULE related regession tests.

in gram.y, RuleStmt, RuleActionList, RuleActionStmt
RuleActionStmt:
            SelectStmt
            | InsertStmt
            | UpdateStmt
            | DeleteStmt
            | NotifyStmt
        ;

So far, I found 2 errors, and one crash.
There might be more bugs, I didn't try all the cases.

drop table if exists fpo_rule;
create table fpo_rule (f1 bigint, f2 int4range);
INSERT INTO fpo_rule values (1, '[1, 10]');
CREATE RULE rule3 AS ON INSERT TO fpo_rule DO INSTEAD UPDATE fpo_rule
FOR PORTION OF f2 FROM 1 to 4 SET F1 = 2;

INSERT INTO fpo_rule values (2, '[2, 12]');
ERROR:  range types do not match
\errverbose
ERROR:  XX000: range types do not match
LOCATION:  range_minus_multi, rangetypes.c:1260

CREATE RULE rule4 AS ON DELETE TO fpo_rule DO INSTEAD UPDATE fpo_rule
FOR PORTION OF f2 FROM 1 to 4 SET F1 = 2;
DELETE FROM fpo_rule;
ERROR:  no relation entry for relid 3
\errverbose
ERROR:  XX000: no relation entry for relid 3
LOCATION:  find_base_rel, relnode.c:556

DROP RULE rule4 ON fpo_rule;
CREATE RULE rule5 AS ON UPDATE TO fpo_rule DO INSTEAD DELETE FROM
fpo_rule FOR PORTION OF f2 FROM 1 to 4;
UPDATE fpo_rule FOR PORTION OF f2 FROM 1 to 4 SET F1 = 2; -- server crash

As of now, we should try to ban CREATE ROLE with UPDATE/DELETE FOR PORTION OF.



--
jian
https://www.enterprisedb.com/

On Mon, 13 Apr 2026 at 06:45, jian he <jian.universality@gmail.com> wrote:
>
> Hi.
>
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f
> DELETE/UPDATE FOR PORTION OF with rule system is not working, and
> there are no RULE related regession tests.

Thanks! +cc Paul & Peter as authors of [0]

> in gram.y, RuleStmt, RuleActionList, RuleActionStmt
> RuleActionStmt:
>             SelectStmt
>             | InsertStmt
>             | UpdateStmt
>             | DeleteStmt
>             | NotifyStmt
>         ;
>
> So far, I found 2 errors, and one crash.
> There might be more bugs, I didn't try all the cases.
>
> drop table if exists fpo_rule;
> create table fpo_rule (f1 bigint, f2 int4range);
> INSERT INTO fpo_rule values (1, '[1, 10]');
> CREATE RULE rule3 AS ON INSERT TO fpo_rule DO INSTEAD UPDATE fpo_rule
> FOR PORTION OF f2 FROM 1 to 4 SET F1 = 2;
>
> INSERT INTO fpo_rule values (2, '[2, 12]');
> ERROR:  range types do not match
> \errverbose
> ERROR:  XX000: range types do not match
> LOCATION:  range_minus_multi, rangetypes.c:1260
>
> CREATE RULE rule4 AS ON DELETE TO fpo_rule DO INSTEAD UPDATE fpo_rule
> FOR PORTION OF f2 FROM 1 to 4 SET F1 = 2;
> DELETE FROM fpo_rule;
> ERROR:  no relation entry for relid 3
> \errverbose
> ERROR:  XX000: no relation entry for relid 3
> LOCATION:  find_base_rel, relnode.c:556
>
> DROP RULE rule4 ON fpo_rule;
> CREATE RULE rule5 AS ON UPDATE TO fpo_rule DO INSTEAD DELETE FROM
> fpo_rule FOR PORTION OF f2 FROM 1 to 4;
> UPDATE fpo_rule FOR PORTION OF f2 FROM 1 to 4 SET F1 = 2; -- server crash
>
> As of now, we should try to ban CREATE ROLE with UPDATE/DELETE FOR PORTION OF.

+1 for banning


[0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f

-- 
Best regards,
Kirill Reshke

hi.
Actually it's supported.

The issue mentioned in the first email is caused by:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f

https://git.postgresql.org/cgit/postgresql.git/diff/src/backend/nodes/nodeFuncs.c?id=8e72d914c52876525a90b28444453de8085c866f

--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2579,6 +2579,20 @@ expression_tree_walker_impl(Node *node,
return true;
}
break;
+ case T_ForPortionOfExpr:
+ {
+ ForPortionOfExpr *forPortionOf = (ForPortionOfExpr *) node;
+
+ if (WALK(forPortionOf->targetFrom))
+ return true;
+ if (WALK(forPortionOf->targetTo))
+ return true;
+ if (WALK(forPortionOf->targetRange))
+ return true;
+ if (WALK(forPortionOf->overlapsExpr))
+ return true;
+ }
+ break;

We forgot to WALK (expression_tree_walker_impl)
ForPortionOfExpr->rangeVar and ForPortionOfExpr->rangeTargetList.
We need to WALK those two fields of ForPortionOfExpr in
rewriteRuleAction (ChangeVarNodes,
ReplaceVarsFromTargetList, etc.), and maybe elsewhere.

i am surprised that nothing else has broken because of this.



--
jian
https://www.enterprisedb.com/

On Mon, 13 Apr 2026 at 14:01, jian he <jian.universality@gmail.com> wrote:
>
> hi.
> Actually it's supported.
>
> The issue mentioned in the first email is caused by:
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=8e72d914c52876525a90b28444453de8085c866f
>
https://git.postgresql.org/cgit/postgresql.git/diff/src/backend/nodes/nodeFuncs.c?id=8e72d914c52876525a90b28444453de8085c866f
>
> --- a/src/backend/nodes/nodeFuncs.c
> +++ b/src/backend/nodes/nodeFuncs.c
> @@ -2579,6 +2579,20 @@ expression_tree_walker_impl(Node *node,
> return true;
> }
> break;
> + case T_ForPortionOfExpr:
> + {
> + ForPortionOfExpr *forPortionOf = (ForPortionOfExpr *) node;
> +
> + if (WALK(forPortionOf->targetFrom))
> + return true;
> + if (WALK(forPortionOf->targetTo))
> + return true;
> + if (WALK(forPortionOf->targetRange))
> + return true;
> + if (WALK(forPortionOf->overlapsExpr))
> + return true;
> + }
> + break;
>
> We forgot to WALK (expression_tree_walker_impl)
> ForPortionOfExpr->rangeVar and ForPortionOfExpr->rangeTargetList.
> We need to WALK those two fields of ForPortionOfExpr in
> rewriteRuleAction (ChangeVarNodes,
> ReplaceVarsFromTargetList, etc.), and maybe elsewhere.
>
> i am surprised that nothing else has broken because of this.
>
>
>
> --
> jian
> https://www.enterprisedb.com/


This fix looks valid for me.

Also, are all 4 new test cases really needed? If yes, why are we
missing  ON DELETE TO ... DO INSTEAD INSERT ?


-- 
Best regards,
Kirill Reshke

On Mon, 13 Apr 2026 at 20:10, Kirill Reshke <reshkekirill@gmail.com> wrote:
> If yes, why are we
> missing  ON DELETE TO ... DO INSTEAD INSERT ?

Oh sorry... i mean  ON UPDATE TO DO INSTEAD UPDATE FROM FOR PORTION OF


-- 
Best regards,
Kirill Reshke

On Mon, Apr 13, 2026 at 2:01 AM jian he <jian.universality@gmail.com> wrote:
>
> hi.
> Actually it's supported.

I also don't see any reason not to support this.

> We forgot to WALK (expression_tree_walker_impl)
> ForPortionOfExpr->rangeVar and ForPortionOfExpr->rangeTargetList.
> We need to WALK those two fields of ForPortionOfExpr in
> rewriteRuleAction (ChangeVarNodes,
> ReplaceVarsFromTargetList, etc.), and maybe elsewhere.
>
> i am surprised that nothing else has broken because of this.

Thank you for investigating and sending a fix.

The patch looks fine to me. I like the diversity of tests; I don't
think we need any more.

+-- UPDATE/DELETE FOR PORTION OF with RULEs
+CREATE TABLE fpo_rule (f1 bigint, f2 int4range);
+INSERT INTO fpo_rule VALUES (1, '[1, 10]');
+
+CREATE RULE fpo_rule1 AS ON INSERT TO fpo_rule DO INSTEAD UPDATE
fpo_rule FOR PORTION OF f2 FROM 1 TO 4 SET f1 = 2;
+INSERT INTO fpo_rule VALUES (1, '[1, 10]');
+SELECT * FROM fpo_rule ORDER BY f1;

I only have two small suggestions:

Please use '[1, 11)' syntax to match the other tests.

Breaking these long lines would be nice. For example:

+CREATE RULE fpo_rule1 AS ON INSERT TO fpo_rule
+  DO INSTEAD UPDATE fpo_rule FOR PORTION OF f2 FROM 1 TO 4 SET f1 = 2;

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On Thu, Apr 16, 2026 at 6:40 AM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> I only have two small suggestions:
>
> Please use '[1, 11)' syntax to match the other tests.
>
> Breaking these long lines would be nice. For example:
>
> +CREATE RULE fpo_rule1 AS ON INSERT TO fpo_rule
> +  DO INSTEAD UPDATE fpo_rule FOR PORTION OF f2 FROM 1 TO 4 SET f1 = 2;
>
Please check the attached v2.

V1 only has DO INSTEAD rules, adding one DO ALSO rule would make the
test coverage more robust.



--
jian
https://www.enterprisedb.com/

On Thu, Apr 16, 2026 at 8:20 PM jian he <jian.universality@gmail.com> wrote:
>
> On Thu, Apr 16, 2026 at 6:40 AM Paul A Jungwirth
> <pj@illuminatedcomputing.com> wrote:
> >
> > I only have two small suggestions:
> >
> > Please use '[1, 11)' syntax to match the other tests.
> >
> > Breaking these long lines would be nice. For example:
> >
> > +CREATE RULE fpo_rule1 AS ON INSERT TO fpo_rule
> > +  DO INSTEAD UPDATE fpo_rule FOR PORTION OF f2 FROM 1 TO 4 SET f1 = 2;
> >
> Please check the attached v2.
>
> V1 only has DO INSTEAD rules, adding one DO ALSO rule would make the
> test coverage more robust.

Thanks for those changes. This looks great to me!

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On 19.04.26 20:07, Paul A Jungwirth wrote:
> On Thu, Apr 16, 2026 at 8:20 PM jian he <jian.universality@gmail.com> wrote:
>>
>> On Thu, Apr 16, 2026 at 6:40 AM Paul A Jungwirth
>> <pj@illuminatedcomputing.com> wrote:
>>>
>>> I only have two small suggestions:
>>>
>>> Please use '[1, 11)' syntax to match the other tests.
>>>
>>> Breaking these long lines would be nice. For example:
>>>
>>> +CREATE RULE fpo_rule1 AS ON INSERT TO fpo_rule
>>> +  DO INSTEAD UPDATE fpo_rule FOR PORTION OF f2 FROM 1 TO 4 SET f1 = 2;
>>>
>> Please check the attached v2.
>>
>> V1 only has DO INSTEAD rules, adding one DO ALSO rule would make the
>> test coverage more robust.
> 
> Thanks for those changes. This looks great to me!

committed