Re: Show various offset arrays for heap WAL records - Mailing list pgsql-hackers

On Tue, Apr 11, 2023 at 7:40 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> static void
> infobits_desc(StringInfo buf, uint8 infobits, const char *keyname)
> {
>     appendStringInfo(buf, "%s: [", keyname);
>
> Why can we assume that there will be no space at the end here?

I don't think that anybody is going to try that, but if they do then
the assertion will fail reliably.

> I know we need to be able to avoid doing the comma overwriting if no
> flags were set. In general, we expect record description elements to
> prepend themselves with commas and spaces, but these infobits, for
> example, use a trailing comma and space. If someone adds a description
> element with a trailing space, they will trip this assert. We should at
> least add a comment explaining this assertion so someone knows what to
> do if they trip it.

The invariant here is roughly: caller's keyname argument cannot have
trailing spaces or punctuation characters. It looks like it would be
inconvenient to write a precise assertion for that, but it doesn't
feel particularly necessary, given that this is just a static helper
function.

> Otherwise, we can return early if no flags are set. That will probably
> make for slightly messier code since we would still have to construct
> the empty list.

I prefer to keep this as simple as possible for now.

> Also you didn't add the same assert to truncate_flags_desc().

That's because truncate_flags_desc doesn't have a "keyname" argument.
Though it does have an assertion at the end that is almost equivalent:
the "Assert(buf->data[buf->len - 2] == ',') assertion (a matching
assertion appears at the end of infobits_desc).

> 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.

> Also not the fault of this patch, but looking at the output while using
> this, I realized truncate record type has a stringified version of its
> flags while other record types, like update, don't. Do you think this
> makes sense? Perhaps not something we can change now, though...

You don't have to look at the truncate record type (which is a
relatively obscure and unimportant record type) to see these kinds of
inconsistencies. You can see the same thing with HEAP_UPDATE and
HEAP_HOT_UPDATE, which have stringified constants for infomask bits,
but not for the xl_heap_update.flags status bits.

I don't see any principled reason why such an inconsistency should
exist -- and we're talking about a pretty glaring inconsistency here.
On the other hand I don't think that we're obligated to do anything
about it for 16.

> Also not the fault of this patch, but I noticed that leaftopparent is
> often InvalidBlockNumber--which shows up as 4294967295. I wonder if
> anyone would be confused by this. Maybe devs know that this value is
> InvalidBlockNumber. In the future, perhaps we should interpolate the
> string "InvalidBlockNumber"?
>
> description | left: 436, right: 389, level: 0, safexid: 0:1091,
> leafleft: 436, leafright: 389, leaftopparent: 4294967295

In my personal opinion (this is a totally subjective question), the
current approach here is okay because (on its own) "leaftopparent:
4294967295" isn't any more or less meaningful than "leaftopparent:
InvalidBlockNumber". It's not as if the REDO routine actually relies
on the value ever being InvalidBlockNumber at all (except in an
assertion).

Plus it's easier to parse as-is. That's what swings it for me, in fact
(as with the "two xmax fields in update records" question).

This is the kind of question that tends to lead to bikeshedding. The
guidelines should avoid taking a firm position on these more
subjective questions, where we must make a subjective trade-off.
Especially a trade-off around how faithfully we represent the physical
WAL record versus readability (whatever "readability" means). I
pondered a similar trade-off in comments added to delvacuum_desc. That
contributed to my feeling that this is best left up to individual rmgr
desc routines.

--
Peter Geoghegan