On Tue, Apr 11, 2023 at 1:35 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman <melanieplageman@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 <pg@bowt.ie> wrote:
>
> On Tue, Apr 11, 2023 at 11:48 AM Peter Geoghegan <pg@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