On Wed, Apr 6, 2022 at 10:32 AM Jeff Davis <pg...@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.
v17-0001-Refactor-pg_waldump-code.patch
Description: Binary data
v17-0002-pg_walinspect.patch
Description: Binary data
v17-0003-pg_walinspect-tests.patch
Description: Binary data
v17-0004-pg_walinspect-docs.patch
Description: Binary data
