On 10/19/23 16:00, Robert Haas wrote:
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.
Then I'm fine with it as is.
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 simply have not had time to look at the main patch set in any detail.
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.
In my view this feature puts the cart way before the horse. I'd think
higher priority features might be parallelism, a backup repository,
expiration management, archiving, or maybe even a restore command.
It seems the only goal here is to make pg_basebackup a tool for external
backup software to use, which might be OK, but I don't believe this
feature really advances pg_basebackup as a usable piece of stand-alone
software. If people really think that start/stop backup is too
complicated an interface how are they supposed to track page
incrementals and get them to a place where pg_combinebackup can put them
backup together? If automation is required to use this feature,
shouldn't pg_basebackup implement that automation?
I have plenty of thoughts about the implementation as well, but I have a
lot on my plate right now and I don't have time to get into it.
I don't plan to stand in your way on this feature. I'm reviewing what
patches I can out of courtesy and to be sure that nothing adjacent to
your work is being affected. My apologies if my reviews are not meeting
your expectations, but I am contributing as my time constraints allow.
Regards,
-David
