Horton

Re: trying again to get incremental backup

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Andres Freund <andres@anarazel.de>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2023-11-16T19:36:11Z
Lists: pgsql-hackers

Attachments

On Thu, Nov 16, 2023 at 12:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> So, wal_summary is no longer turned on by default, I think following a
> comment from Peter E.  I think this is a good decision, as we're only
> going to need them on servers from which incremental backups are going
> to be taken, which is a strict subset of all servers; and furthermore,
> people that need them are going to realize that very easily, while if we
> went the other around most people would not realize that they need to
> turn them off to save some resource consumption.
>
> Granted, the amount of resources additionally used is probably not very
> big.  But since it can be changed with a reload not restart, it doesn't
> seem problematic.

Yeah. I meant to say that I'd changed that for that reason, but in the
flurry of new versions I omitted to do so.

> ... oh, I just noticed that this patch now fails to compile because of
> the MemoryContextResetAndDeleteChildren removal.

Fixed.

> (Typo in the pg_walsummary manpage: "since WAL summary files primary
> exist" -> "primarily")

This, too.

> I think it should default to off in primary and standby, and the user
> has to enable it in whichever server they want to take backups from.

Yeah, that's how it works currently.

> I don't understand this point.  Currently, the protocol is that
> UPLOAD_MANIFEST is used to send the manifest prior to requesting the
> backup.  You seem to be saying that you're thinking of removing support
> for UPLOAD_MANIFEST and instead just give the LSN as an option to the
> BASE_BACKUP command?

I don't think I'd want to do exactly that, because then you could only
send one LSN, and I do think we want to send a set of LSN ranges with
the corresponding TLI for each. I was thinking about dumping
UPLOAD_MANIFEST and instead having a command like:

INCREMENTAL_WAL_RANGE 1 2/462AC48 2/462C698

The client would execute this command one or more times before
starting an incremental backup.

> I propose to keep the door open for that binary doing other things that
> dumping the files as text.  So add a command argument, which currently
> can only be "dump", to allow the command do other things later if
> needed.  (For example, remove files from a server on which summarize_wal
> has been turned off; or perhaps remove files that are below some LSN.)

I don't like that very much. That sounds like one of those
forward-compatibility things that somebody designs and then nothing
ever happens and ten years later you still have an ugly wart.

My theory is that these files are going to need very little
management. In general, they're small; if you never removed them, it
probably wouldn't hurt, or at least, not for a long time. As to
specific use cases, if you want to remove files from a server on which
summarize_wal has been turned off, you can just use rm. Removing files
from before a certain LSN would probably need a bit of scripting, but
only a bit. Conceivably we could provide something like that in core,
but it doesn't seem necessary, and it also seems to me that we might
do well to include that in pg_archivecleanup rather than in
pg_walsummary.

Here's a new version. Changes:

- Add preparatory renaming patches to the series.
- Rename wal_summarize_keep_time to wal_summary_keep_time.
- Change while (true) to while (1).
- Typo fixes.
- Fix incorrect assertion in summarizer_read_local_xlog_page; this
could cause occasional regression test failures in 004_pg_xlog_symlink
and 009_growing_files.
- Zero-initialize BlockRefTableKey variables.
- Replace a couple instances of pathbuf + basepathlen + 1 with tarfilename.
- Add const to path argument of GetFileBackupMethod.
- Avoid setting output parameters of GetFileBackupMethod unless the
return value is BACK_UP_FILE_INCREMENTALLY.
- In GetFileBackupMethod, postpone qsorting block numbers slightly.
- Define INCREMENTAL_PREFIX_LENGTH using sizeof(), because that should
hopefully work everywhere and the StaticAssertStmt that checks the
value of this doesn't work on Windows.
- Change MemoryContextResetAndDeleteChildren to MemoryContextReset.

--
Robert Haas
EDB: http://www.enterprisedb.com

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.