On Wed, Mar 08, 2023 at 01:40:46PM +0530, Bharath Rupireddy wrote: > 1. When start_lsn is NULL or invalid ('0/0'), emit an error. There was > a comment on the functions automatically determining start_lsn to be > the oldest WAL LSN. I'm not implementing this change now, as it > requires extra work to traverse the pg_wal directory. I'm planning to > do it in the next set of improvements where I'm planning to make the > functions timeline-aware, introduce functions for inspecting > wal_buffers and so on. > > [.. long description ..] > > 9. Refactored the tests according to the new behaviours.
Hmm. I think this patch ought to have a result simpler than what's proposed here. First, do we really have to begin marking the functions as non-STRICT to abide with the treatment of NULL as a special value? The part that I've found personally the most annoying with these functions is that an incorrect bound leads to a random failure, particularly when such queries are used for monitoring. I would simplify the whole with two simple rules, as of: - Keeping all the functions strict. - When end_lsn is a LSN in the future of the current LSN inserted or replayed, adjust its value to be the exactly GetXLogReplayRecPtr() or GetFlushRecPtr(). This way, monitoring tools can use a value ahead, at will. - Failing if start_lsn > end_lsn. - Failing if start_lsn refers to a position older than what exists is still fine by me. I would also choose to remove pg_get_wal_records_info_till_end_of_wal() from the SQL interface in 1.1 to limit the confusion arount it, but keep a few lines of code so as we are still able to use it when pg_walinspect 1.0 is the version enabled with CREATE EXTENSION. In short, pg_get_wal_records_info_till_end_of_wal() should be able to use exactly the same code as pg_get_wal_records_info(), still you need to keep *two* functions for their prosrc with PG_FUNCTION_ARGS as arguments so as 1.0 would work when dropped in place. The result, it seems to me, mostly comes to simplify ValidateInputLSNs() and remove its till_end_of_wal argument. +-- Removed function +SELECT pg_get_functiondef('pg_get_wal_records_info_till_end_of_wal'::regproc); +ERROR: function "pg_get_wal_records_info_till_end_of_wal" does not exist +LINE 1: SELECT pg_get_functiondef('pg_get_wal_records_info_till_end_... It seems to me that you should just replace all that and anything depending on pg_get_functiondef() with a \dx+ pg_walinspect, that would list all the objects part of the extension for the specific version you want to test. Not sure that there is a need to list the full function definitions, either. That just bloats the tests. I think, however, that it is critical to test in oldextversions.out the *executions* of the functions, so as we make sure that they don't crash. The patch is missing that. +-- Invalid input LSNs +SELECT * FROM pg_get_wal_record_info('0/0'); -- ERROR +ERROR: invalid input LSN -- Michael
signature.asc
Description: PGP signature