On Fri, Aug 14, 2026 at 10:09 PM Zhijie Hou (Fujitsu) <[email protected]> wrote: > > 0001: heap_update check >
Few comments on 0001: =================== 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. 2. Can we check the required value from relation's pubdesc before calling RelationBuildPublicationDesc()? > I haven't added doc yet, but I can add it once we reach consensus. > Feel free to add where required. -- With Regards, Amit Kapila.
