Re: Show various offset arrays for heap WAL records

On Mon, Apr 10, 2023 at 3:04 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> I took a look at the first patch even though you've pushed the bugfix
> part. Any reason you didn't use array_desc() for the inner array (of
> "ptids")? I find that following the pattern of using array_desc (when it
> is correct, of course!) helps me to quickly identify: "okay, this is an
> array of x" without having to stare at the loop too much.

It was fairly arbitrary. I was thinking "we can't use array_desc for
this", which wasn't 100% true, but seemed close enough. It helped that
this allowed me to remove uint16_elem_desc(), which likely wouldn't
have been reused later on.

> I will say that the prefix of p in "ptid" makes it sound like pointer to
> a tid, which I don't believe is what you meant.

I was thinking of the symbol name "ptid" from
_bt_delitems_delete_check() (it even appears in code comments). I
intended "posting list TID". But "pointer to a TID" actually kinda
works too, since these are offsets into a posting list (a simple
ItemPointerData array) for those TIDs that we're in the process of
removing/deleted from the tuple.

> I like the new guidelines you proposed (in the patch).
> They are well-written and clear.

Thanks. The guidelines might well become stricter in the future. Right
now I'd be happy if everybody could at least be in rough agreement
about the most basic things.

> I recognized that the output doesn't look nice, but I hadn't exactly
> thought of it as malformed. Perhaps you are right.

It does seem like an annoying thing to have to handle if you actually
want to parse the array. It requires a different approach to every
other array, which seems bad.

> I will say and I am still not a fan of the "if (first) else" logic in
> your attached patch.

I agree that my approach there was pretty ugly.

> How about we have the flags use a trailing comma and space and then
> overwrite the last one with something this:
>
> if (infobits & XLHL_KEYS_UPDATED)
> appendStringInfoString(buf, "KEYS_UPDATED, ");
> buf->data[buf->len -= strlen(", ")] = '\0';

I'll try something like that instead.

> - offsets = (OffsetNumber *) &plans[xlrec->nplans];
> + offsets = (OffsetNumber *) ((char *) plans +
> + (xlrec->nplans *
> + sizeof(xl_heap_freeze_plan)));
> appendStringInfoString(buf, ", plans:");
> array_desc(buf, plans, sizeof(xl_heap_freeze_plan), xlrec->nplans,
> &plan_elem_desc, &offsets);

I thought that it made sense to match the FREEZE_PAGE REDO routine.

Another fairly arbitrary change, to be honest.

> > Note that the patch makes many individual (say) HOT_UPDATE records
> > have descriptions that look like this:
> >
> > ... old_infobits: [], ...
> >
> > This differs from HEAD, where the output is totally suppressed because
> > there are no flag bits to show. I think that this behavior is more
> > logical and consistent overall.
>
> Yea, I think it is better to include things and show that they are empty
> then omit them. I find it more clear.

Right. It makes sense for something like this, because generally
speaking the structures aren't nested in any real sense. They're also
very static -- WAL records have a fixed structure. So it's unlikely
that anybody is going to try to parse the description before knowing
which particular WAL record type (or perhaps types, plural) are
involved.

--
Peter Geoghegan