Heap lock levels for REINDEX INDEX CONCURRENTLY not quite right?
Hi,
While looking at /messages/by-id/20190430070552.jzqgcy4ihalx7nur@alap3.anarazel.de
I noticed that
/*
* ReindexIndex
* Recreate a specific index.
*/
void
ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
{
Oid indOid;
Oid heapOid = InvalidOid;
Relation irel;
char persistence;
/*
* Find and lock index, and check permissions on table; use callback to
* obtain lock on table first, to avoid deadlock hazard. The lock level
* used here must match the index lock obtained in reindex_index().
*/
indOid = RangeVarGetRelidExtended(indexRelation,
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
(void *) &heapOid);
doesn't pass concurrent-ness to RangeVarCallbackForReindexIndex(). Which
then goes on to lock the table
static void
RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg)
if (OidIsValid(*heapOid))
LockRelationOid(*heapOid, ShareLock);
without knowing that it should use ShareUpdateExclusive. Which
e.g. ReindexTable knows:
/* The lock level used here should match reindex_relation(). */
heapOid = RangeVarGetRelidExtended(relation,
concurrent ? ShareUpdateExclusiveLock : ShareLock,
0,
RangeVarCallbackOwnsTable, NULL);
so there's a lock upgrade hazard.
Creating a table
CREATE TABLE blarg(id serial primary key);
and then using pgbench to reindex it:
REINDEX INDEX CONCURRENTLY blarg_pkey;
indeed proves that there's a problem:
2019-04-30 08:12:58.679 PDT [30844][7/925] ERROR: 40P01: deadlock detected
2019-04-30 08:12:58.679 PDT [30844][7/925] DETAIL: Process 30844 waits for ShareUpdateExclusiveLock on relation 50661 of database 13408; blocked by process 30848.
Process 30848 waits for ShareUpdateExclusiveLock on relation 50667 of database 13408; blocked by process 30844.
Process 30844: REINDEX INDEX CONCURRENTLY blarg_pkey;
Process 30848: REINDEX INDEX CONCURRENTLY blarg_pkey;
2019-04-30 08:12:58.679 PDT [30844][7/925] HINT: See server log for query details.
2019-04-30 08:12:58.679 PDT [30844][7/925] LOCATION: DeadLockReport, deadlock.c:1140
2019-04-30 08:12:58.679 PDT [30844][7/925] STATEMENT: REINDEX INDEX CONCURRENTLY blarg_pkey;
I assume the fix woudl be to pass a struct {LOCKMODE lockmode; Oid
heapOid;} to RangeVarCallbackForReindexIndex().
Greetings,
Andres Freund
On 2019-04-30 17:17, Andres Freund wrote:
indOid = RangeVarGetRelidExtended(indexRelation,
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
(void *) &heapOid);doesn't pass concurrent-ness to RangeVarCallbackForReindexIndex(). Which
then goes on to lock the tablestatic void
RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg)if (OidIsValid(*heapOid))
LockRelationOid(*heapOid, ShareLock);without knowing that it should use ShareUpdateExclusive. Which
e.g. ReindexTable knows:/* The lock level used here should match reindex_relation(). */
heapOid = RangeVarGetRelidExtended(relation,
concurrent ? ShareUpdateExclusiveLock : ShareLock,
0,
RangeVarCallbackOwnsTable, NULL);so there's a lock upgrade hazard.
Confirmed.
What seems weird to me is that the existing callback argument heapOid
isn't used at all. It seems to have been like that since the original
commit of the callback infrastructure. Therefore also, this code
if (relId != oldRelId && OidIsValid(oldRelId))
{
/* lock level here should match reindex_index() heap lock */
UnlockRelationOid(*heapOid, ShareLock);
in RangeVarCallbackForReindexIndex() can't ever do anything useful.
Patch to remove the unused code attached; but needs some checking for
this dubious conditional block.
Thoughts?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Remove-unused-argument-of-RangeVarCallbackForReindex.patchtext/plain; charset=UTF-8; name=0001-Remove-unused-argument-of-RangeVarCallbackForReindex.patch; x-mac-creator=0; x-mac-type=0Download
From 89db1445787b3fb676ecfa07964682e72a58e7bb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 2 May 2019 10:38:06 +0200
Subject: [PATCH] Remove unused argument of RangeVarCallbackForReindexIndex()
---
src/backend/commands/indexcmds.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fabba3bacd..2800255d9d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2304,7 +2304,6 @@ void
ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
{
Oid indOid;
- Oid heapOid = InvalidOid;
Relation irel;
char persistence;
@@ -2317,7 +2316,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
- (void *) &heapOid);
+ NULL);
/*
* Obtain the current persistence of the existing index. We already hold
@@ -2350,19 +2349,6 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg)
{
char relkind;
- Oid *heapOid = (Oid *) arg;
-
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
- {
- /* lock level here should match reindex_index() heap lock */
- UnlockRelationOid(*heapOid, ShareLock);
- *heapOid = InvalidOid;
- }
/* If the relation does not exist, there's nothing more to do. */
if (!OidIsValid(relId))
@@ -2394,9 +2380,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* isn't valid, it means the index as concurrently dropped, which is
* not a problem for us; just return normally.
*/
- *heapOid = IndexGetRelation(relId, true);
- if (OidIsValid(*heapOid))
- LockRelationOid(*heapOid, ShareLock);
+ Oid heapOid = IndexGetRelation(relId, true);
+ if (OidIsValid(heapOid))
+ LockRelationOid(heapOid, ShareLock);
}
}
--
2.21.0
On 2019-05-02 10:44:44 +0200, Peter Eisentraut wrote:
On 2019-04-30 17:17, Andres Freund wrote:
indOid = RangeVarGetRelidExtended(indexRelation,
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
(void *) &heapOid);doesn't pass concurrent-ness to RangeVarCallbackForReindexIndex(). Which
then goes on to lock the tablestatic void
RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg)if (OidIsValid(*heapOid))
LockRelationOid(*heapOid, ShareLock);without knowing that it should use ShareUpdateExclusive. Which
e.g. ReindexTable knows:/* The lock level used here should match reindex_relation(). */
heapOid = RangeVarGetRelidExtended(relation,
concurrent ? ShareUpdateExclusiveLock : ShareLock,
0,
RangeVarCallbackOwnsTable, NULL);so there's a lock upgrade hazard.
Confirmed.
What seems weird to me is that the existing callback argument heapOid
isn't used at all. It seems to have been like that since the original
commit of the callback infrastructure. Therefore also, this code
Hm? But that's a different callback from the one used from
reindex_index()? reindex_relation() uses the
RangeVarCallbackOwnsTable() callback and passes in NULL as the argument,
whereas reindex_index() passses in RangeVarCallbackForReindexIndex() and
passes in &heapOid?
And RangeVarCallbackForReindexIndex() pretty clearly sets it *heapOid:
* Lock level here should match reindex_index() heap lock. If the OID
* isn't valid, it means the index as concurrently dropped, which is
* not a problem for us; just return normally.
*/
*heapOid = IndexGetRelation(relId, true);
Patch to remove the unused code attached; but needs some checking for
this dubious conditional block.Thoughts?
I might miss something here, and it's actually unused. But if so the fix
would be to make it being used, because it's actually
important. Otherwise ReindexIndex() becomes racy or has even more
deadlock hazards.
Greetings,
Andres Freund
On 2019-05-02 16:33, Andres Freund wrote:
And RangeVarCallbackForReindexIndex() pretty clearly sets it *heapOid:
* Lock level here should match reindex_index() heap lock. If the OID
* isn't valid, it means the index as concurrently dropped, which is
* not a problem for us; just return normally.
*/
*heapOid = IndexGetRelation(relId, true);
It sets it but uses it only internally. There is no code path that
passes in a non-zero heapOid, and there is no code path that does
anything with the heapOid passed back out.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On 2019-05-02 22:39:15 +0200, Peter Eisentraut wrote:
On 2019-05-02 16:33, Andres Freund wrote:
And RangeVarCallbackForReindexIndex() pretty clearly sets it *heapOid:
* Lock level here should match reindex_index() heap lock. If the OID
* isn't valid, it means the index as concurrently dropped, which is
* not a problem for us; just return normally.
*/
*heapOid = IndexGetRelation(relId, true);It sets it but uses it only internally. There is no code path that
passes in a non-zero heapOid, and there is no code path that does
anything with the heapOid passed back out.
RangeVarGetRelidExtended() can call the callback multiple times, if
there are any concurrent schema changes. That's why it's unlocking the
previously locked heap oid.
Greetings,
Andres Freund
On 2019-05-02 22:42, Andres Freund wrote:
RangeVarGetRelidExtended() can call the callback multiple times, if
there are any concurrent schema changes. That's why it's unlocking the
previously locked heap oid.
Ah that explains it then.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-05-02 10:44, Peter Eisentraut wrote:
so there's a lock upgrade hazard.
Confirmed.
Here is a patch along the lines of your sketch. I cleaned up the
variable naming a bit too.
REINDEX CONCURRENTLY is still deadlock prone because of
WaitForOlderSnapshots(), so this doesn't actually fix your test case,
but that seems unrelated to this particular issue.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-table-lock-levels-for-REINDEX-INDEX-CONCURRENTLY.patchtext/plain; charset=UTF-8; name=0001-Fix-table-lock-levels-for-REINDEX-INDEX-CONCURRENTLY.patch; x-mac-creator=0; x-mac-type=0Download
From 8a4dcc3e6b849fd72bed29c9308c9a897eec0cc5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 3 May 2019 09:15:29 +0200
Subject: [PATCH] Fix table lock levels for REINDEX INDEX CONCURRENTLY
REINDEX CONCURRENTLY locks tables with ShareUpdateExclusiveLock rather
than the ShareLock used by a plain REINDEX. However,
RangeVarCallbackForReindexIndex() was not updated for that and still
used the ShareLock only. This would lead to lock upgrades later,
leading to possible deadlocks.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/20190430151735.wi52sxjvxsjvaxxt%40alap3.anarazel.de
---
src/backend/commands/indexcmds.c | 45 +++++++++++++++++++++++---------
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fabba3bacd..acfb2d24c9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -91,6 +91,15 @@ static bool ReindexRelationConcurrently(Oid relationOid, int options);
static void ReindexPartitionedIndex(Relation parentIdx);
static void update_relispartition(Oid relationId, bool newval);
+/*
+ * callback argument type for RangeVarCallbackForReindexIndex()
+ */
+struct ReindexIndexCallbackState
+{
+ bool concurrent; /* flag from statement */
+ Oid locked_table_oid; /* tracks previously locked table */
+};
+
/*
* CheckIndexCompatible
* Determine whether an existing index definition is compatible with a
@@ -2303,8 +2312,8 @@ ChooseIndexColumnNames(List *indexElems)
void
ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
{
+ struct ReindexIndexCallbackState state;
Oid indOid;
- Oid heapOid = InvalidOid;
Relation irel;
char persistence;
@@ -2313,11 +2322,13 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
* obtain lock on table first, to avoid deadlock hazard. The lock level
* used here must match the index lock obtained in reindex_index().
*/
+ state.concurrent = concurrent;
+ state.locked_table_oid = InvalidOid;
indOid = RangeVarGetRelidExtended(indexRelation,
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
- (void *) &heapOid);
+ &state);
/*
* Obtain the current persistence of the existing index. We already hold
@@ -2350,7 +2361,15 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg)
{
char relkind;
- Oid *heapOid = (Oid *) arg;
+ struct ReindexIndexCallbackState *state = arg;
+ LOCKMODE table_lockmode;
+
+ /*
+ * Lock level here should match table lock in reindex_index() for
+ * non-concurrent case and table locks used by index_concurrently_*() for
+ * concurrent case.
+ */
+ table_lockmode = state->concurrent ? ShareUpdateExclusiveLock : ShareLock;
/*
* If we previously locked some other index's heap, and the name we're
@@ -2359,9 +2378,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
*/
if (relId != oldRelId && OidIsValid(oldRelId))
{
- /* lock level here should match reindex_index() heap lock */
- UnlockRelationOid(*heapOid, ShareLock);
- *heapOid = InvalidOid;
+ UnlockRelationOid(state->locked_table_oid, table_lockmode);
+ state->locked_table_oid = InvalidOid;
}
/* If the relation does not exist, there's nothing more to do. */
@@ -2389,14 +2407,17 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
/* Lock heap before index to avoid deadlock. */
if (relId != oldRelId)
{
+ Oid table_oid = IndexGetRelation(relId, true);
+
/*
- * Lock level here should match reindex_index() heap lock. If the OID
- * isn't valid, it means the index as concurrently dropped, which is
- * not a problem for us; just return normally.
+ * If the OID isn't valid, it means the index as concurrently dropped,
+ * which is not a problem for us; just return normally.
*/
- *heapOid = IndexGetRelation(relId, true);
- if (OidIsValid(*heapOid))
- LockRelationOid(*heapOid, ShareLock);
+ if (OidIsValid(table_oid))
+ {
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
+ }
}
}
--
2.21.0
Hi,
On 2019-05-03 09:37:07 +0200, Peter Eisentraut wrote:
REINDEX CONCURRENTLY is still deadlock prone because of
WaitForOlderSnapshots(), so this doesn't actually fix your test case,
but that seems unrelated to this particular issue.
Right.
I've not tested the change, but it looks reasonable to me. The change
of moving the logic the reset of *heapOid to the unlock perhaps is
debatable, but I think it's OK.
Greetings,
Andres Freund
On Fri, May 03, 2019 at 08:23:21AM -0700, Andres Freund wrote:
I've not tested the change, but it looks reasonable to me. The change
of moving the logic the reset of *heapOid to the unlock perhaps is
debatable, but I think it's OK.
I have not checked the patch in details yet, but it strikes me that
we should have an isolation test case which does the following:
- Take a lock on the table created, without committing yet the
transaction where the lock is taken.
- Run two REINDEX CONCURRENTLY in two other sessions.
- Commit the first transaction.
The result should be no deadlocks happening in the two sessions
running the reindex. I can see the deadlock easily with three psql
sessions, running manually the queries.
--
Michael
On Sat, May 04, 2019 at 09:59:20PM +0900, Michael Paquier wrote:
The result should be no deadlocks happening in the two sessions
running the reindex. I can see the deadlock easily with three psql
sessions, running manually the queries.
+ * If the OID isn't valid, it means the index as concurrently dropped,
+ * which is not a problem for us; just return normally.
Typo here s/as/is/.I have looked closer at the patch and the change proposed looks good
to me.
Now, what do we do about the potential deadlock issues in
WaitForOlderSnapshots? The attached is an isolation test able to
reproduce the deadlock within WaitForOlderSnapshots() with two
parallel REINDEX CONCURRENTLY. I'd like to think that the best way to
do that would be to track in vacuumFlags the backends running a
REINDEX and just exclude them from GetCurrentVirtualXIDs() because
we don't actually care about missing index entries in this case like
VACUUM. But it looks also to me that is issue is broader and goes
down to utility commands which can take a lock on a table which cannot
be run in transaction blocks, hence code paths used by CREATE INDEX
CONCURRENTLY and DROP INDEX CONCURRENTLY could also cause a similar
deadlock, no?
--
Michael
Attachments:
reindex-deadlock.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/isolation/expected/reindex-concurrently-ddl.out b/src/test/isolation/expected/reindex-concurrently-ddl.out
new file mode 100644
index 0000000000..14ac712ef4
--- /dev/null
+++ b/src/test/isolation/expected/reindex-concurrently-ddl.out
@@ -0,0 +1,41 @@
+Parsed test spec with 3 sessions
+
+starting permutation: beg1 lock1 index2 index3 end1
+step beg1: BEGIN;
+step lock1: LOCK reind_con_tab IN ROW EXCLUSIVE MODE;
+step index2: REINDEX INDEX CONCURRENTLY reind_con_tab_pkey; <waiting ...>
+step index3: REINDEX INDEX CONCURRENTLY reind_con_tab_pkey; <waiting ...>
+step end1: COMMIT;
+step index2: <... completed>
+step index3: <... completed>
+ERROR: deadlock detected
+
+starting permutation: beg1 lock1 index2 table3 end1
+step beg1: BEGIN;
+step lock1: LOCK reind_con_tab IN ROW EXCLUSIVE MODE;
+step index2: REINDEX INDEX CONCURRENTLY reind_con_tab_pkey; <waiting ...>
+step table3: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step end1: COMMIT;
+step index2: <... completed>
+step table3: <... completed>
+ERROR: deadlock detected
+
+starting permutation: beg1 lock1 table2 index3 end1
+step beg1: BEGIN;
+step lock1: LOCK reind_con_tab IN ROW EXCLUSIVE MODE;
+step table2: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step index3: REINDEX INDEX CONCURRENTLY reind_con_tab_pkey; <waiting ...>
+step end1: COMMIT;
+step table2: <... completed>
+step index3: <... completed>
+ERROR: deadlock detected
+
+starting permutation: beg1 lock1 table2 table3 end1
+step beg1: BEGIN;
+step lock1: LOCK reind_con_tab IN ROW EXCLUSIVE MODE;
+step table2: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step table3: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step end1: COMMIT;
+step table2: <... completed>
+step table3: <... completed>
+ERROR: deadlock detected
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 11cd24fc98..bb5648aa74 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -45,6 +45,7 @@ test: lock-committed-update
test: lock-committed-keyupdate
test: update-locked-tuple
test: reindex-concurrently
+test: reindex-concurrently-ddl
test: propagate-lock-delete
test: tuplelock-conflict
test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-concurrently-ddl.spec b/src/test/isolation/specs/reindex-concurrently-ddl.spec
new file mode 100644
index 0000000000..d2c675b9ff
--- /dev/null
+++ b/src/test/isolation/specs/reindex-concurrently-ddl.spec
@@ -0,0 +1,37 @@
+# REINDEX CONCURRENTLY with table locks
+#
+# Ensure that REINDEX CONCURRENTLY works correctly and serially with
+# locks taken on the table worked on.
+
+setup
+{
+ CREATE TABLE reind_con_tab(id serial primary key, data text);
+ INSERT INTO reind_con_tab(data) VALUES ('aa');
+ INSERT INTO reind_con_tab(data) VALUES ('aaa');
+ INSERT INTO reind_con_tab(data) VALUES ('aaaa');
+ INSERT INTO reind_con_tab(data) VALUES ('aaaaa');
+}
+
+teardown
+{
+ DROP TABLE reind_con_tab;
+}
+
+session "s1"
+step "beg1" { BEGIN; }
+step "lock1" { LOCK reind_con_tab IN ROW EXCLUSIVE MODE; }
+step "end1" { COMMIT; }
+
+session "s2"
+step "index2" { REINDEX INDEX CONCURRENTLY reind_con_tab_pkey; }
+step "table2" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+
+session "s3"
+step "index3" { REINDEX INDEX CONCURRENTLY reind_con_tab_pkey; }
+step "table3" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+
+# Each REINDEX blocks and should complete serially
+permutation "beg1" "lock1" "index2" "index3" "end1"
+permutation "beg1" "lock1" "index2" "table3" "end1"
+permutation "beg1" "lock1" "table2" "index3" "end1"
+permutation "beg1" "lock1" "table2" "table3" "end1"
On Tue, May 07, 2019 at 12:07:56PM +0900, Michael Paquier wrote:
Now, what do we do about the potential deadlock issues in
WaitForOlderSnapshots? The attached is an isolation test able to
reproduce the deadlock within WaitForOlderSnapshots() with two
parallel REINDEX CONCURRENTLY. I'd like to think that the best way to
do that would be to track in vacuumFlags the backends running a
REINDEX and just exclude them from GetCurrentVirtualXIDs() because
we don't actually care about missing index entries in this case like
VACUUM. But it looks also to me that is issue is broader and goes
down to utility commands which can take a lock on a table which cannot
be run in transaction blocks, hence code paths used by CREATE INDEX
CONCURRENTLY and DROP INDEX CONCURRENTLY could also cause a similar
deadlock, no?
More to the point, one can just do that without REINDEX:
- session 1:
create table aa (a int);
begin;
lock aa in row exclusive mode;
- session 2:
create index concurrently aai on aa(a); --blocks
- session 3:
create index concurrently aai2 on aa(a); --blocks
- session 1:
commit;
Then session 2 deadlocks while session 3 finishes correctly. I don't
know if this is a class of problems we'd want to address for v12, but
if we do then CIC (and DROP INDEX CONCURRENTLY?) could benefit from
it.
--
Michael
Hi,
On 2019-05-07 12:25:43 +0900, Michael Paquier wrote:
On Tue, May 07, 2019 at 12:07:56PM +0900, Michael Paquier wrote:
Now, what do we do about the potential deadlock issues in
WaitForOlderSnapshots? The attached is an isolation test able to
reproduce the deadlock within WaitForOlderSnapshots() with two
parallel REINDEX CONCURRENTLY. I'd like to think that the best way to
do that would be to track in vacuumFlags the backends running a
REINDEX and just exclude them from GetCurrentVirtualXIDs() because
we don't actually care about missing index entries in this case like
VACUUM. But it looks also to me that is issue is broader and goes
down to utility commands which can take a lock on a table which cannot
be run in transaction blocks, hence code paths used by CREATE INDEX
CONCURRENTLY and DROP INDEX CONCURRENTLY could also cause a similar
deadlock, no?More to the point, one can just do that without REINDEX:
- session 1:
create table aa (a int);
begin;
lock aa in row exclusive mode;
- session 2:
create index concurrently aai on aa(a); --blocks
- session 3:
create index concurrently aai2 on aa(a); --blocks
- session 1:
commit;Then session 2 deadlocks while session 3 finishes correctly. I don't
know if this is a class of problems we'd want to address for v12, but
if we do then CIC (and DROP INDEX CONCURRENTLY?) could benefit from
it.
This seems like a pre-existing issue to me. We probably should improve
that, but I don't think it has to be tied to 12.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2019-05-07 12:25:43 +0900, Michael Paquier wrote:
Then session 2 deadlocks while session 3 finishes correctly. I don't
know if this is a class of problems we'd want to address for v12, but
if we do then CIC (and DROP INDEX CONCURRENTLY?) could benefit from
it.
This seems like a pre-existing issue to me. We probably should improve
that, but I don't think it has to be tied to 12.
Yeah. CREATE INDEX CONCURRENTLY has always had a deadlock hazard,
so it's hardly surprising that REINDEX CONCURRENTLY does too.
I don't think that fixing that is in-scope for v12, even if we had
an idea how to do it, which we don't.
We do need to fix the wrong-lock-level problem of course, but
that seems straightforward.
regards, tom lane
On Tue, May 07, 2019 at 06:45:36PM -0400, Tom Lane wrote:
Yeah. CREATE INDEX CONCURRENTLY has always had a deadlock hazard,
so it's hardly surprising that REINDEX CONCURRENTLY does too.
I don't think that fixing that is in-scope for v12, even if we had
an idea how to do it, which we don't.
The most straight-forward approach I can think of would be to
determine if non-transactional commands taking a lock on a table can
be safely skipped or not when checking for older snapshots than the
minimum where the index is marked as valid. That's quite complex to
target v12, so I agree to keep it out of the stability work.
We do need to fix the wrong-lock-level problem of course, but
that seems straightforward.
Sure.
--
Michael
On 2019-05-07 05:07, Michael Paquier wrote:
On Sat, May 04, 2019 at 09:59:20PM +0900, Michael Paquier wrote:
The result should be no deadlocks happening in the two sessions
running the reindex. I can see the deadlock easily with three psql
sessions, running manually the queries.+ * If the OID isn't valid, it means the index as concurrently dropped, + * which is not a problem for us; just return normally. Typo here s/as/is/.I have looked closer at the patch and the change proposed looks good
to me.
committed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services