Re: Crashes on a partition whose concurrent detach never finished

On Mon, Sep 7, 2026 at 3:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Sep 4, 2026 at 11:45 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> >
> > Thank You Mikhail for the patches.
> >
> > I would like to discuss the design and the intent of patch001 alone.
> >
> > 1. My initial thought was whether get_partition_ancestors() itself
> > should explicitly handle partitions undergoing concurrent detach,
> > similar to get_partition_parent(). This would let all callers
> > recognize the detaching state and handle it appropriately, rather than
> > discovering a NIL ancestor list only after hitting an Assert or crash.
> > Have you considered it this way?
> >
>
> Yes, that is another way of dealing with the issue but it will be
> better to fix the issue without changing the interface. Alvaro also
> mentioned the same [1]. We can discuss changing the interface as a
> separate HEAD-only patch.
>
> > 2. I am also unclear on the intent. When a partition is undergoing
> > concurrent detach, do we ultimately want to publish its changes or
> > not?
> >
>
> It depends. For example, if it is ALL TABLES publication then the
> in-process detach partition should be replicated as an individual
> table.

Yes, that's what I thought initially, but the test case validates
otherwise. See the test in patch-001: the publication is for ALL
TABLES, but the partition being detached is not replicated.

I think this patch focuses on fixing the crash without clarifying the
intent for various cases: whether to publish or not publish the
detached partition.

>
> > ~~
> >
> > I am slightly confused by the code comments and the test case behavior:
> >
> > a)
> > In get_rel_sync_entry(), the comment states that the partition is
> > handled like a standalone table once its ancestors report NIL. If it
> > is treated as a normal table, what prevents it from being published
> > under a FOR ALL TABLES publication? Specifically, if
> > get_rel_sync_entry() allows it, why does the test case comment note
> > "the part1 change is not replicated"? Which exact code part filters
> > out or blocks this partition from being streamed in that state?
> >
>
> Unless I am missing something, the comment needs adjustment.
>
> > b)
> > Inconsistency with is_table_publishable_in_publication(): We updated
> > get_rel_sync_entry() to handle a NULL ancestor list, but
> > is_table_publishable_in_publication() remains unchanged. Won't
> > is_table_publishable_in_publication() return false for this partition
> > during a concurrent detach for a pub created for 'FOR TBALE ROOT'? If
> > so, aren't get_rel_sync_entry() and
> > is_table_publishable_in_publication() out of sync, and what practical
> > side-effects does this discrepancy cause?
> >
>
> I have tried by reproducing it with a test but it appears to be an
> issue that should be fixed as well.
>
> [1] - https://www.postgresql.org/message-id/aoCXNrzbb-yryPg9%40alvherre.pgsql
>
> --
> With Regards,
> Amit Kapila.