public inbox for [email protected]  

help / color / mirror / Atom feed

From: Dilip Kumar <[email protected]>
To: Robert Haas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: trying again to get incremental backup
Date: Tue, 12 Sep 2023 15:26:00 +0530
Message-ID: <CAFiTN-sx4JS-2tKe-Vv8otWHAp3MaSTt678a7d4EdaSzBJgpBg@mail.gmail.com> (raw)
In-Reply-To: <CA+TgmoZoHNqx=zconLLbV+8=gNhv7g5ocXd-Mee97wvTkKc-6A@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>

On Wed, Aug 30, 2023 at 9:20 PM Robert Haas <[email protected]> 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

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]
  Subject: Re: trying again to get incremental backup
  In-Reply-To: <CAFiTN-sx4JS-2tKe-Vv8otWHAp3MaSTt678a7d4EdaSzBJgpBg@mail.gmail.com>

* 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