Crashes on a partition whose concurrent detach never finished

On 2026-Aug-15, Mihail Nikalayeu wrote:

I was working on stress-suite and discovered a few crashes caused
mainly by the same issue.

DETACH PARTITION ... CONCURRENTLY marks the partition and commits, and
only then waits. If that wait is interrupted the mark remains.
Only PARTITION ... FINALIZE clears it. Reaching this needs no race,
and the state outlives the command that made it.

In that state pg_class still says relispartition, while
get_partition_ancestors() already reports nothing.

Interesting, thanks for the testing and for the patches. As far as
backpatchable fixes these look okay to me in a quick glance, and unless
something else comes up, I'll try to get them pushed and backpatched
next week.

However, it can be argued (esp. due to the new bug in 19) that the
choices I made for concurrent detach lead to a system that's far too
brittle, and we should improve it -- for example, maybe it makes more
sense to have get_partition_ancestor() take a boolean parameter to
indicate whether to scan past an in-detach partition or not, as
suggested in the comment in 0002, so that the developer has to make a
conscious decision regarding that condition. That would probably lead
to better code and less room for future bugs.

Patches are attached, one for each place; each includes a test that
reproduces the crash.

This is great, thanks.

Please note I have updated my email address. The old one will still
work indefinitely, but I avoid it as much as I am able.

Regards

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them." (Larry Wall)

Does this one deserve a mention on the open items wiki [0]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items?

[0]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

--
nathan

On Sat, Aug 15, 2026 at 10:20 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Aug-15, Mihail Nikalayeu wrote:

I was working on stress-suite and discovered a few crashes caused
mainly by the same issue.

DETACH PARTITION ... CONCURRENTLY marks the partition and commits, and
only then waits. If that wait is interrupted the mark remains.
Only PARTITION ... FINALIZE clears it. Reaching this needs no race,
and the state outlives the command that made it.

In that state pg_class still says relispartition, while
get_partition_ancestors() already reports nothing.

Interesting, thanks for the testing and for the patches. As far as
backpatchable fixes these look okay to me in a quick glance, and unless
something else comes up, I'll try to get them pushed and backpatched
next week.

While testing another feature patch in the recent past (Except table
for tables in a schema), I also came across the same issue in
get_rel_sync_entry(). The issue and testcase are shared at [1]/messages/by-id/CABdArM64ZSat=dB094U8OCBzp1m8_Y85m6T+xPEgpLVJb60_rA@mail.gmail.com. The
root cause is the same as described here.

I tested patch v1-0001 against my reproducer, and it fixes the issue for me.

Overall, the fixes in all three places look good to me. Just a small
nit: patch-001's pgoutput.c requires pgindent run.

[1]: /messages/by-id/CABdArM64ZSat=dB094U8OCBzp1m8_Y85m6T+xPEgpLVJb60_rA@mail.gmail.com

--
Thanks,
Nisha

On Thu, Sep 3, 2026 at 5:25 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Sat, Aug 15, 2026 at 10:20 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Aug-15, Mihail Nikalayeu wrote:

I was working on stress-suite and discovered a few crashes caused
mainly by the same issue.

DETACH PARTITION ... CONCURRENTLY marks the partition and commits, and
only then waits. If that wait is interrupted the mark remains.
Only PARTITION ... FINALIZE clears it. Reaching this needs no race,
and the state outlives the command that made it.

In that state pg_class still says relispartition, while
get_partition_ancestors() already reports nothing.

Interesting, thanks for the testing and for the patches. As far as
backpatchable fixes these look okay to me in a quick glance, and unless
something else comes up, I'll try to get them pushed and backpatched
next week.

While testing another feature patch in the recent past (Except table
for tables in a schema), I also came across the same issue in
get_rel_sync_entry(). The issue and testcase are shared at [1]. The
root cause is the same as described here.

My thread about a get_partition_root function is related to this too [1]/messages/by-id/CAHut+Ptc73UpWSn4teMyMxQoHwGJjRwhWiu=hGJqAj5k2AOXEw@mail.gmail.com.

I tested patch v1-0001 against my reproducer, and it fixes the issue for me.

Overall, the fixes in all three places look good to me. Just a small
nit: patch-001's pgoutput.c requires pgindent run.

[1] /messages/by-id/CABdArM64ZSat=dB094U8OCBzp1m8_Y85m6T+xPEgpLVJb60_rA@mail.gmail.com

