Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: refactoring basebackup.c
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CA+TgmoYgVN=-Yoh71r3P9N7eKysd7_9b9s+1QFfFcs3w7Z-tig@mail.gmail.com>
Date: Mon, 19 Jul 2021 11:51:44 -0700
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 tushar <tushar.ahuja@enterprisedb.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <CE497CA6-D19C-4AF9-9954-4EEDBFDB65CD@enterprisedb.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>
To: Robert Haas <robertmhaas@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/CE497CA6-D19C-4AF9-9954-4EEDBFDB65CD%40enterprisedb.com>
Precedence: bulk


> On Jul 8, 2021, at 8:56 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>=20
> 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 =3D 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=3Dp|t       output format (plain (default), =
tar)\n"));

    printf(_("  -z, --gzip             compress tar output\n"));
    printf(_("  -Z, --compress=3D0-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 !=3DNULL)

Need a space after !=3D


=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company