Re: BUG #17339: Assert failed on attempt to detach a sequence concurrently

At Tue, 21 Dec 2021 09:30:42 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Dec 20, 2021 at 09:57:47PM +0100, Peter Eisentraut wrote:
> > On 20.12.21 13:38, Michael Paquier wrote:
> >> Attached is a patch, with some regression tests.
> > 
> > Is it possible for child tables in partitioned tables to have different
> > ownerships?  Then this change would introduce a new failure mode.
> 
> Hmm.  Yes, you are right here.  It is possible to change the ownership
> of a partition after it gets attached, so this could cause a
> regression.  I recalled that this was not possible, so my memories
> were wrong.
> 
> At the end, perhaps we should just remove the assertion that assumes
> which relkind is right for the partition, as of:
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -15013,8 +15013,6 @@ MarkInheritDetached(Relation child_rel,
> Relation parent_rel)
>     HeapTuple   inheritsTuple;
>         bool        found = false;
> 
> -   Assert(child_rel->rd_rel->relkind == RELKIND_RELATION ||
> -          child_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
>     Assert(parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
> 
> This would also fail when attempting to detach a foreign table, as
> well, and these are legal relkinds in a partition tree.  Once we do
> that, we fall down into the same failure as for the non-concurrent
> mode in RemoveInheritance(), telling that the relation is not a member
> of the partition tree.

I agree to the discussion above thus it seems to me right that we just
remove the assertion.  We never have a partition of un-attachable
relkind so such kind of relations are surely rejected just after. The
assertion on "== RELKIND_PARTITIONED_TABLE" is still valid since we
don't reach there for the case of the traditional inheritance.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center