From fbc1eed8a8dd3178de70de53a0e95786c80f9dbc Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 19 Aug 2021 12:12:35 -0400 Subject: [PATCH] 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 --- src/backend/parser/analyze.c | 19 +++++++++++++++++-- src/include/nodes/parsenodes.h | 8 ++++---- src/test/regress/expected/rules.out | 25 +++++++++++++++++++++++++ src/test/regress/sql/rules.sql | 14 ++++++++++++++ 4 files changed, 60 insertions(+), 6 deletions(-) diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index f491d93739..21d26cf7db 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -2758,13 +2758,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: @@ -2794,7 +2803,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); @@ -2815,6 +2828,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 50f7e228d5..e364f8f386 100644 --- 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 c2a591848d..3c814e2d33 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -2706,6 +2706,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 689b931935..92b66cdb5f 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -991,6 +991,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;