Improve representation of PlanRowMark.

This patch fixes two inadequacies of the PlanRowMark representation.

First, that the original LockingClauseStrength isn't stored (and cannot be
inferred for foreign tables, which always get ROW_MARK_COPY).  Since some
PlanRowMarks are created out of whole cloth and don't actually have an
ancestral RowMarkClause, this requires adding a dummy LCS_NONE value to
enum LockingClauseStrength, which is fairly annoying but the alternatives
seem worse.  This fix allows getting rid of the use of get_parse_rowmark()
in FDWs (as per the discussion around commits 462bd95705a0c23b and
8ec8760fc87ecde0), and it simplifies some things elsewhere.

Second, that the representation assumed that all child tables in an
inheritance hierarchy would use the same RowMarkType.  That's true today
but will soon not be true.  We add an "allMarkTypes" field that identifies
the union of mark types used in all a parent table's children, and use
that where appropriate (currently, only in preprocess_targetlist()).

In passing fix a couple of minor infelicities left over from the SKIP
LOCKED patch, notably that _outPlanRowMark still thought waitPolicy
is a bool.

Catversion bump is required because the numeric values of enum
LockingClauseStrength can appear in on-disk rules.

Extracted from a much larger patch to support foreign table inheritance;
it seemed worth breaking this out, since it's a separable concern.

