Re: [PATCH v1] strengthen backup history filename check - Mailing list pgsql-hackers

From Junwang Zhao
Subject Re: [PATCH v1] strengthen backup history filename check
Date
Msg-id CAEG8a3+K-1oocgvkuPZ+wDW1RbNfC+UpafrmSdo=xPQ5NqaE=g@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH v1] strengthen backup history filename check  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
List pgsql-hackers
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].

[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
>  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



pgsql-hackers by date:

Previous
From: Andrew Dunstan
Date:
Subject: Re: Cleaning up historical portability baggage
Next
From: Noah Misch
Date:
Subject: Re: fairywren hung in pg_basebackup tests