On Thu, Aug 20, 2026 at 1:02 PM Masahiko Sawada <[email protected]> wrote: > > On Wed, Aug 19, 2026 at 10:55 PM Amit Kapila <[email protected]> wrote: > > > > On Thu, Aug 20, 2026 at 4:54 AM Masahiko Sawada <[email protected]> > > wrote: > > > > > It can cost us high in some cases that can occur in the narrow window > > of above conditions. I could think of two major points to consider: > > (a) Changed toasted columns don't need their old value logged at all — > > once a column is toasted in this transaction, its new value is already > > fully resolvable from the new tuple, so flattening the old tuple just > > pays a detoast fetch and extra WAL for a value nothing downstream > > reads; (b) The trigger condition (rf_exists_for_update && > > id_key_changed && HeapTupleHasExternal(oldtup)) can't tell whether the > > filter's match result actually differs between old and new row (we > > need additional WAL only when row_filter matches new row but not old), > > since that needs evaluating the filter itself, already rejected as too > > invasive inside heap_update(). > > I think we can somehow check (a) during heap_update(). But I agree > that we should not check (b) during heap_update(). >
Fair enough. I think we can try to come up with a patch and see if it is good for both HEAD and back-branches. BTW, one thing to note in at least the current posted patch, there is an ABI break with new member rf_exists_for_update in PublicationDesc. I feel as it can be added at the end of structure, the chances of breaking any existing extension are very less. Also, the new parameter in pub_rf_contains_invalid_column could be harmless as the function shouldn't be used but we can overcome that by having a separate function like pub_has_row_filter() as shown in Hou-San's patch for PG18 version. > > > > So, combining both cases, I am worried it could be additional CPU and > > WAL cost when the same is not required though only in narrow cases. > > But I see there is an argument that all of this work is done when > > toasting for both old and new tuples is there which is not a very hot > > code path but still I am not sure this additional cost is worth it. > > > > > While it does > > > write more WAL, but only in that narrow case, It would be better than > > > requiring users to change RI setting or publication settings. Also, I > > > guess it can be back-patched as it neither adds a new WAL record type > > > nor changes the existing WAL format. Ideally, it would be sufficient > > > to write RI + unchanged out-of-line column data. But that needs new > > > logic to select the attribute, so I would leave it for the master. > > > > > > > Right, we can have this solution for master but thought first we can > > have a somewhat less risky fix for backbranches (including master). > > Then as a separate task try to improve the situation in master by > > considering various options like the one you suggested, or have > > something like INCLUDE kind of syntax for RI, or yet another one is to > > consider evaluating row_filter for such narrow cases. > > While the proposed fix is less risky, I'd like to be clear about what > a user can actually do when it fires. The patch suggests "To enable > updating the table, set REPLICA IDENTITY FULL or remove the row filter > from the publication.". As far as I can see, possible actions are: > > 1. Set RI FULL. > 2. Remove the row filter from the publication. > 3. Rewrite the offending statements to also modify the out-of-line column. > 4. Change the application so that it never issues such updates. > > (1) doesn't actually work if the publication has a column list. > pub_contains_invalid_column() rejects that combination (RI FULL + > column list). So every UPDATE on the table fails. Even if users remove > the column list from the publication, RI FULL logs the whole old tuple > on every UPDATE, which is more WAL and more CPU than what I proposed. > > (2) could require some system architecture changes. Dropping the > filter sends the subscriber rows it was deliberately not supposed to > receive, and adds the network traffic and apply cost for all of them. > > (3) works: e.g., "UPDATE t SET id = 7, body = body || ''" passes the > check. But it writes a fresh copy of the value into the toast table, > so it costs at least as much WAL as logging the old value would have, > and leaves the previous toast rows behind for vacuum. > > (4) I don't think this is reachable in practice. > > Am I missing an option? If there is a workaround that keeps both the > row filter and the column list, and does not cost more than the WAL we > are trying to avoid, it would be great. Otherwise, I'm concerned that > prohibiting these updates leaves users without a practical answer. > If we have to give an ERROR during heap_update then we won't have an alternative but if we follow what Nikhil has proposed there is another alternative to use Alter Subscription ... Skip on subscriber-side but not sure if it is a good idea. -- With Regards, Amit Kapila.
