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