Re: pg_walinspect - a new extension to get raw WAL data and WAL stats - Mailing list pgsql-hackers

On Wed, Apr 6, 2022 at 10:32 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2022-04-04 at 09:15 +0530, Bharath Rupireddy wrote:
> > 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.
>
> 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.

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

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

Done.

> > 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.a pointer into
>
> Now I agree. We should also have an equivalent of "pg_waldump --
> 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.

> > 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 still think an extra column for the results of rm_identify() would
> 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.

> > 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).
>
> 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
> a better suggestion.

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

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

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.