======
[1]: /messages/by-id/CAHut+Ptc73UpWSn4teMyMxQoHwGJjRwhWiu=hGJqAj5k2AOXEw@mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Sat, Aug 15, 2026 at 8:54 PM Mihail Nikalayeu
<mihailnikalayeu@gmail.com> wrote:

Hello, everyone! (Added CC to authors of patches that introduced the
issues described below, sorry if it is duplicate for you).

I was working on stress-suite and discovered a few crashes caused
mainly by the same issue.

DETACH PARTITION ... CONCURRENTLY marks the partition and commits, and
only then waits. If that wait is interrupted the mark remains.
Only PARTITION ... FINALIZE clears it. Reaching this needs no race,
and the state outlives the command that made it.

In that state pg_class still says relispartition, while
get_partition_ancestors() already reports nothing.

Three places read the former as meaning the latter is not empty and
ask for its last element, which causes an assert() or NPE:

- get_rel_sync_entry()
- getIdentitySequence()
- RelationBuildPublicationDesc()

The last one is a v19 regression, so I think it may be added to the
open items list.

Patches are attached, one for each place; each includes a test that
reproduces the crash.

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?

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?

~~

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?

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?

thanks
Shveta

On Fri, Sep 4, 2026 at 11:45 AM shveta malik <shveta.malik@gmail.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]/messages/by-id/aoCXNrzbb-yryPg9@alvherre.pgsql. 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]: /messages/by-id/aoCXNrzbb-yryPg9@alvherre.pgsql

--
With Regards,
Amit Kapila.

On Sat, Aug 15, 2026 at 10:20 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Aug-15, Mihail Nikalayeu wrote:

I was working on stress-suite and discovered a few crashes caused
mainly by the same issue.

DETACH PARTITION ... CONCURRENTLY marks the partition and commits, and
only then waits. If that wait is interrupted the mark remains.
Only PARTITION ... FINALIZE clears it. Reaching this needs no race,
and the state outlives the command that made it.

In that state pg_class still says relispartition, while
get_partition_ancestors() already reports nothing.

Interesting, thanks for the testing and for the patches. As far as
backpatchable fixes these look okay to me in a quick glance, and unless
something else comes up, I'll try to get them pushed and backpatched
next week.

Are you planning to look into it? BTW, I see that there is an open
item for PG19 due to v1-0003. So we need to decide what we should do
with this? IIUC, this is a general problem at various places in code
(three pointed by Mihail with test cases and one pointed by Shveta
without a test case). So, we could consider it a bug due to the
behavior of get_partition_ancestors() and deal at all places together
without first trying to close it for PG-19 in which case this should
be moved to the "Older bugs affecting stable branches" section. OTOH,
if you are planning to deal with the reported cases sooner then anyway
PG-19 items will be closed. Will it be helpful if I start dealing with
a few of these, especially 0001 and 0003 provided we agree that API
change could be done as a separate HEAD-only patch?

However, it can be argued (esp. due to the new bug in 19) that the
choices I made for concurrent detach lead to a system that's far too
brittle, and we should improve it -- for example, maybe it makes more
sense to have get_partition_ancestor() take a boolean parameter to
indicate whether to scan past an in-detach partition or not, as
suggested in the comment in 0002, so that the developer has to make a
conscious decision regarding that condition. That would probably lead
to better code and less room for future bugs.

Yes, we can probably consider doing that though for HEAD branch as it
changes the exposed interface.

--
With Regards,
Amit Kapila.

On Mon, Sep 7, 2026 at 3:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Sep 4, 2026 at 11:45 AM shveta malik <shveta.malik@gmail.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.

On Tue, Sep 8, 2026 at 9:22 AM shveta malik <shveta.malik@gmail.com> wrote:

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 tested it further on my machine. Here are the observations:

1)
FOR ALL TABLES pub:
While DETACH PARTITION ... CONCURRENTLY is in its intermediate state
(inhdetachpending = true), changes made directly to the detached
partition continue to be replicated. This is expected IMO, since the
partition is independently included in an ALL TABLES publication,
regardless of its partition relationship with t1.

<The testcase of patch001 shows otherwise. It needs some analysis to
determine why. This means comment in get_rel_sync_entry in patch001 is
correct, the functionality is as per that comment.>

