Horton

Re: trying again to get incremental backup

David Steele <david@pgmasters.net>

View thread View on postgr.es
From: David Steele <david@pgmasters.net>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Andres Freund <andres@anarazel.de>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2023-10-20T15:30:40Z
Lists: pgsql-hackers 
On 10/19/23 16:00, Robert Haas wrote:
> 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.

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

Commits

  1. Minor fixes to pg_combinebackup and its documentation.

  2. Fix defects in PrepareForIncrementalBackup.

  3. Add WALSummarizerLock to wait_event_names.txt

  4. Initialize variable to placate compiler.

  5. Replace nonsense comment with a relevant one.

  6. Fix numerous typos in incremental backup commits.

  7. Add support for incremental backup.

  8. Add a new WAL summarizer process.

  9. Move src/bin/pg_verifybackup/parse_manifest.c into src/common.

  10. Fix brown paper bag bug in 5c47c6546c413d5eb51c1626070a807026e6139d.

  11. Rename pg_verifybackup's JsonManifestParseContext callback functions.

  12. Rename JsonManifestParseContext callbacks.

  13. Change how a base backup decides which files have checksums.

  14. Change struct tablespaceinfo's oid member from 'char *' to 'Oid'

  15. Refactor parse_filename_for_nontemp_relation to parse more.

  16. During online checkpoints, insert XLOG_CHECKPOINT_REDO at redo point.

  17. In basebackup.c, refactor to create read_file_data_into_buffer.

  18. In basebackup.c, refactor to create verify_page_checksum.

  19. Report syncscan position at end of scan.

  20. Exclude additional directories in pg_basebackup

  21. Add new JSON processing functions and parser API.