Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

At Tue, 22 Mar 2022 11:00:06 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> On 2022-03-22 21:57:51 +0530, Bharath Rupireddy wrote:
> > > This is probably close to an order of magnitude slower than pg_waldump
> > > --stats. Which imo renders this largely useless.
> > 
> > Yeah that's true. Do you suggest having pg_get_wal_stats() a
> > c-function like in v8 patch [1]?
> 
> Yes.
>
> > SEe some numbers at [2] with pg_get_wal_stats using
> > pg_get_wal_records_info and pg_get_wal_records_info as a direct
> > c-function like in v8 patch [1]. A direct c-function always fares
> > better (84 msec vs 1400msec).
> 
> That indeed makes the view as is pretty much useless. And it'd probably be
> worse in a workload with longer records / many FPIs.

FWIW agreed. The SQL version is too slow..


> > > > +     for (block_id = 0; block_id <= record->max_block_id; block_id++)
> > > > +     {
> > >
> > > To me duplicating this much code from waldump seems like a bad idea from a
> > > maintainability POV.
> > 
> > Even if we were to put the above code from pg_walinspect and
> > pg_waldump into, say, walutils.c or some other existing file, there we
> > had to make if (pg_walinspect) appendStringInfo else if (pg_waldump)
> > printf() sort of thing, isn't it clumsy?
> 
> Why is that needed? Just use the stringinfo in both places? You're outputting
> the exact same thing in both places right now. There's already a stringinfo in
> XLogDumpDisplayRecord() these days (there wasn't back when pg_xlogddump was
> written), so you could just convert at least the relevant printfs in
> XLogDumpDisplayRecord().

> > Also, unnecessary if
> > conditions need to be executed for every record. For maintainability,
> > I added a note in pg_walinspect.c and pg_waldump.c to consider fixing
> > things in both places (of course this might sound dumbest way of doing
> > it, IMHO, it's sensible, given the if(pg_walinspect)-else
> > if(pg_waldump) sorts of code that we need to do in the common
> > functions). Thoughts?
> 
> IMO we shouldn't merge this with as much duplication as there is right now,
> the notes don't change that it's a PITA to maintain.

The two places emit different outputs but the only difference is the
delimiter between two blockrefs. (By the way, the current code forgets
to insert a delimiter there).  So even if the function took "bool
is_waldump", it is used only when appending a line delimiter.  It
would be nicer if the "bool is_waldump" were "char *delimiter".
Others might think differently, though..

So, the function looks like this.

StringInfo XLogBlockRefInfos(XLogReaderState *record, char *delimiter,
                            uint32 &fpi_len);


> > Yeah. It is to handle some edge cases to print the WAL  upto end_lsn
> > and avoid waiting in read_local_xlog_page. I will change it.
> > 
> > Actually, there's an open point as specified in [3]. Any thoughts on it?
> 
> Seems more user-friendly to wait - it's otherwise hard for a user to know what
> LSN to put in.

I'm not sure it is user-friendly that the function "freeze"s for a
reason uncertain to the user..  Even if any results are accumulated
before waiting, all of them vanishes by entering Ctrl-C to release the
"freeze".

About the usefulness of the waiting behavior, it depends on what we
think the function's major use cases are.  Robert (AFAIU) thinks it as
a simple WAL dumper that is intended to use in some automated
mechanism.  The start/end LSNs simply identifies the records to emit.
No warning/errors and no waits except for apparently invalid inputs.

I thought it as a means by which to manually inspect wal on SQL
interface but don't have a strong opinion on the waiting behavior.
(Because I can avoid that by giving a valid LSN pair to the function
if I don't want it to "freeze".)


Anyway, the opinions here on the interface are described as follows.

A. as a diag interface for human use.

  1. If the whole region is filled with records, return them all.
  2. If start-LSN is too past, starts from the first available record.

  3-1. If start-LSN is in futnre, wait for the record to come.
  4-1. If end-LSN is in future, waits for new records.
  5-1. If end-LSN is too past, error out?

B. as a simple WAL dumper

  1. If the whole region is filled with records, return them all.
  2. If start-LSN is too past, starts from the first available record.

  3-2. If start-LSN is in futnre, returns nothig.
  4-2. If end-LSN is in future, ends with the last available record.
  5-2. If end-LSN is too past, returns northing.

1 and 2 are uncontroversial.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center