On Sat, Aug 15, 2026 at 2:02 PM Shinya Kato <[email protected]> wrote: > > Thank you all for the discussion. We now have options A to E and > Amit's Option 1 and 2, so let me sort the proposals by where each one > intervenes. > > - Publication DDL time: forbid creating the publication (Matthias's > D), or warn (Greg's E). > > - UPDATE time on the publisher: reject the UPDATE, per row in > heap_update() (Amit's Option 1, Hou's 0001), or per statement in > CheckCmdReplicaIdentity() (Amit's Option 2, Hou's 0002). > > - Decode time on the publisher: error in pgoutput_row_filter() (my A). > > - Apply time on the subscriber: error in apply_handle_insert() (Nikhil's > patch). > > - Make it work instead of erroring: WAL-log the missing values, either > always (my B) or for user-chosen columns (the INCLUDE-like idea > upthread). > > - Document only (my C). > > The earlier a check runs, the more it prevents and the less it knows. > The DDL and UPDATE time checks fire before anything is written to WAL, > but they have to be conservative. Even Option 1 rejects an UPDATE > whose old and new rows both match the filter, which replicates fine as > a plain UPDATE today. >
Right, there will be some false positives due to that but I think we can't avoid that without evaluating a row_filter which I don't think is a good idea to do in the update code path as it can impact performance. > The decode and apply time checks are precise, > they fire exactly when a value is dropped, but by then the value is > gone. Between those two, the apply time error is recoverable with > ALTER SUBSCRIPTION SKIP while the decode time error leaves the slot > stuck, so Nikhil's check supersedes my A. > > Given that, the combination I would aim for is: > > - All branches (15+): Nikhil's apply time error plus a documentation > note. This is not redundant on master even after an UPDATE time check > lands there, because a master subscriber can replicate from an older > publisher that has no such check. > Is there a reason for your preference for an apply-time patch for back branches? I could think of following two reasons but not sure they are worth having different fix in back-branches: (a) a new member in exposed struct PublicationDesc; This is an ABI break due to which ideally this shouldn't be preferred to be backpatched? Though the risk is narrow as all six pre-existing fields keep their old offsets exactly. Old code reading any of them still gets the right value. It's only code that tries to read the new rf_exists_for_update field (which by definition doesn't exist in old-compiled code) that's affected, and only in the narrow "extension itself declares PublicationDesc pubdesc; on the stack and calls RelationBuildPublicationDesc()" scenario where the risk is an out-of-bounds stack write by the new backend into memory the old-sized struct doesn't own, not a silently-wrong read. (b) We are adding the check in performance sensitive code path (heap_update). However, it is guarded by multiple checks (like RI is changed, row_filter exists, old tuple has toasted data, wal_level is logical, etc.) which makes us traverse the attribute level loop in non-performance critical code-path. But still we can run some performance tests once the basic review of the patch is done. > - master: additionally reject at UPDATE time. I agree with Amit's lean > towards Option 1. Option 2 rejects every UPDATE on a row-filtered > table that merely has a toastable column outside the replica identity, > which is close to D in impact. > > - Future: the INCLUDE-like logging in a separate thread, which would > turn the remaining errors into working replication. > Yeah, the INCLUDE can be discussed separately once we decide on the main fix in this thread. Thanks for helping in fixing this bug. -- With Regards, Amit Kapila.
