On Tue, 28 Nov 2023 at 14:37, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 28/11/2023 12:14, Pavel Borisov wrote: > On Tue, 28 Nov 2023 at 13:13, Heikki Linnakangas <hlinnaka@iki.fi> wrote: >> >> On 27/11/2023 01:43, Alexander Korotkov wrote: >>> v61 looks good to me. I'm going to push it as long as there are no objections. >> This was discussed earlier, but is still present in v61: >> >>> +/* >>> + * An internal function used by SlruScanDirectory(). >>> + * >>> + * Returns true if a file with a name of a given length may be a correct >>> + * SLRU segment. >>> + */ >>> +static inline bool >>> +SlruCorrectSegmentFilenameLength(SlruCtl ctl, size_t len) >>> +{ >>> + if (ctl->long_segment_names) >>> + return (len == 15); /* see SlruFileName() */ >>> + else >>> + /* >>> + * Commit 638cf09e76d allowed 5-character lengths. Later commit >>> + * 73c986adde5 allowed 6-character length. >>> + * >>> + * XXX should we still consider such names to be valid? >>> + */ >>> + return (len == 4 || len == 5 || len == 6); >>> +} >>> + >> >> I think it's pretty sloppy that the "short" filenames can be 4, 5 or 6 >> chars long. For pg_multixact/members, which introduced the 5-char case, >> I think we should always pad the filenames 5 characters, and for >> commit_ts which introduced the 6 char case, always pad to 6 characters. >> >> Instead of a "long_segment_names" boolean, how about an integer field, >> to specify the length. >> >> That means that we'll need pg_upgrade to copy pg_multixact/members files >> under the new names. That should be pretty straightforward. > > I think what's done in patch 0001 is just an extension of existing > logic and moving it into separate function.
That's right. I'm arguing that now is a good time to clean it up.
I won't insist if Alexander prefers to commit it as it is, though. But let's at least explain how this works in the comment, instead of the XXX.
I agree with you that would be good to add a comment instead of XXX and commit.