public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Steele <[email protected]>
To: Robert Haas <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: trying again to get incremental backup
Date: Fri, 20 Oct 2023 11:30:40 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmobfDSutwXQRKgJvrZCyXY6xyM74FX9U_U2W4H-SaQU-UQ@mail.gmail.com>
References: <CA+TgmoYOYZfMCyOXFyC-P+-mdrZqm5pP2N7S-r0z3_402h9rsA@mail.gmail.com>
<[email protected]>
<CA+TgmoaF-TnXK9D5m3m1pTj6p5u0DiyzKuPZM1pqPGKzXhQw1w@mail.gmail.com>
<[email protected]>
<CA+TgmoaTZPKzcTW0pf30n+_uQtrEKRWB8qoOgkmnVh+QK2Wn_A@mail.gmail.com>
<[email protected]>
<CA+TgmoZoHNqx=zconLLbV+8=gNhv7g5ocXd-Mee97wvTkKc-6A@mail.gmail.com>
<CAFiTN-sx4JS-2tKe-Vv8otWHAp3MaSTt678a7d4EdaSzBJgpBg@mail.gmail.com>
<CA+Tgmoa_Gq2Qc9dh-WiZEUyF-nk3XULuQR-zFY2vOGNWRCZjkQ@mail.gmail.com>
<CA+TgmoYw=8CnKEPQX9qk2estSTCaB7OGcnVM8oa-hAePG2O4Yw@mail.gmail.com>
<CA+TgmoZNVeBzoqDL8xvr-nkaepq815jtDR4nJzPew7=3iEuM1g@mail.gmail.com>
<[email protected]>
<CA+TgmobfDSutwXQRKgJvrZCyXY6xyM74FX9U_U2W4H-SaQU-UQ@mail.gmail.com>
On 10/19/23 16:00, Robert Haas wrote:
> On Thu, Oct 19, 2023 at 3:18 PM David Steele <[email protected]> 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
view thread (22+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: trying again to get incremental backup
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox