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].
> 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 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).
Thanks,
Amit
[1] https://www.postgresql.org/message-id/565EB1F2.7000201%40lab.ntt.co.jp