Re: Combine pg_walinspect till_end_of_wal functions with others

On Tue, Mar 7, 2023 at 11:17 AM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> On Tue, Mar 07, 2023 at 01:56:24PM +0900, Michael Paquier wrote:
> > On Tue, Mar 07, 2023 at 12:42:20PM +0800, Julien Rouhaud wrote:
> > > ah right I should have checked. but the same ABI compatibility concern
> > > still exists for version 1.0 of the extension.
> >
> > Yes, we'd better make sure that the past code is able to run, at
> > least. Now I am not really convinced that we have the need to enforce
> > an error with the new code even if 1.0 is still installed,
>
> So keep this "deprecated" C function working, as it would only be a few lines
> of code?
>
> > so as it is
> > possible to remove all the traces of the code that triggers errors if
> > an end LSN is higher than the current insert LSN for primaries or
> > replayed LSN for standbys.
>
> +1 for that

I understand that we want to keep till_end_of_wal functions defined
around in .c file so that if someone does CREATE EXTENSION
pg_walinspect WITH VERSION '1.0'; on the latest extension shared
library (with 1.1 version), the till_end_of_wal functions should work
for them.

Also, I noticed that there's some improvement needed for the input
validations, especially for the end_lsn.

Here I'm with the v3 patch addressing the above comments. Please
review it further.

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.
2. When end_lsn is NULL or invalid ('0/0') IOW end_lsn is not
specified, deduce end_lsn to be the current flush LSN when not in
recovery, current replayed LSN when in recovery. This is the main
change that avoids till_end_of_wal functions in version 1.1.
3. When end_lsn is specified but greater than or equal to the
start_lsn, return NULL. Given the above review comments on more errors
being reported, I chose to return NULL for better usability.
4. When end_lsn is specified but less than the start_lsn, get
info/stats up until end_lsn.
5. Retained pg_get_wal_records_info_till_end_of_wal and
pg_get_wal_stats_till_end_of_wal for backward compatibility.
6. Piggybacked these functions and behaviour under the new HEAD-only
extension version 1.1 introduced recently, instead of bumping to 1.2.
When PG16 is out, users will have 1.1 with all of these new
functionality.
7. Added tests to verify the extension update path in
oldextversions.sql similar to other extensions'. (suggested by Michael
Paquier).
8. Added a note in the pg_walinspect documentation about removal of
pg_get_wal_records_info_till_end_of_wal and
pg_get_wal_stats_till_end_of_wal in version 1.1 and how the other
functions can be used to achieve the same functionality and how these
till_end_of_wal functions can work if extension is installed
explicitly with version 1.0.
9. Refactored the tests according to the new behaviours.

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