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

Hello,

At Mon, 26 Jun 2017 18:16:42 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<7f5fe522-f328-3c40-565f-5e1ce37455d1@lab.ntt.co.jp>
> Hello,
> 
> On 2017/06/26 17:46, Kyotaro HORIGUCHI wrote:
> > Hello.
> > 
> > I had a case of unexpected error caused by ALTER TABLE NO
> > INHERIT.
> > 
> > =# CREATE TABLE p (a int);
> > =# CREATE TABLE c1 () INHERITS (p);
> > 
> > session A=# BEGIN;
> > session A=# ALTER TABLE c1 NO INHERIT p;
> > 
> > session B=# EXPLAIN ANALYZE SELECT * FROM p;
> > (blocked)
> > 
> > session A=# COMMIT;
> > 
> > session B: ERROR:  could not find inherited attribute "a" of relation "c1"
> > 
> > This happens at least back to 9.1 to master and doesn't seem to
> > be a designed behavior.
> 
> I recall I had proposed a fix for the same thing some time ago [1].

Wow. About 1.5 years ago and stuck? I have a concrete case where
this harms  so I'd like to fix that this time. How can we move on?

> > The cause is that NO INHERIT doesn't take an exlusive lock on the
> > parent. This allows expand_inherited_rtentry to add the child
> > relation into appendrel after removal from the inheritance but
> > still exists.
> 
> Right.
> 
> > I see two ways to fix this.
> > 
> > The first patch adds a recheck of inheritance relationship if the
> > corresponding attribute is missing in the child in
> > make_inh_translation_list(). The recheck is a bit complex but it
> > is not performed unless the sequence above is happen. It checks
> > duplication of relid (or cycles in inheritance) following
> > find_all_inheritors (but doing a bit different) but I'm not sure
> > it is really useful.
> 
> I had proposed adding a syscache on pg_inherits(inhrelid, inhparent), but
> I guess your hash table based solution will do the job as far as
> performance of this check is concerned, although I haven't checked the
> code closely.

The hash table is not crucial in the patch. Substantially it just
recurs down looking up pg_inherits for the child. I considered
the new index but abandoned because I thought that such case
won't occur so frequently.

> > The second patch lets ALTER TABLE NO INHERIT to acquire locks on
> > the parent first.
> > 
> > Since the latter has a larger impact on the current behavior and
> > we already treat "DROP TABLE child" case in the similar way, I
> > suppose that the first approach would be preferable.
> 
> That makes sense.
> 
> BTW, in the partitioned table case, the parent is always locked first
> using an AccessExclusiveLock.  There are other considerations in that case
> such as needing to recreate the partition descriptor upon termination of
> inheritance (both the DETACH PARTITION and also DROP TABLE child cases).

Apart from the degree of concurrency, if we keep parent->children
order of locking, such recreation does not seem to be
needed. Maybe I'm missing something.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center