From c2a7044a5128a17256c771b545a534c7dccb7736 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 12 Jan 2018 15:46:37 -0500 Subject: [PATCH] Avoid unnecessary failure in SELECT concurrent with ALTER NO INHERIT. If a query against an inheritance tree runs concurrently with an ALTER TABLE that's disinheriting one of the tree members, it's possible to get a "could not find inherited attribute" error because after obtaining lock on the removed member, make_inh_translation_list sees that its columns have attinhcount=0 and decides they aren't the columns it's looking for. An ideal fix, perhaps, would avoid including such a just-removed member table in the query at all; but there seems no way to accomplish that without adding expensive catalog rechecks or creating a likelihood of deadlocks. Instead, let's just drop the check on attinhcount. In this way, a query that's included a just-disinherited child will still succeed, which is not a completely unreasonable behavior. This problem has existed for a long time, so back-patch to all supported branches. Also add an isolation test verifying related behaviors. Patch by me; the new isolation test is based on Kyotaro Horiguchi's work. Discussion: https://postgr.es/m/20170626.174612.23936762.horiguchi.kyotaro@lab.ntt.co.jp --- src/backend/optimizer/prep/prepunion.c | 4 +- src/test/isolation/expected/alter-table-4.out | 57 +++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/alter-table-4.spec | 37 ++++++++++++ 4 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 src/test/isolation/expected/alter-table-4.out create mode 100644 src/test/isolation/specs/alter-table-4.spec diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index b441f7e5ca..266ee4db5e 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1638,7 +1638,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation, */ if (old_attno < newnatts && (att = new_tupdesc->attrs[old_attno]) != NULL && - !att->attisdropped && att->attinhcount != 0 && + !att->attisdropped && strcmp(attname, NameStr(att->attname)) == 0) new_attno = old_attno; else @@ -1646,7 +1646,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation, for (new_attno = 0; new_attno < newnatts; new_attno++) { att = new_tupdesc->attrs[new_attno]; - if (!att->attisdropped && att->attinhcount != 0 && + if (!att->attisdropped && strcmp(attname, NameStr(att->attname)) == 0) break; } diff --git a/src/test/isolation/expected/alter-table-4.out b/src/test/isolation/expected/alter-table-4.out new file mode 100644 index 0000000000..d2dac0be09 --- /dev/null +++ b/src/test/isolation/expected/alter-table-4.out @@ -0,0 +1,57 @@ +Parsed test spec with 2 sessions + +starting permutation: s1b s1delc1 s2sel s1c s2sel +step s1b: BEGIN; +step s1delc1: ALTER TABLE c1 NO INHERIT p; +step s2sel: SELECT SUM(a) FROM p; +step s1c: COMMIT; +step s2sel: <... completed> +sum + +11 +step s2sel: SELECT SUM(a) FROM p; +sum + +1 + +starting permutation: s1b s1delc1 s1addc2 s2sel s1c s2sel +step s1b: BEGIN; +step s1delc1: ALTER TABLE c1 NO INHERIT p; +step s1addc2: ALTER TABLE c2 INHERIT p; +step s2sel: SELECT SUM(a) FROM p; +step s1c: COMMIT; +step s2sel: <... completed> +sum + +11 +step s2sel: SELECT SUM(a) FROM p; +sum + +101 + +starting permutation: s1b s1dropc1 s2sel s1c s2sel +step s1b: BEGIN; +step s1dropc1: DROP TABLE c1; +step s2sel: SELECT SUM(a) FROM p; +step s1c: COMMIT; +step s2sel: <... completed> +sum + +1 +step s2sel: SELECT SUM(a) FROM p; +sum + +1 + +starting permutation: s1b s1delc1 s1modc1a s2sel s1c s2sel +step s1b: BEGIN; +step s1delc1: ALTER TABLE c1 NO INHERIT p; +step s1modc1a: ALTER TABLE c1 ALTER COLUMN a TYPE float; +step s2sel: SELECT SUM(a) FROM p; +step s1c: COMMIT; +step s2sel: <... completed> +error in steps s1c s2sel: ERROR: attribute "a" of relation "c1" does not match parent's type +step s2sel: SELECT SUM(a) FROM p; +sum + +1 diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 413cf028f1..254adaff42 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -56,6 +56,7 @@ test: multiple-cic test: alter-table-1 test: alter-table-2 test: alter-table-3 +test: alter-table-4 test: create-trigger test: async-notify test: vacuum-reltuples diff --git a/src/test/isolation/specs/alter-table-4.spec b/src/test/isolation/specs/alter-table-4.spec new file mode 100644 index 0000000000..a9c1a93723 --- /dev/null +++ b/src/test/isolation/specs/alter-table-4.spec @@ -0,0 +1,37 @@ +# ALTER TABLE - Add and remove inheritance with concurrent reads + +setup +{ + CREATE TABLE p (a integer); + INSERT INTO p VALUES(1); + CREATE TABLE c1 () INHERITS (p); + INSERT INTO c1 VALUES(10); + CREATE TABLE c2 (a integer); + INSERT INTO c2 VALUES(100); +} + +teardown +{ + DROP TABLE IF EXISTS c1, c2, p; +} + +session "s1" +step "s1b" { BEGIN; } +step "s1delc1" { ALTER TABLE c1 NO INHERIT p; } +step "s1modc1a" { ALTER TABLE c1 ALTER COLUMN a TYPE float; } +step "s1addc2" { ALTER TABLE c2 INHERIT p; } +step "s1dropc1" { DROP TABLE c1; } +step "s1c" { COMMIT; } + +session "s2" +step "s2sel" { SELECT SUM(a) FROM p; } + +# NO INHERIT will not be visible to concurrent select, +# since we identify children before locking them +permutation "s1b" "s1delc1" "s2sel" "s1c" "s2sel" +# adding inheritance likewise is not seen if s1 commits after s2 locks p +permutation "s1b" "s1delc1" "s1addc2" "s2sel" "s1c" "s2sel" +# but we do cope with DROP on a child table +permutation "s1b" "s1dropc1" "s2sel" "s1c" "s2sel" +# this case currently results in an error; doesn't seem worth preventing +permutation "s1b" "s1delc1" "s1modc1a" "s2sel" "s1c" "s2sel" -- 2.40.0