From 7fa367d96bb1ae25f422d6c2a78424f0f9227b5a 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 d09e8ccde4..8a47b854d4 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -2753,13 +2753,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: @@ -2789,7 +2798,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); @@ -2810,6 +2823,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 557074c2cf..b907e97207 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -915,10 +915,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 afac6fa243..8f27135a04 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -2887,6 +2887,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 6ec37c4381..b732833e63 100644 --- 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;