Re: refactoring basebackup.c - Mailing list pgsql-hackers

On Fri, Jul 16, 2021 at 12:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 5:51 PM tushar <tushar.ahuja@enterprisedb.com> wrote:
> >
> > On 7/8/21 9:26 PM, Robert Haas wrote:
> > > Here at last is a new version.
> > Please refer this scenario ,where backup target using
> > --server-compression is closing the server
> > unexpectedly if we don't provide -no-manifest option
> >
> > [tushar@localhost bin]$ ./pg_basebackup --server-compression=gzip4  -t
> > server:/tmp/data_1  -Xnone
> > NOTICE:  WAL archiving is not enabled; you must ensure that all required
> > WAL segments are copied through other means to complete the backup
> > pg_basebackup: error: could not read COPY data: server closed the
> > connection unexpectedly
> >      This probably means the server terminated abnormally
> >      before or while processing the request.
> >
>
> I think the problem is that bbsink_gzip_end_archive() is not
> forwarding the end request to the next bbsink.  The attached patch so
> fix it.

I was going through the patch, I think the refactoring made the base
backup code really clean and readable.  I have a few minor
suggestions.

v3-0003

1.
+    Assert(sink->bbs_next != NULL);
+    bbsink_begin_archive(sink->bbs_next, gz_archive_name);

I have noticed that the interface for forwarding the request to next
bbsink is not uniform, for example bbsink_gzip_begin_archive() is
calling bbsink_begin_archive(sink->bbs_next, gz_archive_name); for
forwarding the request to next bbsink where as
bbsink_progress_begin_backup() is calling
bbsink_forward_begin_backup(sink); I think it will be good if we keep
the usage uniform.

2.
I have noticed that bbbsink_copytblspc_* are not forwarding the
request to the next sink, thats probably because we assume this should
always be the last sink.  I agree that its true for this patch but the
commit message of the patch says that in future this might change, so
wouldn't it be good to keep the interface generic? I mean
bbsink_copytblspc_new(), should take the next sink as an input and the
caller can pass it as NULL.  And the other apis can also try to
forward the request if next is not NULL?

3.
It would make more sense to order the function in
basebackup_progress.c same as done in other file i.e
bbsink_progress_begin_backup, bbsink_progress_archive_contents and
then bbsink_progress_end_archive, and this will also be in sync with
function pointer declaration in bbsink_ops.

v3-0005-
4.
+ *
+ * 'copystream' sends a starts a single COPY OUT operation and transmits
+ * all the archives and the manifest if present during the course of that

typo 'copystream' sends a starts a single COPY OUT -->  'copystream'
sends a single COPY OUT


--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com