> > Option B: when a table belongs to a publication with a row filter, >> make heap_update() log the whole old tuple, as it already does for >> REPLICA IDENTITY FULL. The existing copy loop then finds the value, >> the INSERT is complete, and no error is needed. This is the real fix, >> but every UPDATE of such a table writes the unchanged out-of-line >> value to WAL even when no transformation happens, which can be a large >> regression. It also needs a new field in PublicationDesc, so I do not >> think it can be back-patched. >> >> > The concern with option B is that every UPDATE of a row-filtered table > would write the unchanged out-of-line value even when no transformation > happens. I think that can be narrowed considerably: the extra logging > can be done based on HeapTupleHasExternal(), which is a single infomask bit > test requiring no deforming. ExtractReplicaIdentity() already uses > exactly that gate for REPLICA IDENTITY FULL. > > The cost then falls on every UPDATE of a row-filtered table whose row > currently holds out-of-line values, rather than on every UPDATE of such > a table. Where the wide column is usually NULL or stays inline this is > close to free, and where every row is toasted the cost is real -- but > that is precisely the case where the current behaviour loses data. > > What makes B attractive is that it needs no change to the output plugin, > the protocol or the subscriber. Once the old tuple carries the > flattened values, the copy loop already in pgoutput_row_filter() finds > them and the INSERT goes out complete. It is the same mechanism that > makes REPLICA IDENTITY FULL work today. > > I took a close look at option B and it might not be so attractive (what ever is? :-)) and might need invasive changes.
> I agree it cannot be back-patched, for the reasons given: it needs to > know at heap_update() time that the table is published with a row > filter, and it would introduce a WAL volume regression in a minor > release. > > >> Option C: document the restriction and leave the behavior alone. This >> is the only option that changes nothing on the back branches, but the >> value keeps disappearing without any warning. > > I lean towards A because losing data silently seems worse than >> stopping, but the unrecoverable error bothers me. Which approach do >> you prefer, and should the fix be back-patched? >> >> > > Rather than choosing among the three, would this combination work? > > - back branches (15 and up): the subscriber-side error described > above, together with a documentation note in the UPDATE > transformation section stating that a column which is stored > out-of-line, unchanged, and outside the replica identity cannot be > carried through the transformation, and that REPLICA IDENTITY FULL > avoids it. > > But this backpatching reason still holds good as far as I can see! Regards, Nikhil > - master: option B, so the INSERT is complete and no error is needed. > > Option C then becomes the documentation half of the first item rather > than a standalone choice. > > One thing worth being explicit about: back-patching an error changes > behaviour in a minor release. I still think it is the right trade, since > the > alternative is undetectable data loss, and where the column is NOT NULL > replication already fails today—just with a constraint violation that > points > at the symptom rather than the cause. > > Thanks, > --- > Nikhil Sontakke > PlanetScale >
