Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 12 Jan 2023 15:30:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Subject: Re: recovery modules
Message-ID: <Y7+pEJbJGcxpvqLF@paquier.xyz>
References: <20221227192449.GA3672473@nathanxps13>
 <20221227221111.xjw5hc3fqzt773gu@awork3.anarazel.de>
 <Y6uNJVNP29B4egDx@paquier.xyz>
 <20221229194326.GA296099@nathanxps13>
 <20230103175917.GA204418@nathanxps13>
 <20230103190538.GA287415@nathanxps13>
 <Y75rA+YNIWnJ1zsH@paquier.xyz>
 <20230111192901.GA1955161@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="mqq342nMs5gbc+qX"
Content-Disposition: inline
In-Reply-To: <20230111192901.GA1955161@nathanxps13>
Archived-At: <https://www.postgresql.org/message-id/Y7%2BpEJbJGcxpvqLF%40paquier.xyz>
Precedence: bulk


--mqq342nMs5gbc+qX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Jan 11, 2023 at 11:29:01AM -0800, Nathan Bossart wrote:
> I'm having trouble thinking of any practical advantage of providing the
> redo LSN and TLI.  If the main use-case is removing older archives as the
> documentation indicates, it seems better to provide the file name so that
> you can plug it straight into strcmp() to determine whether the file can be
> removed (i.e., what pg_archivecleanup does).  If we provided the LSN and
> TLI instead, you'd either need to convert that into a WAL file name for
> strcmp(), or you'd need to convert the candidate file name into an LSN and
> TLI and compare against those.

Logging was one thing that came immediately in mind, to let the module
know the redo LSN and TLI the segment name was built from without
having to recalculate it back.  If you don't feel strongly about that,
I am fine to discard this remark.  It is not like this hook should be
set in stone across major releases, in any case.

> I initially created a separate basic_restore module, but decided to fold it
> into basic_archive to simplify the patch and tests.  I hesitated to rename
> it because it already exists in v15, and since it deals with creating and
> restoring archive files, the name still seemed somewhat accurate.  That
> being said, I don't mind renaming it if that's what folks want.

I've done that in the past for pg_verify_checksums -> pg_checksums, so
I would not mind renaming it so as it reflects better its role.
(Being outvoted is fine for me if this suggestion sounds bad).

Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with
the duplication for the segment name build), so I would be tempted to
get this one done.  My gut tells me that we'd better remove the
duplication and just pass down the two fields to
shell_archive_cleanup() and shell_recovery_end(), with the segment
name given to ExecuteRecoveryCommand()..
--
Michael

--mqq342nMs5gbc+qX
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmO/qRAACgkQnvQgOdby
QH3LiQ/+KlQ3ChTMTcRE7xVuoOjcc3kQRJw+KQkOuaVivvMlgM4OAuF6bF+zGcb/
RevkJHVCU2a02XPubR9OKn7glBCNYzxvIpfRidznGf+fzOj9naV04/Ynxm6I1TBV
ut4d8qW/os+v1Ijj4/Z0EujrxAxzeBf2qawohB53k3ZKoDSUmOSGOQHzdV7Lw34b
kk1ASibrJxsiHPwSDwRCvIp1QxQRRWoeUmoSOVg8L+WOVx6x+SrNZOxiUESPGdcE
EtHm9jQCIm+1MGV9bKvP64gimF15cShlV3eGQxkZFWfjUjerOz3lsTA2X+46bWQu
xcvYwaZNYhPhG++lud1XvJm0cWF4atGKNF0hiyYtjov7MtOeu3QnfkEVcMwxDWqb
i8wOVNypzSpKcmtC2cTZAWlroewfSHfCUjH2DWeSLo+Frrw7sb1LboGrOH1KCAhd
Vk7uUqFPAW66KCyzTPZ/JWHyfU86AWVR3w4EeWd6m18HzViJc/t+nOl1JBiDFgWI
iVWxn3mLWm/7W3kAQVGliMcjhbkGBEIGxcN/LPYNgKiM4dRKuZjAqcDH3ksMBDYf
5ad4RPurM3fmlgwhkABrd+iOgl95R8bW2cOT9mZZvbt0tsZ/KkjfpspRGbD1ZFxE
fibGJG2YwJrRB0y6bNvcVHHtyBnkv/uFgoihKygLB+5dPBC8CaQ=
=23/4
-----END PGP SIGNATURE-----

--mqq342nMs5gbc+qX--