Re: Crashes on a partition whose concurrent detach never finished

On Wed, Nov 11, 2026 at 8:04 PM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Wed, Nov11, 2026 at 05:06 PM shveta malik wrote:
> > On Wed, Nov 11, 2026 at 1:05 PM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
> > > Since we are considering whether to push 0001 first, I reviewed it
> > > and have few suggestions:
> > >
> > > 1. We can remove the am_partition variable completely and simply check
> > > "ancestors" in the subsequent code, to be consistent with
> > > RelationBuildPublicationDesc.
> > >
> > > 2. The test can be simplified to cover only the decoding crash (since
> > > the subscriber side is not touched by this patch), which saves
> > > buildfarm test time. In addition, we can move the test into the
> > > existing isolation test to speed it up further.
> > >
> > > I have attached a top-up patch for 0001 with these changes. Feel
> > > free to merge it if it looks good.
> >
> > I agree with the changes. Few concerns:
>
> Thanks for reviewing.
>
> >
> > It would be better to show in the additional steps that
> > pg_inherits.inhdetachpending is true before we actually try
> > pg_logical_slot_get_binary_changes.
>
> I checked that no existing tests (either for DETACH CONCURRENTLY or
> otherwise) refer to the inhdetachpendin in their test cases, and since
> our test basically tests the DETACH behavior as well, I think it's fine
> to skip testing this.
>
> >
> > Also I feel it would make more sense to create-pub with default
> > publish_via_partition_root rather than publish_via_partition_root=true
> > for PG19's testcase.
>
> Changed as suggested.
>
> After some analysis, I think creating slots in src/test/isolation/
> wouldn't be stable, since some buildfarm animals may run the main
> regression tests under wal_level=minimal. I also didn't find slot
> creation in the main isolation tests. So I chose to put the test in
> test_decoding/concurrent_ddl_dml.spec, which has precedent for creating
> slots with pgoutput under test_decoding tests, so it can be acceptable.
>
> I'm helping to merge the changes into 0001 and preparing patches for all
> branches, hoping to close this item soon.
>

Overall, the patches look good to me. Just a nit: the name "pub" looks
very generic for a test spec shared by many permutations. Perhaps we
can name it "pub_test_detach" or "pub_detach".

thanks
Shveta