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

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 4 Apr 2022 09:15:42 +0530

On Sat, Apr 2, 2022 at 5:05 AM Jeff Davis <[email-masked]> wrote:
>
> On Sat, 2022-03-26 at 10:31 +0530, Bharath Rupireddy wrote:
> > Attaching v16 patch-set, only change in v16-0002-pg_walinspect.patch,
> > others remain the same.
>
> I looked more closely at this patch.

Thanks Jeff for reviewing this.

> * It seems that pg_get_wal_record() is not returning the correct raw
> data for the record. I tested with pg_logical_emit_message, and the
> message isn't there. pg_get_wal_record_info() uses XLogRecordGetData(),
> which seems closer to what I expect.
>
> * I'm a little unclear on the purpose of pg_get_wal_record(). What does
> it offer that the other functions don't?

My intention is to return the overall undecoded WAL record [5] i.e.
the data starting from XLogReadRecord's output [6] till length
XLogRecGetTotalLen(xlogreader);. Please see [7], where Andres agreed
to have this function, I also mentioned a possible use-case there.

pg_get_wal_record_info returns the main data of the WAL record
(xl_heap_delete, xl_heap_insert, xl_heap_multi_insert, xl_heap_update
and so on).

> * I don't think we need the stats at all. We can run GROUP BY queries
> on the results of pg_get_wal_records_info().

As identified in [1], SQL-version of stats function is way slower in
normal cases, hence it was agreed (by Andres, Kyotaro-san and myself)
to have a C-function for stats.

> * Include the xlinfo portion of the wal record in addition to the rmgr,
> like pg_waldump --stats=record shows. That way we can GROUP BY that as
> well.
>
> 5. For pg_get_wal_record and pg_get_wal_records, also return the xlinfo
> using rm_identify() if available.

Yes, that's already part of the description column (much like
pg_waldump does) and the users can still do it with GROUP BY and
HAVING clauses, see [4].

> * I don't think we need the changes to xlogutils.c. You calculate the
> end pointer based on the flush pointer, anyway, so we should never need
> to wait (and when I take it out, the tests still pass).
>
> 6. Remove changes to xlogutils.

As mentioned in [1], read_local_xlog_page_no_wait required because the
functions can still wait in read_local_xlog_page for WAL while finding
the first valid record after the given input LSN (the use case is
simple - just provide input LSN closer to server's current flush LSN,
may be off by 3 or 4 bytes).

Also, I tried to keep the changes minimal with the
read_local_xlog_page_guts static function. IMO, that shouldn't be a
problem.

> I think we can radically simplify it without losing functionality,
> unless I'm missing something.
>
> 1. Eliminate pg_get_wal_record(),
> pg_get_wal_records_info_till_end_of_wal(), pg_get_wal_stats(),
> pg_get_wal_stats_till_end_of_wal().
>
> 4. For pg_get_wal_records, if end_lsn is NULL, read until the end of
> WAL.

It's pretty much clear to the users with till_end_of_wal functions
instead of cooking many things into the same functions with default
values for input LSNs as NULL which also requires the functions to be
"CALLED ON NULL INPUT" types which isn't good. This was also suggested
by Andres, see [2], and I agree with it.

> 2. Rename pg_get_wal_record_info -> pg_get_wal_record
>
> 3. Rename pg_get_wal_records_info -> pg_get_wal_records

As these functions aren't returning the WAL record data, but info
about it (decoded data), I would like to retain the function names
as-is.

> 8. With only two functions in the API, it may even make sense to just
> make it a part of postgres rather than a separate module.

As said above, I would like to have till_end_of_wal versions. Firstly,
pg_walinspect functions may not be needed by everyone, the extension
provides a way for the users to install if required. Also, many
hackers have suggested new functions [3], but, right now the idea is
to get pg_walinspect onboard with simple-yet-useful functions and then
think of extending it with new functions later.

[1] https://www.postgresql.org/message-id/CALj2ACUvU2fGLw7keEpxZhGAoMQ9vrCPX-13hexQPoR%2BQRbuOw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/20220322180006.hgbsoldgbljyrcm7%40alap3.anarazel.de
[3] There are many functions we can add to pg_walinspect - functions
with wait mode for future WAL, WAL parsing, function to return all the
WAL record info/stats given a WAL file name, functions to return WAL
info/stats from historic timelines as well, function to see if the
given WAL file is valid and so on.
[4]
postgres=# select count(resource_manager), description, from
pg_get_wal_records_info('0/14E0568', '0/14F2568') group by description
having description like '%INSERT_LEAF%';
 count |     description
-------+---------------------
     7 | INSERT_LEAF off 108
     1 | INSERT_LEAF off 111
     1 | INSERT_LEAF off 135
     1 | INSERT_LEAF off 142
     3 | INSERT_LEAF off 143
     1 | INSERT_LEAF off 144
     1 | INSERT_LEAF off 145
     1 | INSERT_LEAF off 146
     1 | INSERT_LEAF off 274
     1 | INSERT_LEAF off 405
(10 rows)

[5]
/*
 * The overall layout of an XLOG record is:
 * Fixed-size header (XLogRecord struct)
 * XLogRecordBlockHeader struct
 * XLogRecordBlockHeader struct
 * ...
 * XLogRecordDataHeader[Short|Long] struct
 * block data
 * block data
 * ...
 * main data

[6]
XLogRecord *
XLogReadRecord(XLogReaderState *state, char **errormsg)
{
    decoded = XLogNextRecord(state, errormsg);
    if (decoded)
    {
        /*
         * This function returns a pointer to the record's header, not the
         * actual decoded record.  The caller will access the decoded record
         * through the XLogRecGetXXX() macros, which reach the decoded
         * recorded as xlogreader->record.
         */
        Assert(state->record == decoded);
        return &decoded->header;
    }

