On Thu, Oct 19, 2023 at 3:18 PM David Steele <da...@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
