On Wed, Nov 11, 2026 at 8:04 PM Zhijie Hou <[email protected]> wrote: > > Hi, > > On Wed, Nov11, 2026 at 05:06 PM shveta malik wrote: > > On Wed, Nov 11, 2026 at 1:05 PM Zhijie Hou <[email protected]> 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
