Re: [HACKERS] Race between SELECT and ALTER TABLE NO INHERIT

Kyotaro HORIGUCHI <horiguchi.kyotaro@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;
             }