On Thu, Oct 19, 2023 at 3:18 PM David Steele <david@pgmasters.net> wrote:
> 0001 looks pretty good to me. The only thing I find a little troublesome
> is the repeated construction of file names with/without segment numbers
> in ResetUnloggedRelationsInDbspaceDir(), .e.g.:
>
> + if (segno == 0)
> + snprintf(dstpath, sizeof(dstpath), "%s/%u",
> + dbspacedirname, relNumber);
> + else
> + snprintf(dstpath, sizeof(dstpath), "%s/%u.%u",
> + dbspacedirname, relNumber, segno);
>
>
> If this happened three times I'd definitely want a helper function, but
> even with two I think it would be a bit nicer.
Personally I think that would make the code harder to read rather than
easier. I agree that repeating code isn't great, but this is a
relatively brief idiom and pretty self-explanatory. If other people
agree with you I can change it, but to me it's not an improvement.
> 0002 is definitely a good idea. FWIW pgBackRest does this conversion but
> also errors if it does not succeed. We have never seen a report of this
> error happening in the wild, so I think it must be pretty rare if it
> does happen.
Cool, but ... how about the main patch set? It's nice to get some of
these refactoring bits and pieces out of the way, but if I spend the
effort to work out what I think are the right answers to the remaining
design questions for the main patch set and then find out after I've
done all that that you have massive objections, I'm going to be
annoyed. I've been trying to get this feature into PostgreSQL for
years, and if I don't succeed this time, I want the reason to be
something better than "well, I didn't find out that David disliked X
until five minutes before I was planning to type 'git push'."
I'm not really concerned about detailed bug-hunting in the main
patches just yet. The time for that will come. But if you have views
on how to resolve the design questions that I mentioned in a couple of
emails back, or intend to advocate vigorously against the whole
concept for some reason, let's try to sort that out sooner rather than
later.
--
Robert Haas
EDB: http://www.enterprisedb.com