On Monday, August 17, 2026 3:43 PM Amit Kapila <[email protected]> wrote: > > On Fri, Aug 14, 2026 at 10:09 PM Zhijie Hou (Fujitsu) <[email protected]> > wrote: > > > > 0001: heap_update check > > > > Few comments on 0001:
Thanks for the comments. > =================== > 1. > @@ -3453,6 +3478,34 @@ heap_update(Relation relation, const > ItemPointerData *otid, HeapTuple newtup, > id_attrs, &oldtup, > newtup, &id_has_external); > > + id_changed = bms_overlap(modified_attrs, id_attrs); > + > + /* > + * If the update could be transformed into an insert by a publication > + row > + * filter during decoding, reject it when it would lose an unchanged > + * out-of-line value of a column that is not part of the replica identity. > + */ > + if (check_unchanged_external && id_changed) > > Why did you place the above check in heap_update before label l2? If the > check ran before l2: (e.g. right where modified_attrs/id_key_changed are first > computed), a raised > ereport(ERROR) there could fire for an update attempt that was never actually > going to happen, the row might get updated by someone else in the interim, > EvalPlanQual retries with a different row version, and our error would have > been wrong or at least premature. Placing the check after the TM_Ok > confirmation and after the VM-pin retry (i.e. > after every goto l2 site) guarantees no more retries follow, so raising the > error > here means the update really was about to proceed against this exact tuple. Right, I agree we should move this after l2. > > 2. Can we check the required value from relation's pubdesc before calling > RelationBuildPublicationDesc()? I think we can do this by adding a new relcache API that only accesses the new flag. I've done that in this version. > > > I haven't added doc yet, but I can add it once we reach consensus. > > > > Feel free to add where required. Added. Here's the updated version. In this version, I extended pub_rf_contains_invalid_column to also check for row filter existence, rather than adding a new function. This is fine for HEAD, but for back branches we typically avoid changing public interfaces, so a new function might be needed there. However, since this is an internal cache function and I couldn't find any extensions (via GitHub or Debian code search) that use it, changing the interface is probably acceptable. I am sharing one version v2_PG18 that does not change existing function interface for PG18 for reference. BTW, I also couldn't find any extensions that depend on the size of this struct, so I personally think backpatching should be fine. Best Regards, Zhijie Hou
v2-0001-Reject-UPDATEs-that-would-silently-lose-a-row-fil.patch
Description: v2-0001-Reject-UPDATEs-that-would-silently-lose-a-row-fil.patch
v2-PG18-0001-Reject-UPDATEs-that-would-silently-lose-a-row-fil.patch
Description: v2-PG18-0001-Reject-UPDATEs-that-would-silently-lose-a-row-fil.patch
