Re: Add pg_walinspect function with block info columns - Mailing list pgsql-hackers

On Sun, Mar 19, 2023 at 8:21 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> 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.

I like the idea of sections, even if there is only one function per
section in some cases.

I also think that we should add a "Tip" that advises users that they
may use an "end LSN" that is the largest possible LSN,
'FFFFFFFF/FFFFFFFF' to get information about records up until the
current LSN of the cluster (per commit 5c1b6628).

Is there a straightforward way to get a usable LSN constant for this
purpose? The simplest way I could come up with quickly is "SELECT
pg_lsn(2^64.-1)" -- which still isn't very simple. Actually, it might
be even worse than 'FFFFFFFF/FFFFFFFF', so perhaps we should just use
that in the docs new "Tip".

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

This should be a strict invariant. In other words, it should be part
of the documented contract of pg_get_wal_block_info and
pg_get_wal_records_info. The two functions should be defined in terms
of each other. Their relationship is important.

Users should be able to safely assume that the records that have a
NULL block_ref according to pg_get_wal_records_info are *precisely*
those records that won't have any entries within pg_get_wal_block_info
(assuming that the same LSN range is used with both functions).
pg_walinspect should explicitly promise this, and promise the
corollary condition around non-NULL block_ref records. It is a useful
promise from the point of view of users. It also makes it easier to
understand what's really going on here without any ambiguity.

I don't completely disagree with Michael about the redundancy. I just
think that it's worth it on performance grounds. We might want to say
that directly in the docs, too.

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

+1.

Also, if GetWALBlockInfo() is now supposed to only be called when
XLogRecHasAnyBlockRefs() now then it should probably have an assertion
to verify the precondition.

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

 Agreed. I'm also not sure what to do about it, though.

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

I agree. A little redundancy is better when the alternative is fragile
code, and I'm pretty sure that that applies here -- there won't be
very many duplicated lines, and the final code will be significantly
clearer. There can be a comment about keeping GetWALRecordInfo and
GetWALBlockInfo in sync.

--
Peter Geoghegan