Message-ID: <39c13407c8b7311e9a3b24f2df814696bfa2ebff.camel@j-davis.com>
Subject: Re: pg_walinspect - a new extension to get raw WAL data and WAL
 stats
From: Jeff Davis <pgsql@j-davis.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Ashutosh
	Sharma <ashu.coek88@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>, Robert Haas
 <robertmhaas@gmail.com>,  Greg Stark <stark@mit.edu>, Jeremy Schneider
 <schneider@ardentperf.com>, Bruce Momjian <bruce@momjian.us>,  PostgreSQL
 Hackers <pgsql-hackers@lists.postgresql.org>, SATYANARAYANA NARLAPURAM
 <satyanarlapuram@gmail.com>,  marvin_liang@qq.com, actyzhang@outlook.com
Date: Thu, 10 Mar 2022 00:22:05 -0800
In-Reply-To: <CALj2ACVY37ed+BT_WeM2KQS1TUM90HO9NjNktaKCX2G6X5iXsQ@mail.gmail.com>
References: 
	<CALj2ACUGUYXsEQdKhEdsBzhGEyF3xggvLdD8C0VT72TNEfOiog@mail.gmail.com>
	 <20211005220707.GC24555@momjian.us>
	 <47c04e88-7df8-963c-e32e-491260a09691@ardentperf.com>
	 <20211006004310.GE24555@momjian.us>
	 <9186c1cd-04b6-3dc9-40c5-5977dd854e36@ardentperf.com>
	 <CALj2ACW394mcG+F7SKMt9zvBM2se=k5qtu-oeXC1gKSs97dyZA@mail.gmail.com>
	 <CALj2ACV2yj1xoM2JChv1_R28s_SvMVoFDMFQFnSm0DSMPGuvhA@mail.gmail.com>
	 <CALj2ACUnnvJAEv=+iRBi-f6mW0xTrC1Xz8VLZW4_NzF52b_zRQ@mail.gmail.com>
	 <CALj2ACU1dDczW9hhCqrxLBFN-2f4q7p5nvLc+9A8uM_Ewa0BaQ@mail.gmail.com>
	 <CALj2ACVYike1Vgz=ZWC+mQ=0Cz8d9hh6=ohccwiWWYa3WvUMig@mail.gmail.com>
	 <CAM-w4HMVn1D3zyi6OM-8eMqZizLOReB4z3fTGUwDujTvaqy=vA@mail.gmail.com>
	 <CA+TgmoYu2ucdqFhg4OL+uhvn=bz10MrbCVcTUPWd1_H11W=caw@mail.gmail.com>
	 <e0315b06-7a55-6831-d1a8-f006d398da84@dunslane.net>
	 <CALj2ACWhUrjrTDffN7ctCY+6eJ=izd9itixaNYJQdd8PL1vQHA@mail.gmail.com>
	 <CAE9k0P=9SReU_613TXytZmpwL3ZRpnC5zrf96UoNCATKpK-UxQ@mail.gmail.com>
	 <CALj2ACXsu42jnjyV7zpNtGMmDCsvXr59A9x-C94uyHov2nmp6Q@mail.gmail.com>
	 <CAE9k0P=+-BKGJhx_J+eCTx6dQhkno0Z+cWRThKMf43X6W=MzRw@mail.gmail.com>
	 <CALj2ACUtqWX95uAj2VNJED0PnixEeQ=0MEzpouLi+zd_iTugRA@mail.gmail.com>
	 <CAE9k0Pn4H48D7iK=T-B2CtnCCab7DgQ4MUUCLfzFf345HmkrGQ@mail.gmail.com>
	 <CALj2ACVY37ed+BT_WeM2KQS1TUM90HO9NjNktaKCX2G6X5iXsQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/39c13407c8b7311e9a3b24f2df814696bfa2ebff.camel%40j-davis.com>
Precedence: bulk

On Wed, 2022-03-02 at 22:37 +0530, Bharath Rupireddy wrote:
> 
> Attaching v6 patch set with above review comments addressed. Please
> review it further.

* Don't issue WARNINGs or other messages for ordinary situations, like
when pg_get_wal_records_info() hits the end of WAL.

* It feels like the APIs that allow waiting for the end of WAL are
slightly off. Can't you just do pg_get_wal_records_info(start_lsn,
least(pg_current_wal_flush_lsn(), end_lsn)) if you want the non-waiting 
behavior? Try to make the API more orthogonal, where a few basic
functions can be combined to give you everything you need, rather than
specifying extra parameters and issuing WARNINGs. I 

* In the docs, include some example output. I don't see any output in
the tests, which makes sense because it's mostly non-deterministic, but
it would be helpful to see sample output of at least
pg_get_wal_records_info().

* Is pg_get_wal_stats() even necessary, or can you get the same
information with a query over pg_get_wal_records_info()? For instance,
if you want to group by transaction ID rather than rmgr, then
pg_get_wal_stats() is useless.

* Would be nice to have a pg_wal_file_is_valid() or similar, which
would test that it exists, and the header matches the filename (e.g. if
it was recycled but not used, that would count as invalid). I think
pg_get_first_valid_wal_record_lsn() would make some cases look invalid
even if the file is valid -- for example, if a wal record spans many
wal segments, the segments might look invalid because they contain no
complete records, but the file itself is still valid and contains valid
wal data.

* Is there a reason you didn't include the timeline ID in
pg_get_wal_records_info()?

* Can we mark this extension 'trusted'? I'm not 100% clear on the
standards for that marker, but it seems reasonable for a database owner
with the right privileges might want to install it.

* pg_get_raw_wal_record() seems too powerful for pg_monitor. Maybe that
function should require pg_read_server_files? Or at least
pg_read_all_data?

Regards,
	Jeff Davis