Hello, At Mon, 26 Jun 2017 18:16:42 +0900, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote in <7f5fe522-f328-3c40-565f-5e1ce3745...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