Shigeru Hanada and Etsuro Fujita, somewhat modified by me

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 63f057704e65db38117469c6c3e42de01592bed4..478e12484b940ad9474b3604bc1615883018cde4 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -822,13 +822,14 @@ postgresGetForeignPlan(PlannerInfo *root,
    }
    else
    {
-       RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid);
+       PlanRowMark *rc = get_plan_rowmark(root->rowMarks, baserel->relid);
 
        if (rc)
        {
            /*
             * Relation is specified as a FOR UPDATE/SHARE target, so handle
-            * that.
+            * that.  (But we could also see LCS_NONE, meaning this isn't a
+            * target relation after all.)
             *
             * For now, just ignore any [NO] KEY specification, since (a) it's
             * not clear what that means for a remote table that we don't have
@@ -837,6 +838,9 @@ postgresGetForeignPlan(PlannerInfo *root,
             */
            switch (rc->strength)
            {
+               case LCS_NONE:
+                   /* No locking needed */
+                   break;
                case LCS_FORKEYSHARE:
                case LCS_FORSHARE:
                    appendStringInfoString(&sql, " FOR SHARE");

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 291e6a7c39113fbad8ab4a6cce862d6eb8fa300f..3c6a964a6599b585f969df4eba716b27af7787b3 100644 (file)
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -991,6 +991,8 @@ _copyPlanRowMark(const PlanRowMark *from)
    COPY_SCALAR_FIELD(prti);
    COPY_SCALAR_FIELD(rowmarkId);
    COPY_SCALAR_FIELD(markType);
+   COPY_SCALAR_FIELD(allMarkTypes);
+   COPY_SCALAR_FIELD(strength);
    COPY_SCALAR_FIELD(waitPolicy);
    COPY_SCALAR_FIELD(isParent);
 
@@ -2510,7 +2512,7 @@ _copyXmlSerialize(const XmlSerialize *from)
 static RoleSpec *
 _copyRoleSpec(const RoleSpec *from)
 {
-   RoleSpec *newnode = makeNode(RoleSpec);
+   RoleSpec   *newnode = makeNode(RoleSpec);
 
    COPY_SCALAR_FIELD(roletype);
    COPY_STRING_FIELD(rolename);

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index fc418fcf9de7882437e1e7d0cb62a9a068ef1ac7..385b289bed2ff17e4ec4382aca632be16ffad384 100644 (file)
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -852,7 +852,9 @@ _outPlanRowMark(StringInfo str, const PlanRowMark *node)
    WRITE_UINT_FIELD(prti);
    WRITE_UINT_FIELD(rowmarkId);
    WRITE_ENUM_FIELD(markType, RowMarkType);
-   WRITE_BOOL_FIELD(waitPolicy);
+   WRITE_INT_FIELD(allMarkTypes);
+   WRITE_ENUM_FIELD(strength, LockClauseStrength);
+   WRITE_ENUM_FIELD(waitPolicy, LockWaitPolicy);
    WRITE_BOOL_FIELD(isParent);
 }
 

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 88b91f1b205a2f9ec4cc26a3dbf7e8f5c3a39582..05687a48c9a4a721e2885e499ce59a5bc643dcf8 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -2219,35 +2219,42 @@ preprocess_rowmarks(PlannerInfo *root)
        if (rte->rtekind != RTE_RELATION)
            continue;
 
-       /*
-        * Similarly, ignore RowMarkClauses for foreign tables; foreign tables
-        * will instead get ROW_MARK_COPY items in the next loop.  (FDWs might
-        * choose to do something special while fetching their rows, but that
-        * is of no concern here.)
-        */
-       if (rte->relkind == RELKIND_FOREIGN_TABLE)
-           continue;
-
        rels = bms_del_member(rels, rc->rti);
 
        newrc = makeNode(PlanRowMark);
        newrc->rti = newrc->prti = rc->rti;
        newrc->rowmarkId = ++(root->glob->lastRowMarkId);
-       switch (rc->strength)
+       if (rte->relkind == RELKIND_FOREIGN_TABLE)
        {
-           case LCS_FORUPDATE:
-               newrc->markType = ROW_MARK_EXCLUSIVE;
-               break;
-           case LCS_FORNOKEYUPDATE:
-               newrc->markType = ROW_MARK_NOKEYEXCLUSIVE;
-               break;
-           case LCS_FORSHARE:
-               newrc->markType = ROW_MARK_SHARE;
-               break;
-           case LCS_FORKEYSHARE:
-               newrc->markType = ROW_MARK_KEYSHARE;
-               break;
+           /* For now, we force all foreign tables to use ROW_MARK_COPY */
+           newrc->markType = ROW_MARK_COPY;
+       }
+       else
+       {
+           /* regular table, apply the appropriate lock type */
+           switch (rc->strength)
+           {
+               case LCS_NONE:
+                   /* we intentionally throw an error for LCS_NONE */
+                   elog(ERROR, "unrecognized LockClauseStrength %d",
+                        (int) rc->strength);
+                   break;
+               case LCS_FORKEYSHARE:
+                   newrc->markType = ROW_MARK_KEYSHARE;
+                   break;
+               case LCS_FORSHARE:
+                   newrc->markType = ROW_MARK_SHARE;
+                   break;
+               case LCS_FORNOKEYUPDATE:
+                   newrc->markType = ROW_MARK_NOKEYEXCLUSIVE;
+                   break;
+               case LCS_FORUPDATE:
+                   newrc->markType = ROW_MARK_EXCLUSIVE;
+                   break;
+           }
        }
+       newrc->allMarkTypes = (1 << newrc->markType);
+       newrc->strength = rc->strength;
        newrc->waitPolicy = rc->waitPolicy;
        newrc->isParent = false;
 
@@ -2276,6 +2283,8 @@ preprocess_rowmarks(PlannerInfo *root)
            newrc->markType = ROW_MARK_REFERENCE;
        else
            newrc->markType = ROW_MARK_COPY;
+       newrc->allMarkTypes = (1 << newrc->markType);
+       newrc->strength = LCS_NONE;
        newrc->waitPolicy = LockWaitBlock;      /* doesn't matter */
        newrc->isParent = false;
 

diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
index b382f13504e944351e9c5ccb5b8bd6da5cdab147..0be44c1c2f7ccca8f8d1e546964939d3591999c4 100644 (file)
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -73,8 +73,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
    rt_index = 0;
    foreach(cell, parse->rtable)
    {
-       bool            targetRelation = false;
-       RangeTblEntry  *rte = (RangeTblEntry *) lfirst(cell);
+       bool        targetRelation = false;
+       RangeTblEntry *rte = (RangeTblEntry *) lfirst(cell);
 
        rt_index++;
 
@@ -241,30 +241,10 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
            rc = get_plan_rowmark(root->rowMarks, rt_index);
            if (rc != NULL)
            {
-               switch (rc->markType)
-               {
-                   case ROW_MARK_EXCLUSIVE:
-                       applyLockingClause(subquery, 1, LCS_FORUPDATE,
-                                          rc->waitPolicy, false);
-                       break;
-                   case ROW_MARK_NOKEYEXCLUSIVE:
-                       applyLockingClause(subquery, 1, LCS_FORNOKEYUPDATE,
-                                          rc->waitPolicy, false);
-                       break;
-                   case ROW_MARK_SHARE:
-                       applyLockingClause(subquery, 1, LCS_FORSHARE,
-                                          rc->waitPolicy, false);
-                       break;
-                   case ROW_MARK_KEYSHARE:
-                       applyLockingClause(subquery, 1, LCS_FORKEYSHARE,
-                                          rc->waitPolicy, false);
-                       break;
-                   case ROW_MARK_REFERENCE:
-                   case ROW_MARK_COPY:
-                       /* No locking needed */
-                       break;
-               }
-               root->rowMarks = list_delete(root->rowMarks, rc);
+               if (rc->strength != LCS_NONE)
+                   applyLockingClause(subquery, 1, rc->strength,
+                                      rc->waitPolicy, false);
+               root->rowMarks = list_delete_ptr(root->rowMarks, rc);
            }
 
            /*
@@ -276,6 +256,7 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
            if (targetRelation)
                applyLockingClause(subquery, 1, LCS_FORUPDATE,
                                   LockWaitBlock, false);
+
            /*
             * Replace any variables in the outer query that refer to the
             * original relation RTE with references to columns that we will

diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
index 8a6c3cc5e5c644de516a32a68e9b81898d2a65a8..08e7c446d880c0c082436a71013a6569b84a2643 100644 (file)
--- a/src/backend/optimizer/prep/preptlist.c
+++ b/src/backend/optimizer/prep/preptlist.c
@@ -92,9 +92,9 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
        if (rc->rti != rc->prti)
            continue;
 
-       if (rc->markType != ROW_MARK_COPY)
+       if (rc->allMarkTypes & ~(1 << ROW_MARK_COPY))
        {
-           /* It's a regular table, so fetch its TID */
+           /* Need to fetch TID */
            var = makeVar(rc->rti,
                          SelfItemPointerAttributeNumber,
                          TIDOID,
@@ -125,9 +125,9 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
                tlist = lappend(tlist, tle);
            }
        }
-       else
+       if (rc->allMarkTypes & (1 << ROW_MARK_COPY))
        {
-           /* Not a table, so we need the whole row as a junk var */
+           /* Need the whole row as a junk var */
            var = makeWholeRowVar(rt_fetch(rc->rti, range_table),
                                  rc->rti,
                                  0,

diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index b90fee387b4c73af26ef1e2235fd92a7546da98e..cd40afce767cc45fdb3397bec65c2d71a7659ce6 100644 (file)
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -1389,9 +1389,14 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
            newrc->prti = rti;
            newrc->rowmarkId = oldrc->rowmarkId;
            newrc->markType = oldrc->markType;
+           newrc->allMarkTypes = (1 << newrc->markType);
+           newrc->strength = oldrc->strength;
            newrc->waitPolicy = oldrc->waitPolicy;
            newrc->isParent = false;
 
+           /* Include child's rowmark type in parent's allMarkTypes */
+           oldrc->allMarkTypes |= newrc->allMarkTypes;
+
            root->rowMarks = lappend(root->rowMarks, newrc);
        }
 

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index a68f2e8bb1406d117ab505382f2af9bdbbf4d3c2..4a5a5205391b932d76f71f2e34660f724426543a 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -2254,11 +2254,18 @@ transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt)
 }
 
 
-char *
+/*
+ * Produce a string representation of a LockClauseStrength value.
+ * This should only be applied to valid values (not LCS_NONE).
+ */
+const char *
 LCS_asString(LockClauseStrength strength)
 {
    switch (strength)
    {
+       case LCS_NONE:
+           Assert(false);
+           break;
        case LCS_FORKEYSHARE:
            return "FOR KEY SHARE";
        case LCS_FORSHARE:
@@ -2279,6 +2286,8 @@ LCS_asString(LockClauseStrength strength)
 void
 CheckSelectLocking(Query *qry, LockClauseStrength strength)
 {
+   Assert(strength != LCS_NONE);       /* else caller error */
+
    if (qry->setOperations)
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -2498,6 +2507,8 @@ applyLockingClause(Query *qry, Index rtindex,
 {
    RowMarkClause *rc;
 
+   Assert(strength != LCS_NONE);       /* else caller error */
+
    /* If it's an explicit clause, make sure hasForUpdate gets set */
    if (!pushedDown)
        qry->hasForUpdate = true;
@@ -2506,20 +2517,21 @@ applyLockingClause(Query *qry, Index rtindex,
    if ((rc = get_parse_rowmark(qry, rtindex)) != NULL)
    {
        /*
-        * If the same RTE is specified for more than one locking strength,
-        * treat is as the strongest.  (Reasonable, since you can't take both
-        * a shared and exclusive lock at the same time; it'll end up being
-        * exclusive anyway.)
+        * If the same RTE is specified with more than one locking strength,
+        * use the strongest.  (Reasonable, since you can't take both a shared
+        * and exclusive lock at the same time; it'll end up being exclusive
+        * anyway.)
         *
-        * Similarly, if the same RTE is specified with more than one lock wait
-        * policy, consider that NOWAIT wins over SKIP LOCKED, which in turn
-        * wins over waiting for the lock (the default).  This is a bit more
-        * debatable but raising an error doesn't seem helpful.  (Consider for
-        * instance SELECT FOR UPDATE NOWAIT from a view that internally
+        * Similarly, if the same RTE is specified with more than one lock
+        * wait policy, consider that NOWAIT wins over SKIP LOCKED, which in
+        * turn wins over waiting for the lock (the default).  This is a bit
+        * more debatable but raising an error doesn't seem helpful. (Consider
+        * for instance SELECT FOR UPDATE NOWAIT from a view that internally
         * contains a plain FOR UPDATE spec.)  Having NOWAIT win over SKIP
         * LOCKED is reasonable since the former throws an error in case of
-        * coming across a locked tuple, which may be undesirable in some cases
-        * but it seems better than silently returning inconsistent results.
+        * coming across a locked tuple, which may be undesirable in some
+        * cases but it seems better than silently returning inconsistent
+        * results.
         *
         * And of course pushedDown becomes false if any clause is explicit.
         */

diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 126e38d7f73935812c15bbaf2076137fb97fd146..065475dda2a0ba75736e6935a82b17bf0ff54b13 100644 (file)
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -2395,26 +2395,22 @@ CreateCommandTag(Node *parsetree)
                        else if (stmt->rowMarks != NIL)
                        {
                            /* not 100% but probably close enough */
-                           switch (((PlanRowMark *) linitial(stmt->rowMarks))->markType)
+                           switch (((PlanRowMark *) linitial(stmt->rowMarks))->strength)
                            {
-                               case ROW_MARK_EXCLUSIVE:
-                                   tag = "SELECT FOR UPDATE";
-                                   break;
-                               case ROW_MARK_NOKEYEXCLUSIVE:
-                                   tag = "SELECT FOR NO KEY UPDATE";
+                               case LCS_FORKEYSHARE:
+                                   tag = "SELECT FOR KEY SHARE";
                                    break;
-                               case ROW_MARK_SHARE:
+                               case LCS_FORSHARE:
                                    tag = "SELECT FOR SHARE";
                                    break;
-                               case ROW_MARK_KEYSHARE:
-                                   tag = "SELECT FOR KEY SHARE";
+                               case LCS_FORNOKEYUPDATE:
+                                   tag = "SELECT FOR NO KEY UPDATE";
                                    break;
-                               case ROW_MARK_REFERENCE:
-                               case ROW_MARK_COPY:
-                                   tag = "SELECT";
+                               case LCS_FORUPDATE:
+                                   tag = "SELECT FOR UPDATE";
                                    break;
                                default:
-                                   tag = "???";
+                                   tag = "SELECT";
                                    break;
                            }
                        }

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 2fa30be401f77be32fbc12e5d15dd63a985050d2..28e1acfb86a9d9e704a04336fb5c0a1364b312fa 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -4453,6 +4453,11 @@ get_select_query_def(Query *query, deparse_context *context,
 
            switch (rc->strength)
            {
+               case LCS_NONE:
+                   /* we intentionally throw an error for LCS_NONE */
+                   elog(ERROR, "unrecognized LockClauseStrength %d",
+                        (int) rc->strength);
+                   break;
                case LCS_FORKEYSHARE:
                    appendContextKeyword(context, " FOR KEY SHARE",
                                     -PRETTYINDENT_STD, PRETTYINDENT_STD, 0);

diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 76c64cd1227337c7c76e0cf8a35fb5ca4e1ad7db..479fc91d4662b66e0d34898d0e8c0c84aa23b43c 100644 (file)
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*                         yyyymmddN */
-#define CATALOG_VERSION_NO 201503061
+#define CATALOG_VERSION_NO 201503151
 
 #endif

diff --git a/src/include/nodes/lockoptions.h b/src/include/nodes/lockoptions.h
index 55324baf40f7618520ce4a34df526a81d957f83d..2e55622b043089032dd089474999d6b19ce10721 100644 (file)
--- a/src/include/nodes/lockoptions.h
+++ b/src/include/nodes/lockoptions.h
@@ -20,6 +20,7 @@
  */
 typedef enum LockClauseStrength
 {
+   LCS_NONE,                   /* no such clause - only used in PlanRowMark */
    LCS_FORKEYSHARE,            /* FOR KEY SHARE */
    LCS_FORSHARE,               /* FOR SHARE */
    LCS_FORNOKEYUPDATE,         /* FOR NO KEY UPDATE */

diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index af44ddf5dc552db88a8e32418003c08a6bd2aa9b..21cbfa8cf0febf77d67e6b82b85a07aaa8cf746d 100644 (file)
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -820,7 +820,7 @@ typedef enum RowMarkType
    ROW_MARK_NOKEYEXCLUSIVE,    /* obtain no-key exclusive tuple lock */
    ROW_MARK_SHARE,             /* obtain shared tuple lock */
    ROW_MARK_KEYSHARE,          /* obtain keyshare tuple lock */
-   ROW_MARK_REFERENCE,         /* just fetch the TID */
+   ROW_MARK_REFERENCE,         /* just fetch the TID, don't lock it */
    ROW_MARK_COPY               /* physically copy the row value */
 } RowMarkType;
 
@@ -841,7 +841,9 @@ typedef enum RowMarkType
  * list for each child relation (including the target rel itself in its role
  * as a child).  The child entries have rti == child rel's RT index and
  * prti == parent's RT index, and can therefore be recognized as children by
- * the fact that prti != rti.
+ * the fact that prti != rti.  The parent's allMarkTypes field gets the OR
+ * of (1<
+ * to use different markTypes).
  *
  * The planner also adds resjunk output columns to the plan that carry
  * information sufficient to identify the locked or fetched rows.  For
@@ -851,6 +853,8 @@ typedef enum RowMarkType
  * The tableoid column is only present for an inheritance hierarchy.
  * When markType == ROW_MARK_COPY, there is instead a single column named
  *     wholerow%u          whole-row value of relation
+ * (An inheritance hierarchy could have all three resjunk output columns,
+ * if some children use a different markType than others.)
  * In all three cases, %u represents the rowmark ID number (rowmarkId).
  * This number is unique within a plan tree, except that child relation
  * entries copy their parent's rowmarkId.  (Assigning unique numbers
@@ -867,6 +871,8 @@ typedef struct PlanRowMark
    Index       prti;           /* range table index of parent relation */
    Index       rowmarkId;      /* unique identifier for resjunk columns */
    RowMarkType markType;       /* see enum above */
+   int         allMarkTypes;   /* OR of (1<
+   LockClauseStrength strength;    /* LockingClause's strength, or LCS_NONE */
    LockWaitPolicy waitPolicy;  /* NOWAIT and SKIP LOCKED options */
    bool        isParent;       /* true if this is a "dummy" parent entry */
 } PlanRowMark;


diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h

index 4a31cbf9b034e247f7026c8b37c7a02424efdc97..258d9d9b3eb80cf683fb0fc0948e89922dd61b96 100644 (file)

--- a/src/include/parser/analyze.h
+++ b/src/include/parser/analyze.h
@@ -36,7 +36,7 @@ extern Query *transformStmt(ParseState *pstate, Node *parseTree);
 
 extern bool analyze_requires_snapshot(Node *parseTree);
 
-extern char *LCS_asString(LockClauseStrength strength);
+extern const char *LCS_asString(LockClauseStrength strength);
 extern void CheckSelectLocking(Query *qry, LockClauseStrength strength);
 extern void applyLockingClause(Query *qry, Index rtindex,
                   LockClauseStrength strength,

diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h
index 4a31cbf9b034e247f7026c8b37c7a02424efdc97..258d9d9b3eb80cf683fb0fc0948e89922dd61b96 100644 (file)
--- a/src/include/parser/analyze.h
+++ b/src/include/parser/analyze.h
@@ -36,7 +36,7 @@ extern Query *transformStmt(ParseState *pstate, Node *parseTree);
 
 extern bool analyze_requires_snapshot(Node *parseTree);
 
-extern char *LCS_asString(LockClauseStrength strength);
+extern const char *LCS_asString(LockClauseStrength strength);
 extern void CheckSelectLocking(Query *qry, LockClauseStrength strength);
 extern void applyLockingClause(Query *qry, Index rtindex,
                   LockClauseStrength strength,