On Tue, Apr 11, 2023 at 1:35 PM Peter Geoghegan <p...@bowt.ie> wrote: > > On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman <melanieplage...@gmail.com> > wrote: > > Not the fault of this patch, but I also noticed that heap UPDATE and > > HOT_UPDATE records have xmax twice and don't differentiate between new > > and old. I think that was probably a mistake. > > > > description | off: 119, xmax: 1105, flags: 0x00, old_infobits: > > [], new off: 100, xmax 0 > > That doesn't seem great to me either. I don't like this ambiguity, > because it seems like it makes the description hard to parse in a way > that flies in the face of what we're trying to do here, in general. > So it seems like it might be worth fixing now, in the scope of this > patch.
Agreed. On Tue, Apr 11, 2023 at 3:22 PM Peter Geoghegan <p...@bowt.ie> wrote: > > On Tue, Apr 11, 2023 at 11:48 AM Peter Geoghegan <p...@bowt.ie> wrote: > > Attached revision deals with this by spelling out the names in full > > (e.g., "old_xmax" and "new_xmax"). It also reorders the output fields > > to match the order from the physical UPDATE, HOT_UPDATE, and LOCK WAL > > record types, on the theory that those should match the physical > > record (unless there is a good reason not to, which doesn't apply > > here). > > I just noticed that we don't even show xmax in the case of DELETE > records. Perhaps the original assumption is that it must match the > record's own XID, but that's not true after the MultiXact enhancements > for foreign key locking added to 9.3 (and in any case there is no > reason at all to show xmax in UPDATE but not in DELETE). > > Attached revision v4 fixes this, making DELETE, UPDATE, HOT_UPDATE, > LOCK, and LOCK_UPDATED record types consistent with each other in > terms of the key names output by the heap desc routine. The field > order also needed a couple of tweaks for struct consistency (and > cross-record consistency) for v4. Code in v4 all seems fine to me. I like the update guidelines comment. I agree it would be nice for xl_heap_lock->locking_xid to be renamed xmax for clarity. I would suggest that if you don't intend to put it in a separate commit, you mention it explicitly in the final commit message. Its motivation isn't immediately obvious to the reader. - Melanie
