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

On Wed, 2022-03-02 at 22:37 +0530, Bharath Rupireddy wrote:
> 
> Attaching v6 patch set with above review comments addressed. Please
> review it further.

* Don't issue WARNINGs or other messages for ordinary situations, like
when pg_get_wal_records_info() hits the end of WAL.

* It feels like the APIs that allow waiting for the end of WAL are
slightly off. Can't you just do pg_get_wal_records_info(start_lsn,
least(pg_current_wal_flush_lsn(), end_lsn)) if you want the non-waiting 
behavior? Try to make the API more orthogonal, where a few basic
functions can be combined to give you everything you need, rather than
specifying extra parameters and issuing WARNINGs. I 

* In the docs, include some example output. I don't see any output in
the tests, which makes sense because it's mostly non-deterministic, but
it would be helpful to see sample output of at least
pg_get_wal_records_info().

* Is pg_get_wal_stats() even necessary, or can you get the same
information with a query over pg_get_wal_records_info()? For instance,
if you want to group by transaction ID rather than rmgr, then
pg_get_wal_stats() is useless.

* Would be nice to have a pg_wal_file_is_valid() or similar, which
would test that it exists, and the header matches the filename (e.g. if
it was recycled but not used, that would count as invalid). I think
pg_get_first_valid_wal_record_lsn() would make some cases look invalid
even if the file is valid -- for example, if a wal record spans many
wal segments, the segments might look invalid because they contain no
complete records, but the file itself is still valid and contains valid
wal data.

* Is there a reason you didn't include the timeline ID in
pg_get_wal_records_info()?

* Can we mark this extension 'trusted'? I'm not 100% clear on the
standards for that marker, but it seems reasonable for a database owner
with the right privileges might want to install it.

* pg_get_raw_wal_record() seems too powerful for pg_monitor. Maybe that
function should require pg_read_server_files? Or at least
pg_read_all_data?

Regards,
	Jeff Davis