| From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
|---|---|
| To: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
| Cc: | Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, vignesh C <vignesh21(at)gmail(dot)com>, ashutosh(dot)bapat(dot)oss(at)gmail(dot)com |
| Subject: | Re: Crashes on a partition whose concurrent detach never finished |
| Date: | 2026-09-07 10:26:24 |
| Message-ID: | CAA4eK1Ly8gdwBRGM+bfnX8X_HqTwGQG__FbUHXjxNTS5tF8A=Q@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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.
> ~~
>
> 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.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Zsolt Parragi | 2026-09-07 10:28:40 | Re: SSI: ON CONFLICT DO SELECT takes no predicate lock on the returned row |
| Previous Message | Etsuro Fujita | 2026-09-07 10:21:04 | Re: Further cleanup related to statistics import support in postgres_fdw |