From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: tushar <tushar.ahuja@enterprisedb.com>
Subject: Re: refactoring basebackup.c
Date: Mon, 19 Jul 2021 11:51:44 -0700
Message-ID: <CE497CA6-D19C-4AF9-9954-4EEDBFDB65CD@enterprisedb.com> (raw)
In-Reply-To: <CA+TgmoYgVN=-Yoh71r3P9N7eKysd7_9b9s+1QFfFcs3w7Z-tig@mail.gmail.com>
References: <CA+TgmoZGwR=ZVWFeecncubEyPdwghnvfkkdBe9BLccLSiqdf9Q@mail.gmail.com>
<CA+TgmoZBqs7KkyBUFzXQaztuonKanuOV+f_7DYsHOpJs=0B2DA@mail.gmail.com>
<CA+TgmoY5zpS=gJj2O0PXi6YGao5YR89v0qN_xZzWNvff2Tug+Q@mail.gmail.com>
<58D2BD01-B541-4032-9C0B-B6143841E7ED@enterprisedb.com>
<CA+TgmoYgVN=-Yoh71r3P9N7eKysd7_9b9s+1QFfFcs3w7Z-tig@mail.gmail.com>
> On Jul 8, 2021, at 8:56 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> The interesting
> patches in terms of functionality are 0006 and 0007;
The difficulty in v3-0007 with pg_basebackup only knowing how to parse tar archives seems to be a natural consequence of not sufficiently abstracting out the handling of the tar format. If the bbsink and bbstreamer abstractions fully encapsulated a set of parsing callbacks, then pg_basebackup wouldn't contain things like:
streamer = bbstreamer_tar_parser_new(streamer);
but instead would use the parser callbacks without knowledge of whether they were parsing tar vs. cpio vs. whatever. It just seems really odd that pg_basebackup is using the extensible abstraction layer and then defeating the purpose by knowing too much about the format. It might even be a useful exercise to write cpio support into this patch set rather than waiting until v16, just to make sure the abstraction layer doesn't have tar-specific assumptions left over.
printf(_(" -F, --format=p|t output format (plain (default), tar)\n"));
printf(_(" -z, --gzip compress tar output\n"));
printf(_(" -Z, --compress=0-9 compress tar output with given compression level\n"));
This is the pre-existing --help output, not changed by your patch, but if you anticipate that other output formats will be supported in future releases, perhaps it's better not to write the --help output in such a way as to imply that -z and -Z are somehow connected with the choice of tar format? Would changing the --help now make for less confusion later? I'm just asking...
The new options to pg_basebackup should have test coverage in src/bin/pg_basebackup/t/010_pg_basebackup.pl, though I expect you are waiting to hammer out the interface before writing the tests.
> the rest is
> preparatory refactoring.
patch v3-0001:
The new function AppendPlainCommandOption writes too many spaces, which does no harm, but seems silly, resulting in lines like:
LOG: received replication command: BASE_BACKUP ( LABEL 'pg_basebackup base backup', PROGRESS, WAIT 0, MANIFEST 'yes')
patch v3-0003:
The introduction of the sink abstraction seems incomplete, as basebackup.c still has knowledge of things like tar headers. Calls like _tarWriteHeader(sink, ...) feel like an abstraction violation. I expected perhaps this would get addressed in later patches, but it doesn't.
+ * 'bbs_buffer' is the buffer into which data destined for the bbsink
+ * should be stored. It must be a multiple of BLCKSZ.
+ *
+ * 'bbs_buffer_length' is the allocated length of the buffer.
The length must be a multiple of BLCKSZ, not the pointer.
patch-v3-0005:
+ * 'copystream' sends a starts a single COPY OUT operation and transmits
too many verbs.
+ * Regardless of which method is used, we sent a result set with
"is used" vs. "sent" verb tense mismatch.
+ * So we only check it after the number of bytes sine the last check reaches
typo. s/sine/since/
- * (2) we need to inject backup_manifest or recovery configuration into it.
+ * (2) we need to inject backup_manifest or recovery configuration into
+ * it.
src/bin/pg_basebackup/pg_basebackup.c contains word wrap changes like the above which would better be left to a different commit, if done at all.
+ if (state.manifest_file !=NULL)
Need a space after !=
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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: pgsql-hackers@postgresql.org
Cc: mark.dilger@enterprisedb.com, robertmhaas@gmail.com, tushar.ahuja@enterprisedb.com
Subject: Re: refactoring basebackup.c
In-Reply-To: <CE497CA6-D19C-4AF9-9954-4EEDBFDB65CD@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox