On Tue, Jan 10, 2023 at 09:29:03AM +0100, Drouvot, Bertrand wrote:
> Thanks for updating the patch!
>
> +-- Compare FPI from WAL record and page from table, they must be same
>
> I think "must be the same" or "must be identical" sounds better (but not 100% sure).
>
> Except this nit, V4 looks good to me.
+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?
+ 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. For this one, I think that there is little point in showing 14
records, so I would stick with a style similar to pageinspect.
+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.
Using "relfile_number" would be a first, for what is defined in the
code and the docs as a filenode.
+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.
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
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).
--
Michael
