On Fri, Mar 10, 2023 at 12:24 PM Michael Paquier <mich...@paquier.xyz> wrote: > > 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.
As long as we provide a sensible default value (so I guess '0/0' to mean "no upper bound") and that we therefore don't have to manually specify an upper bound if we don't want one I'm fine with keeping the functions marked as STRICT. > 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. +1 > 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. Yeah the SQL function should be removed no matter what.