Re: Add a new pg_walinspect function to extract FPIs from WAL records

On Wed, Jan 11, 2023 at 10:07 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> +postgres=# SELECT lsn, tablespace_oid, database_oid, relfile_number,
> block_number, fork_name, length(fpi) > 0 as fpi_ok FROM
> pg_get_wal_fpi_info('0/7418E60', '0/7518218');
>
> This query in the docs is too long IMO. Could you split that across
> multiple lines for readability?

Done.

> + pg_get_wal_fpi_info(start_lsn pg_lsn,
> + end_lsn pg_lsn,
> + lsn OUT pg_lsn,
> + tablespace_oid OUT oid,
> + database_oid OUT oid,
> + relfile_number OUT oid,
> + block_number OUT int8,
> + fork_name OUT text,
> + fpi OUT bytea)
> I am a bit surprised by this format, used to define the functions part
> of the module in the docs, while we have examples that actually show
> what's printed out. I understand that this comes from the original
> commit of the module, but the rendered docs are really hard to parse
> as well, no? FWIW, I think that this had better be fixed as well in
> the docs of v15.. Showing a full set of attributes for the returned
> record is fine by me, still if these are too long we could just use
> \x.

Thanks. I'll work on that separately.

> For this one, I think that there is little point in showing 14
> records, so I would stick with a style similar to pageinspect.

I've done it that way for pg_get_wal_fpi_info. If this format looks
okay, I can propose to do the same for other functions (for
backpatching too) in a separate thread though.

> +CREATE FUNCTION pg_get_wal_fpi_info(IN start_lsn pg_lsn,
> + IN end_lsn pg_lsn,
> + OUT lsn pg_lsn,
> + OUT tablespace_oid oid,
> Slight indentation issue here.

Done.

> Using "relfile_number" would be a first, for what is defined in the
> code and the docs as a filenode.

Yes, I've changed the column names to be consistent (like in pg_buffercache).

> +SELECT pg_current_wal_lsn() AS wal_lsn4 \gset
> +-- Get FPI from WAL record
> +SELECT fpi AS page_from_wal FROM pg_get_wal_fpi_info(:'wal_lsn3', :'wal_lsn4')
> + WHERE relfile_number = :'sample_tbl_oid' \gset
> I would be tempted to keep the checks run here minimal with only a
> basic set of checks on the LSN, without the dependencies to
> pageinspect (tuple_data_split and get_raw_page), which would be fine
> enough to check the execution of the function.

I understand the concern here that creating dependency between
extensions just for testing isn't good.

I'm okay to just read the LSN (lsn1) from raw FPI (bytea stream) and
the WAL record's LSN (lsn2) and compare them to be lsn2 > lsn1. I'm
looking for a way to convert the first 8 bytes from bytea stream to
pg_lsn type, on a quick look I couldn't find direct conversion
functions, however, I'll try to figure out a way.

> FWIW, I am surprised by the design choice behind ValidateInputLSNs()
> to allow data to be gathered until the end of WAL in some cases, but
> to not allow it in others. It is likely too late to come back to this
> choice for the existing functions in v15 (quoique?), but couldn't it

Separate functions for users passing end_lsn by themselves and users
letting functions decide the end_lsn (current flush LSN or replay LSN)
were chosen for better and easier usability and easier validation of
user-entered input lsns.

We deliberated to have something like below:
pg_get_wal_stats(start_lsn, end_lsn, till_end_of_wal default false);
pg_get_wal_records_info(start_lsn, end_lsn, till_end_of_wal default false);

We wanted to have better validation of the start_lsn and end_lsn, that
is, start_lsn < end_lsn and end_lsn mustn't fall into the future when
users specify it by themselves (otherwise, one can easily trick the
server by passing in the extreme end of the LSN - 0xFFFFFFFFFFFFFFFF).
And, we couldn't find a better way to deal with when till_end_of_wal
is passed as true (in the above version of the functions).

Another idea was to have something like below:
pg_get_wal_stats(start_lsn, end_lsn default '0/0');
pg_get_wal_records_info(start_lsn, end_lsn default '0/0');

When end_lsn is not entered or entered as invalid lsn, then return the
stats/info till end of the WAL. Again, we wanted to have some
validation of the user-entered end_lsn.

Instead of cooking multiple behaviours into a single function we opted
for till_end_of_wal versions. I still feel this is better unless
there's a strong reason against till_end_of_wal versions.

> be useful to make this new FPI function work at least with an insanely
> high LSN value to make sure that we fetch all the FPIs from a given
> start position, up to the end of WAL? That looks like a pretty good
> default behavior to me, rather than issuing an error when a LSN is
> defined as in the future.. I am really wondering why we have
> ValidateInputLSNs(till_end_of_wal=false) to begin with, while we could
> just allow any LSN value in the future automatically, as we can know
> the current insert or replay LSNs (depending on the recovery state).

Hm. How about having pg_get_wal_fpi_info_till_end_of_wal() then?

With some of the review comments addressed, I'm attaching v5 patch
herewith. I would like to hear thoughts on the above open points
before writing the v6 patch.

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