Re: BUG #17151: A SEGV in optimizer - Mailing list pgsql-bugs

I wrote:
> (This passes check-world, but I've not double-checked to make sure
> that inFromCl will be set in exactly the cases we want.)

After studying the code a bit more, I remembered why my hindbrain
was feeling uncomfortable about that coding: parsenodes.h says that
inFromCl is quasi-deprecated and not used anymore during parsing.

However, we can't really use the ParseNamespaceItem data structure
for this purpose, because baserels should be available to lock
whether or not they are visible according to join aliasing rules.
I don't see a lot of point to inventing some complicated add-on
for this when inFromCl will serve fine.  So I think we should just
adjust the relevant comments, say like the attached.

We probably need some regression test cases added (I wonder whether
FOR UPDATE in rule actions is covered at all ATM).  Otherwise
I feel like this is OK to commit.

            regards, tom lane

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 438b077004..15669c8238 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -3170,13 +3170,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:
@@ -3206,7 +3215,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);
@@ -3227,6 +3240,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 e28248af32..7af13dee43 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -929,10 +929,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*