From 8c6ab3b0398fca106f87044abd46c0ce5f7a538b Mon Sep 17 00:00:00 2001 From: gayyappan Date: Thu, 20 Dec 2018 18:55:34 -0500 Subject: [PATCH] Fix for deadlock between select and drop_chunks Acquire foreign key related table locks prior to dropping chunks. This helps avoid deadlocks in scenarios where select and drop chunks are trying to acquire locks on the same set of tables. Changes to isolation test suite to prevent diffs related to displaying hypertable id differences across test suite runs. Fixes #865 --- .travis.yml | 2 +- src/chunk.c | 46 +++++++++++++ .../expected/deadlock_dropchunks_select.out | 68 +++++++++++++++++++ test/isolation/expected/isolation_nop.out | 4 +- .../expected/read_committed_insert.out | 24 +++---- .../expected/read_uncommitted_insert.out | 24 +++---- .../expected/repeatable_read_insert.out | 24 +++---- .../specs/deadlock_dropchunks_select.spec | 27 ++++++++ test/isolation/specs/isolation_nop.spec | 2 +- .../specs/read_committed_insert.spec | 3 +- .../specs/read_uncommitted_insert.spec | 2 +- .../specs/repeatable_read_insert.spec | 2 +- 12 files changed, 185 insertions(+), 43 deletions(-) create mode 100644 test/isolation/expected/deadlock_dropchunks_select.out create mode 100644 test/isolation/specs/deadlock_dropchunks_select.spec diff --git a/.travis.yml b/.travis.yml index 2d7903d7f..78fa5fe0d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -41,7 +41,7 @@ script: - ci_env=`bash <(curl -s https://codecov.io/env)` - docker exec -it $ci_env pgbuild /bin/bash -c "cd /build/debug && bash <(curl -s https://codecov.io/bash) || echo \"Codecov did not collect coverage reports\" " after_failure: - - docker exec -it pgbuild cat /build/debug/test/regression.diffs /build/debug/test/pgtest/regressions.diffs + - docker exec -it pgbuild cat /build/debug/test/regression.diffs /build/debug/test/pgtest/regressions.diffs /build/debug/test/isolation/regression.diffs after_script: - docker rm -f pgbuild diff --git a/src/chunk.c b/src/chunk.c index a8b558a97..42c871081 100644 --- a/src/chunk.c +++ b/src/chunk.c @@ -694,6 +694,7 @@ chunk_tuple_found(TupleInfo *ti, void *arg) return SCAN_DONE; } + /* Fill in a chunk stub. The stub data structure needs the chunk ID and constraints set. * The rest of the fields will be filled in from the table data. */ static Chunk * @@ -1890,7 +1891,52 @@ ts_chunk_drop_chunks(PG_FUNCTION_ARGS) foreach(lc, ht_oids) { Oid table_relid = lfirst_oid(lc); + List *fk_relids = NIL; + ListCell *lf; + /* get foreign key tables associated with the hypertable */ + { + List *cachedfkeys = NIL; + ListCell *lf; + Relation table_rel; + + table_rel = heap_open(table_relid, AccessShareLock); + + /* + * this list is from the relcache and can disappear with a cache + * flush, so no further catalog access till we save the fk relids + */ + cachedfkeys = RelationGetFKeyList(table_rel); + foreach(lf, cachedfkeys) + { + ForeignKeyCacheInfo *cachedfk = (ForeignKeyCacheInfo *) lfirst(lf); + + /* + * conrelid should always be that of the table we're + * considering + */ + Assert(cachedfk->conrelid == RelationGetRelid(table_rel)); + fk_relids = lappend_oid(fk_relids, cachedfk->confrelid); + } + heap_close(table_rel, AccessShareLock); + } + + /* + * We have a FK between hypertable H and PAR. Hypertable H has number + * of chunks C1, C2, etc. When we execute "drop table C", PG acquires + * locks on C and PAR. If we have a query as "select * from + * hypertable", this acquires a lock on C and PAR as well. But the + * order of the locks is not the same and results in deadlocks. - + * github issue 865 We hope to alleviate the problem by aquiring a + * lock on PAR before executing the drop table stmt. This is not + * fool-proof as we could have multiple fkrelids and the order of lock + * acquistion for these could differ as well. Do not unlock - let the + * transaction semantics take care of it. + */ + foreach(lf, fk_relids) + { + LockRelationOid(lfirst_oid(lf), AccessExclusiveLock); + } do_drop_chunks(table_relid, older_than_datum, newer_than_datum, older_than_type, newer_than_type, cascade); } diff --git a/test/isolation/expected/deadlock_dropchunks_select.out b/test/isolation/expected/deadlock_dropchunks_select.out new file mode 100644 index 000000000..1146df66f --- /dev/null +++ b/test/isolation/expected/deadlock_dropchunks_select.out @@ -0,0 +1,68 @@ +Parsed test spec with 2 sessions + +starting permutation: s1a s1b s2a s2b +step s1a: select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); +drop_chunks + + +step s1b: COMMIT; +step s2a: SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; +typ loc mtim + +step s2b: COMMIT; + +starting permutation: s1a s2a s1b s2b +step s1a: select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); +drop_chunks + + +step s2a: SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; +step s1b: COMMIT; +step s2a: <... completed> +typ loc mtim + +step s2b: COMMIT; + +starting permutation: s1a s2a s2b s1b +step s1a: select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); +drop_chunks + + +step s2a: SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; +step s2a: <... completed> +ERROR: canceling statement due to user request +step s2b: COMMIT; +step s1b: COMMIT; + +starting permutation: s2a s1a s1b s2b +step s2a: SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; +typ loc mtim + +step s1a: select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); +step s1a: <... completed> +ERROR: canceling statement due to user request +step s1b: COMMIT; +step s2b: COMMIT; + +starting permutation: s2a s1a s2b s1b +step s2a: SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; +typ loc mtim + +step s1a: select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); +step s2b: COMMIT; +step s1a: <... completed> +drop_chunks + + +step s1b: COMMIT; + +starting permutation: s2a s2b s1a s1b +step s2a: SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; +typ loc mtim + +step s2b: COMMIT; +step s1a: select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); +drop_chunks + + +step s1b: COMMIT; diff --git a/test/isolation/expected/isolation_nop.out b/test/isolation/expected/isolation_nop.out index e97b9e1d4..48cc2c1f4 100644 --- a/test/isolation/expected/isolation_nop.out +++ b/test/isolation/expected/isolation_nop.out @@ -1,9 +1,9 @@ Parsed test spec with 1 sessions starting permutation: s1a -create_hypertable +table_name -(1,public,ts_cluster_test,t) +ts_cluster_test step s1a: SELECT pg_sleep(0); pg_sleep diff --git a/test/isolation/expected/read_committed_insert.out b/test/isolation/expected/read_committed_insert.out index 038f790c6..e7f04e83a 100644 --- a/test/isolation/expected/read_committed_insert.out +++ b/test/isolation/expected/read_committed_insert.out @@ -1,18 +1,18 @@ Parsed test spec with 2 sessions starting permutation: s1a s1c s2a s2b -create_hypertable +table_name -(2,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s1c: COMMIT; step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2b: COMMIT; starting permutation: s1a s2a s1c s2b -create_hypertable +table_name -(3,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1c: COMMIT; @@ -20,9 +20,9 @@ step s2a: <... completed> step s2b: COMMIT; starting permutation: s1a s2a s2b s1c -create_hypertable +table_name -(4,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2a: <... completed> @@ -31,9 +31,9 @@ step s2b: COMMIT; step s1c: COMMIT; starting permutation: s2a s1a s1c s2b -create_hypertable +table_name -(5,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s1a: <... completed> @@ -42,9 +42,9 @@ step s1c: COMMIT; step s2b: COMMIT; starting permutation: s2a s1a s2b s1c -create_hypertable +table_name -(6,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2b: COMMIT; @@ -52,9 +52,9 @@ step s1a: <... completed> step s1c: COMMIT; starting permutation: s2a s2b s1a s1c -create_hypertable +table_name -(7,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2b: COMMIT; step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); diff --git a/test/isolation/expected/read_uncommitted_insert.out b/test/isolation/expected/read_uncommitted_insert.out index 95fb7cbd0..e7f04e83a 100644 --- a/test/isolation/expected/read_uncommitted_insert.out +++ b/test/isolation/expected/read_uncommitted_insert.out @@ -1,18 +1,18 @@ Parsed test spec with 2 sessions starting permutation: s1a s1c s2a s2b -create_hypertable +table_name -(8,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s1c: COMMIT; step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2b: COMMIT; starting permutation: s1a s2a s1c s2b -create_hypertable +table_name -(9,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1c: COMMIT; @@ -20,9 +20,9 @@ step s2a: <... completed> step s2b: COMMIT; starting permutation: s1a s2a s2b s1c -create_hypertable +table_name -(10,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2a: <... completed> @@ -31,9 +31,9 @@ step s2b: COMMIT; step s1c: COMMIT; starting permutation: s2a s1a s1c s2b -create_hypertable +table_name -(11,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s1a: <... completed> @@ -42,9 +42,9 @@ step s1c: COMMIT; step s2b: COMMIT; starting permutation: s2a s1a s2b s1c -create_hypertable +table_name -(12,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2b: COMMIT; @@ -52,9 +52,9 @@ step s1a: <... completed> step s1c: COMMIT; starting permutation: s2a s2b s1a s1c -create_hypertable +table_name -(13,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2b: COMMIT; step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); diff --git a/test/isolation/expected/repeatable_read_insert.out b/test/isolation/expected/repeatable_read_insert.out index 170892884..e7f04e83a 100644 --- a/test/isolation/expected/repeatable_read_insert.out +++ b/test/isolation/expected/repeatable_read_insert.out @@ -1,18 +1,18 @@ Parsed test spec with 2 sessions starting permutation: s1a s1c s2a s2b -create_hypertable +table_name -(14,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s1c: COMMIT; step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2b: COMMIT; starting permutation: s1a s2a s1c s2b -create_hypertable +table_name -(15,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1c: COMMIT; @@ -20,9 +20,9 @@ step s2a: <... completed> step s2b: COMMIT; starting permutation: s1a s2a s2b s1c -create_hypertable +table_name -(16,public,ts_cluster_test,t) +ts_cluster_test step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2a: <... completed> @@ -31,9 +31,9 @@ step s2b: COMMIT; step s1c: COMMIT; starting permutation: s2a s1a s1c s2b -create_hypertable +table_name -(17,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s1a: <... completed> @@ -42,9 +42,9 @@ step s1c: COMMIT; step s2b: COMMIT; starting permutation: s2a s1a s2b s1c -create_hypertable +table_name -(18,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); step s2b: COMMIT; @@ -52,9 +52,9 @@ step s1a: <... completed> step s1c: COMMIT; starting permutation: s2a s2b s1a s1c -create_hypertable +table_name -(19,public,ts_cluster_test,t) +ts_cluster_test step s2a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090002', 0.72, 1); step s2b: COMMIT; step s1a: INSERT INTO ts_cluster_test VALUES ('2017-01-20T090001', 23.4, 1); diff --git a/test/isolation/specs/deadlock_dropchunks_select.spec b/test/isolation/specs/deadlock_dropchunks_select.spec new file mode 100644 index 000000000..2eeb429d2 --- /dev/null +++ b/test/isolation/specs/deadlock_dropchunks_select.spec @@ -0,0 +1,27 @@ +##github issue 865 deadlock between select and drop chunks + +setup +{ + CREATE TABLE ST ( sid int PRIMARY KEY, typ text) ; + CREATE TABLE SL ( lid int PRIMARY KEY, loc text) ; + CREATE TABLE DT ( sid int REFERENCES ST(sid), lid int REFERENCES SL(lid), mtim timestamp with time zone ) ; + SELECT create_hypertable('DT', 'mtim', chunk_time_interval => interval '1 day'); + INSERT INTO SL VALUES (1, 'LA'); + INSERT INTO ST VALUES (1, 'T1'); + INSERT INTO DT (sid,lid,mtim) + SELECT 1,1, generate_series( '2018-12-01 00:00'::timestamp, '2018-12-31 12:00','1 minute') ; +} + +teardown +{ DROP TABLE DT; DROP table ST; DROP table SL; } + +session "s1" +setup { BEGIN;} +step "s1a" { select drop_chunks( '2018-12-25 00:00'::timestamptz, 'dt'); } +step "s1b" { COMMIT; } + +session "s2" +setup { BEGIN; } +step "s2a" { SELECT typ, loc, mtim FROM DT , SL , ST WHERE SL.lid = DT.lid AND ST.sid = DT.sid AND mtim >= '2018-12-01 03:00:00+00' AND mtim <= '2018-12-01 04:00:00+00' AND typ = 'T1' ; } +step "s2b" { COMMIT; } + diff --git a/test/isolation/specs/isolation_nop.spec b/test/isolation/specs/isolation_nop.spec index 2c3b449e2..2db69f7d7 100644 --- a/test/isolation/specs/isolation_nop.spec +++ b/test/isolation/specs/isolation_nop.spec @@ -1,6 +1,6 @@ setup{ CREATE TABLE ts_cluster_test(time timestamptz, temp float, location int); - SELECT create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); + SELECT table_name from create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); } teardown { diff --git a/test/isolation/specs/read_committed_insert.spec b/test/isolation/specs/read_committed_insert.spec index ebbad0315..ce27c1031 100644 --- a/test/isolation/specs/read_committed_insert.spec +++ b/test/isolation/specs/read_committed_insert.spec @@ -1,7 +1,8 @@ setup { CREATE TABLE ts_cluster_test(time timestamptz, temp float, location int); - SELECT create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); + SELECT table_name from create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); + } teardown { DROP TABLE ts_cluster_test; } diff --git a/test/isolation/specs/read_uncommitted_insert.spec b/test/isolation/specs/read_uncommitted_insert.spec index ff2f3f5e9..8b747253f 100644 --- a/test/isolation/specs/read_uncommitted_insert.spec +++ b/test/isolation/specs/read_uncommitted_insert.spec @@ -1,7 +1,7 @@ setup { CREATE TABLE ts_cluster_test(time timestamptz, temp float, location int); - SELECT create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); + SELECT table_name from create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); } teardown { DROP TABLE ts_cluster_test; } diff --git a/test/isolation/specs/repeatable_read_insert.spec b/test/isolation/specs/repeatable_read_insert.spec index b4b5fd1ab..0af4d6481 100644 --- a/test/isolation/specs/repeatable_read_insert.spec +++ b/test/isolation/specs/repeatable_read_insert.spec @@ -1,7 +1,7 @@ setup { CREATE TABLE ts_cluster_test(time timestamptz, temp float, location int); - SELECT create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); + SELECT table_name from create_hypertable('ts_cluster_test', 'time', chunk_time_interval => interval '1 day'); } teardown { DROP TABLE ts_cluster_test; }