Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> writes: > [ 0002-Lock-parent-on-ALTER-TABLE-NO-INHERIT.patch ]
I don't especially like any of the patches proposed on this thread. The one with rechecking inheritance seems expensive, duplicative, and complicated. The approach of taking a lock on the parent will create deadlocks in usage patterns that did not encounter such deadlocks before --- in particular, in the scenarios in which this behavior matters at all, the ALTER will deadlock against ordinary queries that lock the parent before the child. So I can't believe that anyone who's hitting the problem in the field will think the extra lock is an improvement. I think that there might be a much simpler solution to this, which is to just remove make_inh_translation_list's tests of attinhcount, as per attached. Those are really pretty redundant: since we are matching by column name, the unique index on pg_attribute already guarantees there is at most one matching column. I have a feeling that those tests are my fault and I put them in on the theory that they could save a few strcmp executions --- but if they're causing problems, we can certainly drop them. In any case they'd only save something meaningful if most of the child's columns are non-inherited, which doesn't seem like the way to bet. In this way, if the child gets past the initial check on whether it's been dropped, we'll still succeed in matching its columns, even if they are no longer marked as inherited. For me, this works fine with either the ALTER NO INHERIT case (c1 does get scanned, with no error) or the DROP TABLE case (c1 doesn't get scanned). Now, you can break it if you really try hard: make the concurrent transaction do both ALTER NO INHERIT and then DROP COLUMN. But that's not a scenario that's been complained of, so I don't want to add a ton of mechanism to fix it. regards, tom lane
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 95557d7..5c4d113 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -1832,7 +1832,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation, */ if (old_attno < newnatts && (att = TupleDescAttr(new_tupdesc, old_attno)) != NULL && - !att->attisdropped && att->attinhcount != 0 && + !att->attisdropped && strcmp(attname, NameStr(att->attname)) == 0) new_attno = old_attno; else @@ -1840,7 +1840,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation, for (new_attno = 0; new_attno < newnatts; new_attno++) { att = TupleDescAttr(new_tupdesc, new_attno); - if (!att->attisdropped && att->attinhcount != 0 && + if (!att->attisdropped && strcmp(attname, NameStr(att->attname)) == 0) break; }