Restore sane locking behavior during parallel query.

Commit 9a3cebeaa changed things so that parallel workers didn't obtain
any lock of their own on tables they access.  That was clearly a bad
idea, but I'd mistakenly supposed that it was the intended end result
of the series of patches for simplifying the executor's lock management.
Undo that change in relation_open(), and adjust ExecOpenScanRelation()
so that it gets the correct lock if inside a parallel worker.

In passing, clean up some more obsolete comments about when locks
are acquired.

Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 727d6776e13f6b03967fe9bb386c1084c3dc01ab..5f1a69ca53c6ea8f6b4a13bf4d6d8705cd22555b 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1140,13 +1140,9 @@ relation_open(Oid relationId, LOCKMODE lockmode)
        /*
         * If we didn't get the lock ourselves, assert that caller holds one,
         * except in bootstrap mode where no locks are used.
-        *
-        * Also, parallel workers currently assume that their parent holds locks
-        * for tables used in the parallel query (a mighty shaky assumption).
         */
        Assert(lockmode != NoLock ||
                   IsBootstrapProcessingMode() ||
-                  IsParallelWorker() ||
                   CheckRelationLockedByMe(r, AccessShareLock, true));
 
        /* Make note that we've accessed a temporary relation */

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 23e6749920a1bb96e8e6599c7aeb0158e62c84fb..b6abad554a404a22bd7302c8bc453c22a5cf736e 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1622,8 +1622,8 @@ ExecEndPlan(PlanState *planstate, EState *estate)
        }
 
        /*
-        * close whatever rangetable Relations have been opened.  We did not
-        * acquire locks in ExecGetRangeTableRelation, so don't release 'em here.
+        * close whatever rangetable Relations have been opened.  We do not
+        * release any locks we might hold on those rels.
         */
        num_relations = estate->es_range_table_size;
        for (i = 0; i < num_relations; i++)

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 650fd81ff1729898d3fd9608bc7bbdb023c09b66..d98e90e71173b9c38328fb81afe254b3c9a27d70 100644 (file)
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -732,16 +732,30 @@ ExecGetRangeTableRelation(EState *estate, Index rti)
 
                Assert(rte->rtekind == RTE_RELATION);
 
-               rel = estate->es_relations[rti - 1] = heap_open(rte->relid, NoLock);
+               if (!IsParallelWorker())
+               {
+                       /*
+                        * In a normal query, we should already have the appropriate lock,
+                        * but verify that through an Assert.  Since there's already an
+                        * Assert inside heap_open that insists on holding some lock, it
+                        * seems sufficient to check this only when rellockmode is higher
+                        * than the minimum.
+                        */
+                       rel = heap_open(rte->relid, NoLock);
+                       Assert(rte->rellockmode == AccessShareLock ||
+                                  CheckRelationLockedByMe(rel, rte->rellockmode, false));
+               }
+               else
+               {
+                       /*
+                        * If we are a parallel worker, we need to obtain our own local
+                        * lock on the relation.  This ensures sane behavior in case the
+                        * parent process exits before we do.
+                        */
+                       rel = heap_open(rte->relid, rte->rellockmode);
+               }
 
-               /*
-                * Verify that appropriate lock was obtained before execution.
-                *
-                * In the case of parallel query, only the leader would've obtained
-                * the lock (that needs to be fixed, though).
-                */
-               Assert(IsParallelWorker() ||
-                          CheckRelationLockedByMe(rel, rte->rellockmode, false));
+               estate->es_relations[rti - 1] = rel;
        }
 
        return rel;

diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 5307cd1b87033c2f7fe516ae107727ece21e5caf..304ef07f2cc4e3fde33c4683683e884f239a0e37 100644 (file)
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -899,16 +899,12 @@ ExecInitBitmapHeapScan(BitmapHeapScan *node, EState *estate, int eflags)
        ExecAssignExprContext(estate, &scanstate->ss.ps);
 
        /*
-        * open the base relation and acquire appropriate lock on it.
+        * open the scan relation
         */
        currentRelation = ExecOpenScanRelation(estate, node->scan.scanrelid, eflags);
 
        /*
         * initialize child nodes
-        *
-        * We do this after ExecOpenScanRelation because the child nodes will open
-        * indexscans on our relation's indexes, and we want to be sure we have
-        * acquired a lock on the relation first.
         */
        outerPlanState(scanstate) = ExecInitNode(outerPlan(node), estate, eflags);
 

diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c
index 9a33eda688797a9c004eb6bb765a0d3782fdb7d6..7972d5a952a28784a4763610328884f50e7d6dbf 100644 (file)
--- a/src/backend/executor/nodeCustom.c
+++ b/src/backend/executor/nodeCustom.c
@@ -55,7 +55,7 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
        ExecAssignExprContext(estate, &css->ss.ps);
 
        /*
-        * open the base relation, if any, and acquire an appropriate lock on it
+        * open the scan relation, if any
         */
        if (scanrelid > 0)
        {

diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index cf7df72d8c2e93b11cc2d593dd436dd094b8ee43..2ec7fcb9621cce038ba23a3645e06bbcd58343df 100644 (file)
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -156,8 +156,8 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
        ExecAssignExprContext(estate, &scanstate->ss.ps);
 
        /*
-        * open the base relation, if any, and acquire an appropriate lock on it;
-        * also acquire function pointers from the FDW's handler
+        * open the scan relation, if any; also acquire function pointers from the
+        * FDW's handler
         */
        if (scanrelid > 0)
        {

diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index 1b530cea405cc4546bf46bace66385fcc77e6243..daedf342f726d876e93673cab9139fb6de0b9719 100644 (file)
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -511,7 +511,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
        ExecAssignExprContext(estate, &indexstate->ss.ps);
 
        /*
-        * open the base relation and acquire appropriate lock on it.
+        * open the scan relation
         */
        currentRelation = ExecOpenScanRelation(estate, node->scan.scanrelid, eflags);
 

diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index d9527669f53db627e2daf5028e15a6dba0d20b18..ba7821b0e2b3a5b120036971eaf28ff1a6bc9213 100644 (file)
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -933,7 +933,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
        ExecAssignExprContext(estate, &indexstate->ss.ps);
 
        /*
-        * open the base relation and acquire appropriate lock on it.
+        * open the scan relation
         */
        currentRelation = ExecOpenScanRelation(estate, node->scan.scanrelid, eflags);
 

diff --git a/src/backend/executor/nodeSamplescan.c b/src/backend/executor/nodeSamplescan.c
index 70ae1bc7e462c9027856c3ea0b169ef0a121b466..f01fc3b62a088c96d6319455631e83915016970c 100644 (file)
--- a/src/backend/executor/nodeSamplescan.c
+++ b/src/backend/executor/nodeSamplescan.c
@@ -134,10 +134,7 @@ ExecInitSampleScan(SampleScan *node, EState *estate, int eflags)
        ExecAssignExprContext(estate, &scanstate->ss.ps);
 
        /*
-        * Initialize scan relation.
-        *
-        * Get the relation object id from the relid'th entry in the range table,
-        * open that relation and acquire appropriate lock on it.
+        * open the scan relation
         */
        scanstate->ss.ss_currentRelation =
                ExecOpenScanRelation(estate,

diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index 5dede816c6a8275cabecadbabb6755d2ee5409ac..79729dbbecb3019b519bf6c1353ca58951fe45cd 100644 (file)
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -163,10 +163,7 @@ ExecInitSeqScan(SeqScan *node, EState *estate, int eflags)
        ExecAssignExprContext(estate, &scanstate->ss.ps);
 
        /*
-        * Initialize scan relation.
-        *
-        * Get the relation object id from the relid'th entry in the range table,
-        * open that relation and acquire appropriate lock on it.
+        * open the scan relation
         */
        scanstate->ss.ss_currentRelation =
                ExecOpenScanRelation(estate,

diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index 593185f726ba65a28cbbe9650f5a53eba4464eff..d21d6553e4247e5202e64eac7c4b8972f627a331 100644 (file)
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -531,7 +531,7 @@ ExecInitTidScan(TidScan *node, EState *estate, int eflags)
        tidstate->tss_TidPtr = -1;
 
        /*
-        * open the base relation and acquire appropriate lock on it.
+        * open the scan relation
         */
        currentRelation = ExecOpenScanRelation(estate, node->scan.scanrelid, eflags);