Horton

Re: refactoring basebackup.c

Dilip Kumar <dilipbalaut@gmail.com>

View thread
From: Dilip Kumar <dilipbalaut@gmail.com>
To: tushar <tushar.ahuja@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2021-07-19T11:33:58Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Document BaseBackupSync and BaseBackupWrite wait events.

  2. Support long distance matching for zstd compression

  3. Fix possible NULL-pointer-deference in backup_compression.c.

  4. Allow parallel zstd compression when taking a base backup.

  5. Make PostgreSQL::Test::Cluster::run_log() return a useful value.

  6. Fix a few goofs in new backup compression code.

  7. Replace BASE_BACKUP COMPRESSION_LEVEL option with COMPRESSION_DETAIL.

  8. Add 'basebackup_to_shell' contrib module.

  9. Allow extensions to add new backup targets.

  10. Change HAVE_LIBLZ4 and HAVE_LIBZSTD tests to USE_LZ4 and USE_ZSTD.

  11. pg_basebackup: Clean up some bogus file extension tests.

  12. pg_basebackup: Avoid unclean failure with server-compression and -D -.

  13. Fix LZ4 tests for remaining buffer space.

  14. Add support for zstd base backup compression.

  15. pg_basebackup: Allow client-side LZ4 (de)compression.

  16. Add suport for server-side LZ4 base backup compression.

  17. Add min() and max() aggregates for xid8.

  18. Remove superfluous variable.

  19. pg_basebackup: Cleaner handling when compression is multiply specified.

  20. Allow server-side compression to be used with -Fp.

  21. pg_basebackup: Fix a couple of recently-introduced bugs.

  22. Tidy up a few cosmetic issues related to pg_basebackup.

  23. Server-side gzip compression.

  24. Unbreak pg_basebackup/t/010_pg_basebackup.pl on msys

  25. Suppress variable-set-but-not-used warning from clang 13.

  26. Extend the options of pg_basebackup to control compression

  27. Support base backup targets.

  28. Modify pg_basebackup to use a new COPY subprotocol for base backups.

  29. Document that tar archives are now properly terminated.

  30. Fix thinko in bbsink_throttle_manifest_contents.

  31. Have the server properly terminate tar archives.

  32. Minimal fix for unterminated tar archive problem.

  33. Introduce 'bbstreamer' abstraction to modularize pg_basebackup.

  34. Introduce 'bbsink' abstraction to modularize base backup code.

  35. Refactor basebackup.c's _tarWriteDir() function.

  36. Flexible options for CREATE_REPLICATION_SLOT.

  37. Flexible options for BASE_BACKUP.

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