Re: Combine pg_walinspect till_end_of_wal functions with others

On Fri, Mar 10, 2023 at 9:54 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

Done this way in the attached v5 patch.

> 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.

This has already been taken care of in the previous patches, e.g. v3
and v4 and so in the latest v5 patch.

> +-- 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.

Agreed and used \dx+. One can anyways look at the function definitions
and compare for knowing what's changed.

> 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.

You mean, we need to test the till_end_of_wal functions that were
removed in the latest version 1.1 but they must work if the extension
is installed with 1.0? If yes, I now added them.

> +-- Invalid input LSNs
> +SELECT * FROM pg_get_wal_record_info('0/0'); -- ERROR
> +ERROR: invalid input LSN

Removed InvalidRecPtr checks for input/start LSN because anyways the
functions will fail with ERROR: could not read WAL at LSN 0/0.

Any comments on the attached v5 patch?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com