While looking code further around this, I realized that we need
similar kind of fix for bitmap as well as index only scan as well.
Here is the patch, which does similar fix for bitmap and indexonly
scans.
Thanks,
On Fri, Nov 23, 2018 at 6:47 PM Rushabh Lathia <rushabh.lat...@gmail.com>
wrote:
>
>
> On Fri, Nov 23, 2018 at 3:33 AM David Rowley <david.row...@2ndquadrant.com>
> wrote:
>
>> On Thu, 22 Nov 2018 at 22:33, Rushabh Lathia <rushabh.lat...@gmail.com>
>> wrote:
>> > CREATE TABLE foo (x int primary key);
>> > INSERT INTO foo VALUES (1), (2), (3), (4), (5);
>> >
>> > CREATE OR REPLACE FUNCTION f1(a int) RETURNS int
>> > AS $$
>> > BEGIN
>> > DELETE FROM foo where x = a;
>> > return 0;
>> > END;
>> > $$ LANGUAGE plpgsql;
>> >
>> > postgres@100858=#set plan_cache_mode = force_generic_plan;
>> > SET
>> > postgres@100858=#select f1(4);
>> > f1
>> > ----
>> > 0
>> > (1 row)
>> >
>> > postgres@100858=#select f1(4);
>> > server closed the connection unexpectedly
>>
>>
>> > Now, to fix this issue either we need to hold proper lock before
>> reaching
>> > to ExecInitIndexScan() or teach ExecInitIndexScan() to take
>> AccessShareLock
>> > on the scan coming from CMD_DELETE.
>>
>> I'd say the following comment and code in nodeIndexscan.c is outdated:
>>
>> /*
>> * Open the index relation.
>> *
>> * If the parent table is one of the target relations of the query, then
>> * InitPlan already opened and write-locked the index, so we can avoid
>> * taking another lock here. Otherwise we need a normal reader's lock.
>> */
>> relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
>> indexstate->iss_RelationDesc = index_open(node->indexid,
>> relistarget ? NoLock : AccessShareLock);
>>
>> Despite what the above comment claims, these indexes have not been
>> opened in InitPlan since 389af951552ff2. As you mentioned, they're
>> opened in nodeModifyTable.c for non-delete statements.
>>
>>
> +1.
>
> I tried the same approach and with further testing I haven't found
> any issues.
>
>
>> I think we either need to update the above code to align it to what's
>> now in nodeModifyTable.c, or just obtain an AccessShareLock
>> regardless. I kinda think we should just take the lock regardless as
>> determining if the relation is a result relation may be much more
>> expensive than just taking another lower-level lock on the index.
>>
>> Anyway. I've attached a small patch to update the above fragment.
>>
>> --
>> David Rowley http://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>
> --
> Rushabh Lathia
>
--
Rushabh Lathia
diff --git a/src/backend/executor/nodeBitmapIndexscan.c b/src/backend/executor/nodeBitmapIndexscan.c
index d04f490..c615316 100644
--- a/src/backend/executor/nodeBitmapIndexscan.c
+++ b/src/backend/executor/nodeBitmapIndexscan.c
@@ -210,7 +210,7 @@ BitmapIndexScanState *
ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate, int eflags)
{
BitmapIndexScanState *indexstate;
- bool relistarget;
+ bool indexlocked;
/* check for unsupported flags */
Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)));
@@ -266,9 +266,17 @@ ExecInitBitmapIndexScan(BitmapIndexScan *node, EState *estate, int eflags)
* InitPlan already opened and write-locked the index, so we can avoid
* taking another lock here. Otherwise we need a normal reader's lock.
*/
- relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
+ /*
+ * Open the index relation.
+ *
+ * For non-DELETE statement when the parent table is one of the target
+ * relations of the query, ExecInitModifyTable will already have obtained
+ * a lock on the index, otherwise we must obtain a read lock here.
+ */
+ indexlocked = estate->es_plannedstmt->commandType != CMD_DELETE &&
+ ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
indexstate->biss_RelationDesc = index_open(node->indexid,
- relistarget ? NoLock : AccessShareLock);
+ indexlocked ? NoLock : AccessShareLock);
/*
* Initialize index-specific scan state
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index d1201a1..d9caf00 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -493,8 +493,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
{
IndexOnlyScanState *indexstate;
Relation currentRelation;
- bool relistarget;
TupleDesc tupDesc;
+ bool indexlocked;
/*
* create state structure
@@ -558,13 +558,14 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
/*
* Open the index relation.
*
- * If the parent table is one of the target relations of the query, then
- * InitPlan already opened and write-locked the index, so we can avoid
- * taking another lock here. Otherwise we need a normal reader's lock.
+ * For non-DELETE statement when the parent table is one of the target
+ * relations of the query, ExecInitModifyTable will already have obtained
+ * a lock on the index, otherwise we must obtain a read lock here.
*/
- relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
+ indexlocked = estate->es_plannedstmt->commandType != CMD_DELETE &&
+ ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
indexstate->ioss_RelationDesc = index_open(node->indexid,
- relistarget ? NoLock : AccessShareLock);
+ indexlocked ? NoLock : AccessShareLock);
/*
* Initialize index-specific scan state
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index f209173..d7c6986 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -920,7 +920,7 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
{
IndexScanState *indexstate;
Relation currentRelation;
- bool relistarget;
+ bool indexlocked;
/*
* create state structure
@@ -986,13 +986,14 @@ ExecInitIndexScan(IndexScan *node, EState *estate, int eflags)
/*
* Open the index relation.
*
- * If the parent table is one of the target relations of the query, then
- * InitPlan already opened and write-locked the index, so we can avoid
- * taking another lock here. Otherwise we need a normal reader's lock.
+ * For non-DELETE statement when the parent table is one of the target
+ * relations of the query, ExecInitModifyTable will already have obtained
+ * a lock on the index, otherwise we must obtain a read lock here.
*/
- relistarget = ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
+ indexlocked = estate->es_plannedstmt->commandType != CMD_DELETE &&
+ ExecRelationIsTargetRelation(estate, node->scan.scanrelid);
indexstate->iss_RelationDesc = index_open(node->indexid,
- relistarget ? NoLock : AccessShareLock);
+ indexlocked ? NoLock : AccessShareLock);
/*
* Initialize index-specific scan state
