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

Hi,

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

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.

    Assert(buf->data[buf->len - 1] != ' ');

    if (infobits & XLHL_XMAX_IS_MULTI)
        appendStringInfoString(buf, "IS_MULTI, ");
    if (infobits & XLHL_XMAX_LOCK_ONLY)
        appendStringInfoString(buf, "LOCK_ONLY, ");
    if (infobits & XLHL_XMAX_EXCL_LOCK)
        appendStringInfoString(buf, "EXCL_LOCK, ");
    if (infobits & XLHL_XMAX_KEYSHR_LOCK)
        appendStringInfoString(buf, "KEYSHR_LOCK, ");
    if (infobits & XLHL_KEYS_UPDATED)
        appendStringInfoString(buf, "KEYS_UPDATED, ");

    if (buf->data[buf->len - 1] == ' ')
    {
        /* Truncate-away final unneeded ", "  */
        Assert(buf->data[buf->len - 2] == ',');
        buf->len -= 2;
        buf->data[buf->len] = '\0';
    }

    appendStringInfoString(buf, "]");
}

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

static void
truncate_flags_desc(StringInfo buf, uint8 flags)
{
    appendStringInfoString(buf, "flags: [");

    if (flags & XLH_TRUNCATE_CASCADE)
        appendStringInfoString(buf, "CASCADE, ");
    if (flags & XLH_TRUNCATE_RESTART_SEQS)
        appendStringInfoString(buf, "RESTART_SEQS, ");

    if (buf->data[buf->len - 1] == ' ')
    {
        /* Truncate-away final unneeded ", "  */
        Assert(buf->data[buf->len - 2] == ',');
        buf->len -= 2;
        buf->data[buf->len] = '\0';
    }

    appendStringInfoString(buf, "]");
}

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

    else if (info == XLOG_HEAP_UPDATE)
    {
        xl_heap_update *xlrec = (xl_heap_update *) rec;

        appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ",
                        xlrec->old_offnum,
                        xlrec->old_xmax,
                        xlrec->flags);
        infobits_desc(buf, xlrec->old_infobits_set, "old_infobits");
        appendStringInfo(buf, ", new off: %u, xmax %u",
                        xlrec->new_offnum,
                        xlrec->new_xmax);
    }
    else if (info == XLOG_HEAP_HOT_UPDATE)
    {
        xl_heap_update *xlrec = (xl_heap_update *) rec;

        appendStringInfo(buf, "off: %u, xmax: %u, flags: 0x%02X, ",
                        xlrec->old_offnum,
                        xlrec->old_xmax,
                        xlrec->flags);
        infobits_desc(buf, xlrec->old_infobits_set, "old_infobits");
        appendStringInfo(buf, ", new off: %u, xmax: %u",
                        xlrec->new_offnum,
                        xlrec->new_xmax);
    }

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

description      | off: 1, xmax: 1183, flags: 0x00, old_infobits: [],
new off: 119, xmax 0

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

- Melanie