Avoid trying to lock OLD/NEW in a rule with FOR UPDATE.

transformLockingClause neglected to exclude the pseudo-RTEs for
OLD/NEW when processing a rule's query.  This led to odd errors
or even crashes later on.  This bug is very ancient, but it's
not terribly surprising that nobody noticed, since the use-case
for SELECT FOR UPDATE in a non-view rule is somewhere between
thin and non-existent.  Still, crashing is not OK.

Per bug #17151 from Zhiyong Wu.  Thanks to Masahiko Sawada
for analysis of the problem.

Discussion: https://postgr.es/m/17151-c03a3e6e4ec9aadb@postgresql.org

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 90c23766cdb76135c5ea31ad801b6c544bc7e9a3..32b3a0a8e579d467ec381ab6225700fb9e08f9b4 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -2744,13 +2744,22 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
 
    if (lockedRels == NIL)
    {
-       /* all regular tables used in query */
+       /*
+        * Lock all regular tables used in query and its subqueries.  We
+        * examine inFromCl to exclude auto-added RTEs, particularly NEW/OLD
+        * in rules.  This is a bit of an abuse of a mostly-obsolete flag, but
+        * it's convenient.  We can't rely on the namespace mechanism that has
+        * largely replaced inFromCl, since for example we need to lock
+        * base-relation RTEs even if they are masked by upper joins.
+        */
        i = 0;
        foreach(rt, qry->rtable)
        {
            RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);
 
            ++i;
+           if (!rte->inFromCl)
+               continue;
            switch (rte->rtekind)
            {
                case RTE_RELATION:
@@ -2780,7 +2789,11 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
    }
    else
    {
-       /* just the named tables */
+       /*
+        * Lock just the named tables.  As above, we allow locking any base
+        * relation regardless of alias-visibility rules, so we need to
+        * examine inFromCl to exclude OLD/NEW.
+        */
        foreach(l, lockedRels)
        {
            RangeVar   *thisrel = (RangeVar *) lfirst(l);
@@ -2801,6 +2814,8 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
                RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);
 
                ++i;
+               if (!rte->inFromCl)
+                   continue;
                if (strcmp(rte->eref->aliasname, thisrel->relname) == 0)
                {
                    switch (rte->rtekind)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 96ad25208c08f4d4d570250cfee1c99af34ba11e..f93aff96ac75b4a8be04a6f5a9e52fc06cf21665 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -913,10 +913,10 @@ typedef struct PartitionCmd
  *   inFromCl marks those range variables that are listed in the FROM clause.
  *   It's false for RTEs that are added to a query behind the scenes, such
  *   as the NEW and OLD variables for a rule, or the subqueries of a UNION.
- *   This flag is not used anymore during parsing, since the parser now uses
- *   a separate "namespace" data structure to control visibility, but it is
- *   needed by ruleutils.c to determine whether RTEs should be shown in
- *   decompiled queries.
+ *   This flag is not used during parsing (except in transformLockingClause,
+ *   q.v.); the parser now uses a separate "namespace" data structure to
+ *   control visibility.  But it is needed by ruleutils.c to determine
+ *   whether RTEs should be shown in decompiled queries.
  *
  *   requiredPerms and checkAsUser specify run-time access permissions
  *   checks to be performed at query startup.  The user must have *all*

diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 030d253ed324c4b101a4b15f628ffba14f9d69c0..18336ec3573e6372e9acb9720882a93ba1cf8788 100644 (file)
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2822,6 +2822,31 @@ select * from only t1_2;
 (10 rows)
 
 reset constraint_exclusion;
+-- test FOR UPDATE in rules
+create table rules_base(f1 int, f2 int);
+insert into rules_base values(1,2), (11,12);
+create rule r1 as on update to rules_base do instead
+  select * from rules_base where f1 = 1 for update;
+update rules_base set f2 = f2 + 1;
+ f1 | f2 
+----+----
+  1 |  2
+(1 row)
+
+create or replace rule r1 as on update to rules_base do instead
+  select * from rules_base where f1 = 11 for update of rules_base;
+update rules_base set f2 = f2 + 1;
+ f1 | f2 
+----+----
+ 11 | 12
+(1 row)
+
+create or replace rule r1 as on update to rules_base do instead
+  select * from rules_base where f1 = 11 for update of old; -- error
+ERROR:  relation "old" in FOR UPDATE clause not found in FROM clause
+LINE 2:   select * from rules_base where f1 = 11 for update of old;
+                                                               ^
+drop table rules_base;
 -- test various flavors of pg_get_viewdef()
 select pg_get_viewdef('shoe'::regclass) as unpretty;
                     unpretty                    

diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index f641e303c09ccd040380fe41126dd6e0f23899d7..5e8c9b6421e1212b235cefe8e09ac981b9ae2eb4 100644 (file)
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -992,6 +992,20 @@ select * from only t1_2;
 
 reset constraint_exclusion;
 
+-- test FOR UPDATE in rules
+
+create table rules_base(f1 int, f2 int);
+insert into rules_base values(1,2), (11,12);
+create rule r1 as on update to rules_base do instead
+  select * from rules_base where f1 = 1 for update;
+update rules_base set f2 = f2 + 1;
+create or replace rule r1 as on update to rules_base do instead
+  select * from rules_base where f1 = 11 for update of rules_base;
+update rules_base set f2 = f2 + 1;
+create or replace rule r1 as on update to rules_base do instead
+  select * from rules_base where f1 = 11 for update of old; -- error
+drop table rules_base;
+
 -- test various flavors of pg_get_viewdef()
 
 select pg_get_viewdef('shoe'::regclass) as unpretty;