2)
FOR TABLE root pub:
During the intermediate state of DETACH PARTITION ... CONCURRENTLY,
changes made directly to the partition being detached (t1_part1) are
also replicated, even though t1_part1 is not explicitly listed in
pg_publication_tables and pg_partition_root(t1_part1) already returns
t1_part1. This needs some thought regarding how it should behave.
Should t1_part1 not replicated here? Thoughts?

Once the detach completes, changes to t1_part1 are no longer
replicated through the publication of t1, which appears correct.
~~

Please see attached test-results for details. The behaviour is the
same whether I test patch001 alone or all three combined.

Thanks
Shveta

On Tue, Sep 8, 2026 at 10:42 AM shveta malik <shveta.malik@gmail.com> wrote:

I tested it further on my machine. Here are the observations:

2)
FOR TABLE root pub:
During the intermediate state of DETACH PARTITION ... CONCURRENTLY,
changes made directly to the partition being detached (t1_part1) are
also replicated, even though t1_part1 is not explicitly listed in
pg_publication_tables and pg_partition_root(t1_part1) already returns
t1_part1. This needs some thought regarding how it should behave.
Should t1_part1 not replicated here? Thoughts?

Once the detach completes, changes to t1_part1 are no longer
replicated through the publication of t1, which appears correct.

On debugging further, t1_part1 is also replicated (incorrectly IMO) in
Case 2 because pgoutput's RelationSyncCache is not invalidated during
the first phase of DETACH PARTITION CONCURRENTLY.

In ATExecDetachPartition(), MarkInheritDetached() only updates the
partition's pg_inherits row (inhdetachpending = true), without
generating a cache invalidation. Before commit, only the parent's
relcache is explicitly invalidated via CacheInvalidateRelcache(rel) in
ATExecDetachPartition().

As a result, the partition's previously cached (pre-detach)
RelationSyncEntry is reused, so changes to it continue to be
replicated. This is also confirmed by the fact that if no INSERT is
executed on t1_part1 before DETACH CONCURRENTLY blocks, replication
behaves correctly: the partition is not replicated because a new
RelationSyncCache entry is correctly built after inhdetachpending is
set.

Once FINALIZE runs, the issue self-corrects because
DetachPartitionFinalize() updates the partition's pg_class row
(relispartition = false), which invalidates its relcache, and also
DetachPartitionFinalize() explicitly invalidates the parent and
descendants.

Adding cache-invalidation for part-table here solves the problem, but
I am not sure if it could have any other side-effects. Please have a
look.

@@ -21758,6 +21758,8 @@ ATExecDetachPartition(List **wqueue,
AlteredTableInfo *tab, Relation rel,
/* Invalidate relcache entries for the parent -- must
be before close */
CacheInvalidateRelcache(rel);

+               CacheInvalidateRelcache(partRel);
+
                table_close(partRel, NoLock);
                table_close(rel, NoLock);
                tab->rel = NULL;

thanks
Shveta

On Tue, Sep 8, 2026 at 12:35 PM shveta malik <shveta.malik@gmail.com> wrote:

On Tue, Sep 8, 2026 at 10:42 AM shveta malik <shveta.malik@gmail.com> wrote:

I tested it further on my machine. Here are the observations:

2)
FOR TABLE root pub:
During the intermediate state of DETACH PARTITION ... CONCURRENTLY,
changes made directly to the partition being detached (t1_part1) are
also replicated, even though t1_part1 is not explicitly listed in
pg_publication_tables and pg_partition_root(t1_part1) already returns
t1_part1. This needs some thought regarding how it should behave.
Should t1_part1 not replicated here? Thoughts?

Once the detach completes, changes to t1_part1 are no longer
replicated through the publication of t1, which appears correct.

On debugging further, t1_part1 is also replicated (incorrectly IMO) in
Case 2 because pgoutput's RelationSyncCache is not invalidated during
the first phase of DETACH PARTITION CONCURRENTLY.

In ATExecDetachPartition(), MarkInheritDetached() only updates the
partition's pg_inherits row (inhdetachpending = true), without
generating a cache invalidation. Before commit, only the parent's
relcache is explicitly invalidated via CacheInvalidateRelcache(rel) in
ATExecDetachPartition().

As a result, the partition's previously cached (pre-detach)
RelationSyncEntry is reused, so changes to it continue to be
replicated. This is also confirmed by the fact that if no INSERT is
executed on t1_part1 before DETACH CONCURRENTLY blocks, replication
behaves correctly: the partition is not replicated because a new
RelationSyncCache entry is correctly built after inhdetachpending is
set.

Once FINALIZE runs, the issue self-corrects because
DetachPartitionFinalize() updates the partition's pg_class row
(relispartition = false), which invalidates its relcache, and also
DetachPartitionFinalize() explicitly invalidates the parent and
descendants.

Adding cache-invalidation for part-table here solves the problem, but
I am not sure if it could have any other side-effects. Please have a
look.

@@ -21758,6 +21758,8 @@ ATExecDetachPartition(List **wqueue,
AlteredTableInfo *tab, Relation rel,
/* Invalidate relcache entries for the parent -- must
be before close */
CacheInvalidateRelcache(rel);

+               CacheInvalidateRelcache(partRel);
+
table_close(partRel, NoLock);
table_close(rel, NoLock);
tab->rel = NULL;

+1.

Without invalidation, the publication behavior can become nondeterministic.

We also need to invalidate the whole partRel subtree (if exists), as
we do in FINALIZE phase-2. For members of this subtree, the
publication decision should now be based on partRel instead of rel;
otherwise, we can see the same nondeterministic behavior for them.

I also looked at an alternative that avoids this invalidation: follow
the former root until FINALIZE, i.e. publish based on the root's
publication status until the detach completes.

The problem with this approach is that it can cause publisher and
subscriber data to diverge when publish_via_partition_root = true.

For example, suppose the publisher has t1 with partitions p1 and p2,
while the subscriber has only t1. If t1.p1 is in detach pending and we
continue publishing it, the subscriber will apply its changes to t1,
while the publisher has already stopped showing the new rows from p1
in SELECT * FROM t1.

The changes from the detached partition are published using the
parent's identity, so the subscriber applies them to its own parent.
But the publisher's parent no longer includes those rows because the
partition is no longer in its partition descriptor. As a result,
SELECT * FROM parent can return different data on the publisher and
subscriber. This difference can continue to grow for as long as the
partition remains in detach pending.

So this approach does not seem feasible.
~~~

Given that we are treating a detach-pending partition as an individual
table for publication decisions, the changes in patch-003 (pg19
regression) look correct to me. The changes in relcache.c correctly
treat the detach-pending partition as an individual table.

--
Thanks,
Nisha

On Thu, Sep 10, 2026 at 5:21 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Tue, Sep 8, 2026 at 12:35 PM shveta malik <shveta.malik@gmail.com> wrote:

On Tue, Sep 8, 2026 at 10:42 AM shveta malik <shveta.malik@gmail.com> wrote:

I tested it further on my machine. Here are the observations:

2)
FOR TABLE root pub:
During the intermediate state of DETACH PARTITION ... CONCURRENTLY,
changes made directly to the partition being detached (t1_part1) are
also replicated, even though t1_part1 is not explicitly listed in
pg_publication_tables and pg_partition_root(t1_part1) already returns
t1_part1. This needs some thought regarding how it should behave.
Should t1_part1 not replicated here? Thoughts?

Once the detach completes, changes to t1_part1 are no longer
replicated through the publication of t1, which appears correct.

On debugging further, t1_part1 is also replicated (incorrectly IMO) in
Case 2 because pgoutput's RelationSyncCache is not invalidated during
the first phase of DETACH PARTITION CONCURRENTLY.

In ATExecDetachPartition(), MarkInheritDetached() only updates the
partition's pg_inherits row (inhdetachpending = true), without
generating a cache invalidation. Before commit, only the parent's
relcache is explicitly invalidated via CacheInvalidateRelcache(rel) in
ATExecDetachPartition().

As a result, the partition's previously cached (pre-detach)
RelationSyncEntry is reused, so changes to it continue to be
replicated. This is also confirmed by the fact that if no INSERT is
executed on t1_part1 before DETACH CONCURRENTLY blocks, replication
behaves correctly: the partition is not replicated because a new
RelationSyncCache entry is correctly built after inhdetachpending is
set.

Once FINALIZE runs, the issue self-corrects because
DetachPartitionFinalize() updates the partition's pg_class row
(relispartition = false), which invalidates its relcache, and also
DetachPartitionFinalize() explicitly invalidates the parent and
descendants.

Adding cache-invalidation for part-table here solves the problem, but
I am not sure if it could have any other side-effects. Please have a
look.

@@ -21758,6 +21758,8 @@ ATExecDetachPartition(List **wqueue,
AlteredTableInfo *tab, Relation rel,
/* Invalidate relcache entries for the parent -- must
be before close */
CacheInvalidateRelcache(rel);

+               CacheInvalidateRelcache(partRel);
+
table_close(partRel, NoLock);
table_close(rel, NoLock);
tab->rel = NULL;

+1.

Without invalidation, the publication behavior can become nondeterministic.

We also need to invalidate the whole partRel subtree (if exists), as
we do in FINALIZE phase-2. For members of this subtree, the
publication decision should now be based on partRel instead of rel;
otherwise, we can see the same nondeterministic behavior for them.

I agree.

I also looked at an alternative that avoids this invalidation: follow
the former root until FINALIZE, i.e. publish based on the root's
publication status until the detach completes.

The problem with this approach is that it can cause publisher and
subscriber data to diverge when publish_via_partition_root = true.

For example, suppose the publisher has t1 with partitions p1 and p2,
while the subscriber has only t1. If t1.p1 is in detach pending and we
continue publishing it, the subscriber will apply its changes to t1,
while the publisher has already stopped showing the new rows from p1
in SELECT * FROM t1.

The changes from the detached partition are published using the
parent's identity, so the subscriber applies them to its own parent.
But the publisher's parent no longer includes those rows because the
partition is no longer in its partition descriptor. As a result,
SELECT * FROM parent can return different data on the publisher and
subscriber. This difference can continue to grow for as long as the
partition remains in detach pending.

So this approach does not seem feasible.

Your analysis looks reasonable. But I will think a little more about
this tomorrow and will respond.

~~~

Given that we are treating a detach-pending partition as an individual
table for publication decisions, the changes in patch-003 (pg19
regression) look correct to me. The changes in relcache.c correctly
treat the detach-pending partition as an individual table.

I haven't had a chance to reveiw 003 yet as 001 itself was
problematic. Will review it tomorrow.

thanks
Shveta

On Thu, Sep 10, 2026 at 5:37 PM shveta malik <shveta.malik@gmail.com> wrote:

~~~

Given that we are treating a detach-pending partition as an individual
table for publication decisions, the changes in patch-003 (pg19
regression) look correct to me. The changes in relcache.c correctly
treat the detach-pending partition as an individual table.

I haven't had a chance to reveiw 003 yet as 001 itself was
problematic. Will review it tomorrow.

Here is a rebased version of 003 that applies independently on both
HEAD and pg19. This should make reviewing patch-003 separately easier.

--
Thanks,
Nisha

On Thu, Sep 10, 2026 at 7:40 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Thu, Sep 10, 2026 at 5:37 PM shveta malik <shveta.malik@gmail.com> wrote:

~~~

Given that we are treating a detach-pending partition as an individual
table for publication decisions, the changes in patch-003 (pg19
regression) look correct to me. The changes in relcache.c correctly
treat the detach-pending partition as an individual table.

I haven't had a chance to reveiw 003 yet as 001 itself was
problematic. Will review it tomorrow.

Here is a rebased version of 003 that applies independently on both
HEAD and pg19. This should make reviewing patch-003 separately easier.

Okay, the fix looks good. I verified that it fixes the crash on HEAD.
But it would be good to change the fix slightly to be consistent with
001. See the attached changes for reference.

thanks
Shveta

On Fri, Sep 11, 2026 at 10:41 AM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Sep 10, 2026 at 7:40 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Thu, Sep 10, 2026 at 5:37 PM shveta malik <shveta.malik@gmail.com> wrote:

~~~

Given that we are treating a detach-pending partition as an individual
table for publication decisions, the changes in patch-003 (pg19
regression) look correct to me. The changes in relcache.c correctly
treat the detach-pending partition as an individual table.

I haven't had a chance to reveiw 003 yet as 001 itself was
problematic. Will review it tomorrow.

Here is a rebased version of 003 that applies independently on both
HEAD and pg19. This should make reviewing patch-003 separately easier.

Okay, the fix looks good. I verified that it fixes the crash on HEAD.
But it would be good to change the fix slightly to be consistent with
001. See the attached changes for reference.

Thanks for sharing the patch. It looks good to me. I’ve updated the
attached patch accordingly.

One additional improvement:
The error message when trying to add a detach-pending partition to the
EXCEPT clause was misleading:
ERROR: cannot specify relation "public.child" in the publication EXCEPT clause
DETAIL: This operation is not supported for individual partitions.

The relation is no longer a partition in this state; it is in
detach-pending. I’ve updated the error message to make this clearer:

ERROR: cannot specify relation "public.child" in the publication EXCEPT clause
DETAIL: This operation is not supported for partitions with an
incomplete detach.
HINT: Use ALTER TABLE ... DETACH PARTITION ... FINALIZE to complete
the pending detach operation.

--
Thanks,
Nisha

On Fri, Sep 11, 2026 at 11:31 AM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Fri, Sep 11, 2026 at 10:41 AM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Sep 10, 2026 at 7:40 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Thu, Sep 10, 2026 at 5:37 PM shveta malik <shveta.malik@gmail.com> wrote:

~~~

Given that we are treating a detach-pending partition as an individual
table for publication decisions, the changes in patch-003 (pg19
regression) look correct to me. The changes in relcache.c correctly
treat the detach-pending partition as an individual table.

I haven't had a chance to reveiw 003 yet as 001 itself was
problematic. Will review it tomorrow.

Here is a rebased version of 003 that applies independently on both
HEAD and pg19. This should make reviewing patch-003 separately easier.

Okay, the fix looks good. I verified that it fixes the crash on HEAD.
But it would be good to change the fix slightly to be consistent with
001. See the attached changes for reference.

Thanks for sharing the patch. It looks good to me. I’ve updated the
attached patch accordingly.

One additional improvement:
The error message when trying to add a detach-pending partition to the
EXCEPT clause was misleading:
ERROR: cannot specify relation "public.child" in the publication EXCEPT clause
DETAIL: This operation is not supported for individual partitions.

The relation is no longer a partition in this state; it is in
detach-pending. I’ve updated the error message to make this clearer:

ERROR: cannot specify relation "public.child" in the publication EXCEPT clause
DETAIL: This operation is not supported for partitions with an
incomplete detach.
HINT: Use ALTER TABLE ... DETACH PARTITION ... FINALIZE to complete
the pending detach operation.

Thanks, the error looks better now.

The code LGTM. I feel the tests can be reduced to combine update,
delete and except into a single permutation.

permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1c s1updpart
s1delpart s1exceptpart

thanks
Shveta

Hi,

On Friday, September 11, 2026 2:01 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Thanks for sharing the patch. It looks good to me. I’ve updated the attached
patch accordingly.

The fix looks mostly good to me. The only nit is that we could avoid introducing
a new Boolean flag by writing it like this:

RelationBuildPublicationDesc()
..
if (relation->rd_rel->relispartition)
ancestors = get_partition_ancestors(relid);

if (ancestors)
{
Oid last_ancestor_relid;

last_ancestor_relid = llast_oid(ancestors);
...

Best Regards,
Zhijie Hou

On Fri, Sep 11, 2026 at 12:52 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Hi,

On Friday, September 11, 2026 2:01 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Thanks for sharing the patch. It looks good to me. I’ve updated the attached
patch accordingly.

The fix looks mostly good to me. The only nit is that we could avoid introducing
a new Boolean flag by writing it like this:

RelationBuildPublicationDesc()
..
if (relation->rd_rel->relispartition)
ancestors = get_partition_ancestors(relid);

if (ancestors)
{
Oid last_ancestor_relid;

last_ancestor_relid = llast_oid(ancestors);
...

Okay, agree. Changed as suggested.

Also addressed Shveta’s comments from [1]/messages/by-id/CAJpy0uD9TLWsnLXBk4Wz7ydDc5=Oh+FamgqKRHd64RcG1h-s8g@mail.gmail.com and Amit’s off-list suggestions.

Please find the updated patch attached.

[1]: /messages/by-id/CAJpy0uD9TLWsnLXBk4Wz7ydDc5=Oh+FamgqKRHd64RcG1h-s8g@mail.gmail.com

--
Thanks,
Nisha

On Fri, Sep 11, 2026 at 1:36 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

Okay, agree. Changed as suggested.

Also addressed Shveta’s comments from [1] and Amit’s off-list suggestions.

Please find the updated patch attached.

Pushed. With this we are good to close PostgreSQL 19 open item related
to this. I'll do that early next week unless someone think otherwise.
We can continue to work on other related pre-existing issues.

--
With Regards,
Amit Kapila.