Re: Combine pg_walinspect till_end_of_wal functions with others

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