Horton

Re: trying again to get incremental backup

Dilip Kumar <dilipbalaut@gmail.com>

View thread View on postgr.es
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2023-09-12T09:56:00Z
Lists: pgsql-hackers 
On Wed, Aug 30, 2023 at 9:20 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> Meanwhile, here's a rebased set of patches. The somewhat-primitive
> attempts at writing tests are in 0009, but they don't work, for the
> reasons explained above. I think I'd probably like to go ahead and
> commit 0001 and 0002 soon if there are no objections, since I think
> those are good refactorings independently of the rest of this.
>

I have started reading the patch today, I haven't yet completed one
pass but here are my comments in 0007

1.

+ BlockNumber relative_block_numbers[RELSEG_SIZE];

This is close to 400kB of memory, so I think it is better we palloc it
instead of keeping it in the stack.

2.
  /*
  * Try to parse the directory name as an unsigned integer.
  *
- * Tablespace directories should be positive integers that can
- * be represented in 32 bits, with no leading zeroes or trailing
+ * Tablespace directories should be positive integers that can be
+ * represented in 32 bits, with no leading zeroes or trailing
  * garbage. If we come across a name that doesn't meet those
  * criteria, skip it.

Unrelated code refactoring hunk

3.
+typedef struct
+{
+ const char *filename;
+ pg_checksum_context *checksum_ctx;
+ bbsink    *sink;
+ size_t bytes_sent;
+} FileChunkContext;

This structure is not used anywhere.

4.
+ * If the file is to be set incrementally, then num_incremental_blocks
+ * should be the number of blocks to be sent, and incremental_blocks

/If the file is to be set incrementally/If the file is to be sent incrementally

5.
- while (bytes_done < statbuf->st_size)
+ while (1)
  {
- size_t remaining = statbuf->st_size - bytes_done;
+ /*

I do not really like this change, because after removing this you have
put 2 independent checks for sending the full file[1] and sending it
incrementally[1].  Actually for sending incrementally
'statbuf->st_size' is computed from the 'num_incremental_blocks'
itself so why don't we keep this breaking condition in the while loop
itself?  So that we can avoid these two separate conditions.

[1]
+ /*
+ * If we've read the required number of bytes, then it's time to
+ * stop.
+ */
+ if (bytes_done >= statbuf->st_size)
+ break;

[2]
+ /*
+ * If we've read all the blocks, then it's time to stop.
+ */
+ if (ibindex >= num_incremental_blocks)
+ break;


6.
+typedef struct
+{
+ TimeLineID tli;
+ XLogRecPtr start_lsn;
+ XLogRecPtr end_lsn;
+} backup_wal_range;
+
+typedef struct
+{
+ uint32 status;
+ const char *path;
+ size_t size;
+} backup_file_entry;

Better we add some comments for these structures.

-- 
Regards,
Dilip Kumar
EnterpriseDB: 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.