Re: Add pg_walinspect function with block info columns

At Sat, 18 Mar 2023 10:08:53 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> On Sat, Mar 18, 2023 at 1:06 AM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> >
> > On Fri, Mar 17, 2023 at 12:20 AM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > > +1 for pg_get_wal_block_info emitting per-record WAL info too along
> > > with block info, attached v2 patch does that. IMO, usability wins the
> > > race here.
> >
> > I think that this direction makes a lot of sense. Under this scheme,
> > we still have pg_get_wal_records_info(), which is more or less an SQL
> > interface to the information that pg_waldump presents by default.
> > That's important when the record view of things is of primary
> > importance. But now you also have a "block oriented" view of WAL
> > presented by pg_get_wal_block_info(), which is useful when particular
> > blocks are of primary interest. I think that I'll probably end up
> > using both, while primarily using pg_get_wal_block_info() for more
> > advanced analysis that focuses on what happened to particular blocks
> > over time.
>
> Hm.

Even though I haven't explored every aspect of the view, I believe it
makes sense to have at least the record_type in the block data. I
don't know how often it will be used, but considering that, I have no
objections to adding all the record information (apart from the block
data itself) to the block data.

> > It makes sense to present pg_get_wal_block_info() immediately after
> > pg_get_wal_records_info() in the documentation under this scheme,
> > since they're closely related.
>
> -1. I don't think we need that and even if we did, it's hard to
> maintain that ordering in future. One who knows to use these functions
> will anyway get to know how they're related.

The documentation has just one section titled "General Functions"
which directly contains detailed explation of four functions, making
it hard to get clear understanding of the available functions. I
considered breaking it down into a few subsections, but that wouldn't
look great since most of them would only contain one function.
However, I feel it would be helpful to add a list of all functions at
the beginning of the section.

> > (Checks pg_walinspect once more...)
> >
> > Actually, I now see that block_ref won't be NULL for those records
> > that have no block references at all -- it just outputs an empty
> > string.
>
> Yes, that's unnecessary.
>
> > But wouldn't it be better if it actually output NULL?
>
> +1 done so in the attached 0001 patch.
>
> > Better
> > for its own sake, but also better because doing so enables describing
> > the relationship between the two functions with reference to
> > block_ref. It seems particularly helpful to me to be able to say that
> > pg_get_wal_block_info() doesn't show anything for precisely those WAL
> > records whose block_ref is NULL according to
> > pg_get_wal_records_info().
>
> Hm.

I agree that adding a note about the characteristics would helpful to
avoid the misuse of pg_get_wal_block_info(). How about something like,
"Note that pg_get_wal_block_info() omits records that contains no
block references."?

> Attaching v3 patch set - 0001 optimizations around block references,
> 0002 enables pg_get_wal_block_info() to emit per-record info. Any
> thoughts?

+ /* Get block references, if any, otherwise continue. */
+ if (!XLogRecHasAnyBlockRefs(xlogreader))
+ continue;

I'm not sure, but the "continue" might be confusing since the code
"continue"s if the condition is true and continues the process
otherwise.. And it seems like a kind of "explaination of what the
code does". I feel we don't need the a comment there.

It is not an issue with this patch, but as I look at this version, I'm
starting to feel uneasy about the subtle differences between what
GetWALRecordsInfo and GetWALBlockInfo do. One solution might be to
have GetWALBlockInfo return a values array for each block, but that
could make things more complex than needed. Alternatively, could we
get GetWALRecordsInfo to call tuplestore_putvalues() internally? This
way, both functions can manage the temporary memory context within
themselves.

About 0002:

+ /* Reset only per-block output columns, keep per-record info as-is. */
+ memset(&nulls[PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS], 0,
+ PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS * sizeof(bool));
+ memset(&values[PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS], 0,
+ PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS * sizeof(bool));

sizeof(*values) is not sizeof(bool), but sizeof(Datum).

It seems to me that the starting elemnt of the arrays is
(PG_GET_WAL_BLOCK_INFO_COLS -
PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS). But I don't think simply
rewriting that way is great.

#define PG_GET_WAL_RECORD_INFO_COLS 11
...
+#define PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS 9

This means GetWALBlockInfo overwrites the last two columns generated
by GetWalRecordInfo, but I don't think this approach is clean and
stable. I agree we don't want the final columns in a block info tuple
but we don't want to duplicate the common code path.

I initially thought we could devide the function into
GetWALCommonInfo(), GetWALRecordInfo() and GetWALBlockInfo(), but it
doesn't seem that simple.. In the end, I think we should have separate
GetWALRecordInfo() and GetWALBlockInfo() that have duplicate
"values[i++] = .." lines.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center