[PATCH v1] strengthen backup history filename check
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t46290psql -h localhost -U postgresBuilt from patchset v1 (message #1), October 06, 2026 at 11:43 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t46290_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t46290_1 && git checkout t46290_1Patchset v1 (message #1) is on t46290_1
On Mon, Jul 25, 2022 at 5:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
This patch makes the backup history filename check more tight.
Can you please elaborate a bit on the issue with existing
IsBackupHistoryFileName(), if there's any?
Also, the patch does have hard coded numbers [1]static inline bool IsBackupHistoryFileName(const char *fname) { - return (strlen(fname) > XLOG_FNAME_LEN && + return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") && strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && - strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0); + strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 && + strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0); } which isn't good from
a readability perspective, adding macros and/or comments would help
here.
[1]
static inline bool
IsBackupHistoryFileName(const char *fname)
{
- return (strlen(fname) > XLOG_FNAME_LEN &&
+ return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") &&
strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN &&
- strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0);
+ strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 &&
+ strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0);
}Regards,
Bharath Rupireddy.
On Mon, Jul 25, 2022 at 7:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Mon, Jul 25, 2022 at 5:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
This patch makes the backup history filename check more tight.
Can you please elaborate a bit on the issue with existing
IsBackupHistoryFileName(), if there's any?
There are two call of this function, `CleanupBackupHistory` and
`SetWALFileNameForCleanup`, there
seems no issue of the existing IsBackupHistoryFileName() since the
creation of the backup history file
will make sure it is of format "%08X%08X%08X.%08X.backup".
The patch just makes `IsBackupHistoryFileName()` more match to
`BackupHistoryFileName()`, thus
more easier to understand.
Also, the patch does have hard coded numbers [1] which isn't good from
a readability perspective, adding macros and/or comments would help
here.
I'm not sure using macros is a good idea here, cause I noticed
`IsTLHistoryFileName()` also uses
some hard code numbers [1]static inline bool IsTLHistoryFileName(const char *fname) { return (strlen(fname) == 8 + strlen(".history") && strspn(fname, "0123456789ABCDEF") == 8 && strcmp(fname + 8, ".history") == 0); }.
[1]: static inline bool IsTLHistoryFileName(const char *fname) { return (strlen(fname) == 8 + strlen(".history") && strspn(fname, "0123456789ABCDEF") == 8 && strcmp(fname + 8, ".history") == 0); }
static inline bool
IsTLHistoryFileName(const char *fname)
{
return (strlen(fname) == 8 + strlen(".history") &&
strspn(fname, "0123456789ABCDEF") == 8 &&
strcmp(fname + 8, ".history") == 0);
}
[1] static inline bool IsBackupHistoryFileName(const char *fname) { - return (strlen(fname) > XLOG_FNAME_LEN && + return (strlen(fname) == XLOG_FNAME_LEN + 9 + strlen(".backup") && strspn(fname, "0123456789ABCDEF") == XLOG_FNAME_LEN && - strcmp(fname + strlen(fname) - strlen(".backup"), ".backup") == 0); + strspn(fname + XLOG_FNAME_LEN + 1, "0123456789ABCDEF") == 8 && + strcmp(fname + XLOG_FNAME_LEN + 9, ".backup") == 0); }Regards,
Bharath Rupireddy.
--
Regards
Junwang Zhao