Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 15 May 2023 09:18:32 +0900
From: Michael Paquier <michael@paquier.xyz>
To: torikoshia <torikoshia@oss.nttdata.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: Allow pg_archivecleanup to remove backup history files
Message-ID: <ZGF6WNGoVcUwi2EF@paquier.xyz>
References: <d660ef741ce3d82f3b4283f1cafd576c@oss.nttdata.com>
 <20230425.172948.1630351042955772893.horikyota.ntt@gmail.com>
 <ZEhIiaCa2rdc/SLb@paquier.xyz>
 <5adce8cd5ad2269fe687d2d56a341c88@oss.nttdata.com>
 <CALj2ACWq=axkHYKkWBAy5_DuuRGXDOhju_2EsexhxpWKZ-fiHg@mail.gmail.com>
 <61abc656edfba4a668b367d590b1aaab@oss.nttdata.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="khPDrvZN/9vBN01Y"
Content-Disposition: inline
In-Reply-To: <61abc656edfba4a668b367d590b1aaab@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZGF6WNGoVcUwi2EF%40paquier.xyz>
Precedence: bulk


--khPDrvZN/9vBN01Y
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, May 12, 2023 at 05:53:45PM +0900, torikoshia wrote:
> On 2023-05-10 17:52, Bharath Rupireddy wrote:
> I was a little concerned about what to do when deleting both the files
> ending in .gz and backup history files.
> Is making it possible to specify both "-x .backup" and "-x .gz" the way to
> go?
>=20
> I also concerned someone might add ".backup" to WAL files, but does that
> usually not happen?

Depends on the archive command, of course.  I have seen code using
this suffix for some segment names in the past, FWIW.

>> Comments on the patch:
>> 1. Why just only the backup history files? Why not remove the timeline
>> history files too? Is it because there may not be as many tli switches
>> happening as backups?
>=20
> Yeah, do you think we should also add logic for '-x .history'?

Timeline history files can be critical pieces when it comes to
assigning a TLI as these could be retrieved by a restore_command
during recovery for a TLI jump or just assign a new TLI number at the
end of recovery, still you could presumably remove the TLI history
files that are older than the TLI the segment defined refers too.
(pg_archivecleanup has no specific logic to look at the history with
child TLIs for example, to keep it simple, and I'd rather keep it this
way).  There may be an argument for making that optional, of course,
but it does not strike me as really necessary compared to the backup
history files which are just around for debugging purposes.

>> 2.+sub remove_backuphistoryfile_run_check
>> +{
>> Why to invent a new function when run_check() can be made generic with
>> few arguments passed?
>=20
> Thanks, I'm going to reconsider it.

+       <para>
+         Remove files including backup history files, whose suffix is <fil=
ename>.backup</filename>.
+         Note that when <replaceable>oldestkeptwalfile</replaceable> is a =
backup history file,
+         specified file is kept and only preceding WAL files and backup hi=
story files are removed.
+       </para>

This addition to the documentation looks unprecise to me.  Backup
history files have a more complex format than just the .backup
suffix, and this is documented in backup.sgml.

How about plugging in some long options, and use something more
explicit like --clean-backup-history?

-   if ((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) &&
+   if (((IsXLogFileName(walfile) || IsPartialXLogFileName(walfile)) ||
+           (removeBackupHistoryFile && IsBackupHistoryFileName(walfile))) =
&&
            strcmp(walfile + 8, exclusiveCleanupFileName + 8) < 0)

Could it be a bit cleaner to split this check in two, saving one level
of indentation on the way for its most inner loop?  I would imagine
something like:
    /* Check file name */
    if (!IsXLogFileName(walfile) &&
	!IsPartialXLogFileName(walfile))
	continue;
    /* Check cutoff point */
    if (strcmp(walfile + 8, exclusiveCleanupFileName + 8) >=3D 0)
        continue;
    //rest of the code doing the unlinks.
--
Michael

--khPDrvZN/9vBN01Y
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmRhelcACgkQnvQgOdby
QH2SPRAAiDt0pXYzYGIT1EKRCqJUqPGcqNzTOm0sdNXaWOmZWEYYYlupGS1UobRt
jA802uHh1okeivymt3owB1QJ+IQhgvHSGAo9GWtzA6AC6AbjSgyGxikoIIr8yT7o
UTDIkwZKqRgT05dPPVZGt/2MFFNmM9UsaYVRB1QSC6MO/X3VCmpU1bTx+uJ5pLOd
4X3hjDyDIfBUxqkNRpzzMIT7mTeNJUTK3dx2Ngq3FYjR4G5BoOI9/4Ots+k4qzU4
10AlEK9DGAOxaxGCRkkYBxnmRvbROkB2+/GyabyAU+5kXFFWPUjb03Evl09Z2ndy
MFyMNHZ6+TLt9mmfwaoWkGADOcl7Xfxc4bupZ2nfQltjqG3VhdsRoMyMq406Xclj
EFtxpXOPWdb6jxbVTKmSR96BxcdWrlRBfkN/HI+KA/vCdyFS+9DNrHyfLNXKxCaP
jSVuiMuP7JU+NrBkpBZ/vRaruWjuYaJlj7NLfGTIU0JW/yVRQtTiURRZvR2XvthS
dRCxBIPoasbpzTj2yuj8/deFv0w657+v2WoxJVnaHHGMK8eUJNUFUceFypPq+mqp
u+SmcXX/0YeTnVZrmScngVqR9QomVFPy1lwJYSmgogbPe3U+37aLu55HUHHyngy5
NBbecfQOmW1TPhS+o+gF/AJ9nBDN/o+c/HEaJPCWDC2IWUI/MoI=
=LVjZ
-----END PGP SIGNATURE-----

--khPDrvZN/9vBN01Y--