[7] https://www.postgresql.org/message-id/20220322180006.hgbsoldgbljyrcm7%40alap3.anarazel.de

Regards,
Bharath Rupireddy.

---

From: Jeff Davis <[email-masked]>
Date: Tue, 05 Apr 2022 22:02:43 -0700

> to have this function, I also mentioned a possible use-case there.

The current patch does not actually do this: it's returning a pointer
into the DecodedXLogRecord struct, which doesn't have the raw bytes of
the WAL record.

To return the raw bytes of the record is not entirely trivial: it seems
we have to look in the decoded record and either find a pointer into
readBuf, or readRecordBuf, depending on whether the record crosses a
boundary or not. If we find a good way to do this I'm fine keeping the
function, but if not, we can leave it for v16.

> (xl_heap_delete, xl_heap_insert, xl_heap_multi_insert, xl_heap_update

We also discussed just removing the main data from the output here.
It's not terribly useful, and could be arbitrarily large. Similar to
how we leave out the backup block data and images.

> to have a C-function for stats.a pointer into 

Now I agree. We should also have an equivalent of "pg_waldump --
stats=record" though, too.

> pg_waldump does) and the users can still do it with GROUP BY and

I still think an extra column for the results of rm_identify() would
make sense. Not critical, but seems useful.

> may be off by 3 or 4 bytes).

Did you look into using XLogReadAhead() rather than XLogReadRecord()?

> by Andres, see [2], and I agree with it.

OK, it's a matter of taste I suppose. I don't have a strong opinion.

> about it (decoded data), I would like to retain the function names

The name pg_get_wal_records_info bothers me slightly, but I don't have
a better suggestion.

One other thought: functions like pg_logical_emit_message() return an
LSN, but if you feed that into pg_walinspect you get the *next* record.
That makes sense because pg_logical_emit_message() returns the result
of XLogInsertRecord(), which is the end of the last inserted record.
But it can be slightly annoying/confusing. I don't have any particular
suggestion, but maybe it's worth a mention in the docs or something?

Regards,
	Jeff Davis

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 6 Apr 2022 14:15:12 +0530

> function, but if not, we can leave it for v16.

With no immediate use of raw WAL data without a WAL record parsing
function, I'm dropping that function for now.

> how we leave out the backup block data and images.

Done.

> stats=record" though, too.

Added a parameter per_record (with default being false, emitting
per-rmgr stats) to pg_get_wal_stats and
pg_get_wal_stats_till_end_of_wal, when set returns per-record stats,
much like pg_waldump.

> make sense. Not critical, but seems useful.

Added rm_identify as record_type column in pg_get_wal_record_info,
pg_get_wal_records_info, pg_get_wal_record_info_till_end_of_wal.
Removed the rm_identify from the description column as it's
unnecessary now here.

> Did you look into using XLogReadAhead() rather than XLogReadRecord()?

I don't think XLogReadAhead will help either, as it calls page_read
callback, XLogReadAhead->XLogDecodeNextRecord->ReadPageInternal->page_read->read_local_xlog_page
(which again waits for future WAL).

Per our internal discussion, I'm keeping the
read_local_xlog_page_no_wait as it offers a better solution without
much code duplication.

> The name pg_get_wal_records_info bothers me slightly, but I don't have

IMO, pg_get_wal_records_info looks okay, hence didn't change it.

> suggestion, but maybe it's worth a mention in the docs or something?

Yes, all the pg_walinspect functions would find the next valid WAL
record  to the input/start LSN and start returning the details from
then.

IMO, the descriptions of these functions have already specified it:

pg_get_wal_record_info
      Gets WAL record information of a given LSN. If the given LSN isn't
      containing a valid WAL record, it gives the information of the next
      available valid WAL record. This function emits an error if a future (the

all other functions say this:
     Gets information/statistics of all the valid WAL records between/from

Attaching v17 patch-set with the above review comments addressed.
Please have a look at it.

Regards,
Bharath Rupireddy.

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 7 Apr 2022 15:35:07 +0530

> Please have a look at it.

Had to rebase because of 5c279a6d350 (Custom WAL Resource Managers.).
Please find v18 patch-set.

Regards,
Bharath Rupireddy.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 11 Apr 2022 16:21:26 +0530

On Thu, Apr 7, 2022 at 3:35 PM Bharath Rupireddy
<[email-masked]> wrote:

I am facing the below doc build failure on my machine due to this work:

./filelist.sgml:<!ENTITY pgwalinspect SYSTEM "pgwalinspect.sgml">
Tabs appear in SGML/XML files
make: *** [check-tabs] Error 1

The attached patch fixes this for me.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Mon, 11 Apr 2022 18:33:06 +0530

> The attached patch fixes this for me.

Thanks. It looks like there's a TAB in between. Your patch LGTM.

I'm wondering why this hasn't been caught in the build farm members
(or it may have been found but I'm missing to locate it.).

Can you please provide me with the doc build command to catch these
kinds of errors?

Regards,
Bharath Rupireddy.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 12 Apr 2022 09:26:12 +0530

> Can you please provide me with the doc build command to catch these

Nothing special. In the doc/src/sgml, I did make clean followed by make check.

-- 
With Regards,
Amit Kapila.