Hello again , needed a rebase. v11 is attached.
Best regards, Cagri On Sun, Sep 27, 2026 at 10:35 PM Cagri Biroglu <[email protected]> wrote: > Dear Hayato, > > Thank you for both rounds. The attached v10 answers all four points. It > also > carries the two fixes from your previous review, so it replaces v9. > > In short: > > - The tables are locked root first now, so naming a partition before its > root > no longer deadlocks. > - The TRUNCATE privilege is checked again under AccessExclusiveLock. > - Truncating the partitions of a subscribed partitioned table is what a > replicated TRUNCATE already does, so nothing extra is lost. Documented. > - REFRESH TABLE now runs the origin = none copy check, for the tables it > re-seeds. > > From your earlier review: > > > We may have to re-order relids to be the partition root first > > Done. I reproduced it without gdb, by holding the partition in a prepared > transaction: > > REFRESH TABLE test_c, test_p -- has test_c, wants test_p > INSERT INTO test_p -- has test_p, wants test_c > ERROR: deadlock detected > > SortRefreshTableRelations() sorts the named tables by how many partitioned > ancestors each one has, which puts every root ahead of its partitions. > Unrelated tables keep the order given. The reproducer is clean afterwards. > > > IIUC the check should be done after acquiring the AccessExclusive > > Right. objectNamesToOids() takes only AccessShareLock, deliberately, so > the > privilege can go away after the first check. TRUNCATE is safe because its > check sits in a RangeVarGetRelidExtended() callback that is re-run with the > lock held. This command has no callback, so it repeats the check itself. > > From this review: > > > In this case, REFRESH TABLE tab command truncate partition leaves. Data > in > > tab_part1 could be re-synced but tuples in tab_part2 were gone. Is it OK? > > It is not new. I ran your schema twice: > > local-only rows in tab_part2 surviving, out of 5 > publisher TRUNCATE : 0 > REFRESH TABLE : 0 > > apply_handle_truncate() expands a partitioned target the same way, so a > TRUNCATE on the publisher already empties tab_part2. The docs now say so. > > > How about separating the patch into two parts, 0001 is for normal tables > > (reject for partition root) and 0002 adds the partition handling? > > Rejecting partitioned roots removes only about fifty lines. It does not > remove the ancestor locking or the ancestor half of the cross-subscription > check, which are needed when a leaf is the subscribed table, as it is > whenever > publish_via_partition_root is off. Both problems you found here were in > that > code, so it would stay in 0001 and 0002 would add the easy part. I would > rather keep one patch, but I will split it if you prefer. > > > Can you evaluate the combination? I'm worry because initial sync does not > > take care the origin of tuples. > > thank you. With C -> B -> A and A on origin = none, the same > copy made CREATE SUBSCRIPTION warn and REFRESH TABLE stay silent. > check_publications_origin_tables() skips the tables a subscription already > has, because those come through WAL where origins are known, and this > command > is the one thing that puts such a table back through a copy. v10 runs the > check and passes the subscription's other tables as the skip list, so it > warns > for a table another subscription writes and stays quiet otherwise. > > check-world passes. > > Best regards, > Cagri > > On Fri, Sep 25, 2026 at 11:44 AM Hayato Kuroda (Fujitsu) < > [email protected]> wrote: > >> Dear Cagri, >> >> While thinking this feature again, I had two points: >> >> 01. >> Assuming the below schema definition, which only the subscriber has a >> partition: >> >> ``` >> pub - sub >> tab tab >> - tab_part1 // stores data from pub >> - tab_part2 // stores data generated locally >> ``` >> >> In this case, REFRESH TABLE tab command truncate partition leaves. Data in >> tab_part1 could be re-synced but tuples in tab_part2 were gone. Is it OK? >> >> I felt several issues have been reported for the partition table, and >> this trend >> would continue. How about separating the patch into two parts, 0001 is >> for normal >> tables (reject for partition root) and 0002 adds the partition handling? >> >> 02. >> Logical replication allowed to construct two(three)-way replication with >> the >> subscription parameter origin=none. Can you evaluate the combination? I'm >> worry >> because initial sync does not take care the origin of tuples. >> >> Best regards, >> Hayato Kuroda >> FUJITSU LIMITED >> >
v11-0001-refresh-table.patch
Description: Binary data
