PXB-3747 : Addressing review comments

https://perconadev.atlassian.net/browse/PXB-3747

Follow-up to the two commits on this branch that introduced per-datasink
metrics reporting and uncompressed-backup-size tracking.  Will be squashed
before merge.

Problem:
--------------

Review raised five issues on the initial split:

1. fs_support_punch_hole was left uninitialized for every non-local
   datasink.  ds_local_init() probes the filesystem and sets it, but
   every other init() allocated ds_ctxt_t with my_malloc() without
   MY_ZEROFILL, so the field held whichever bytes my_malloc returned.
   ds_create() previously masked this by unconditionally clobbering
   the field to false after init, which also discarded the local
   datasink's probe result (the regression we already fixed).
   That left xbstream - which natively encodes sparse chunks in its
   wire format and therefore does support punch-hole - advertising
   whatever garbage happened to be there.

2. local_close() writes one trailing zero byte to stamp out the
   final page of a sparse file, but it did not add that byte to
   ds_local_ctxt_t::bytes_written.  Backups with sparse files
   therefore under-reported backup_size by 1 byte per sparse file.

3. backup_copy.cc had a "Finsh" typo and an awkwardly reflowed doc
   comment above report_backup_size().

4. test/inc/common.sh::sum_file_bytes() ran find without a -d guard,
   so callers probing a directory that might not exist got a stray
   "No such file or directory" on stderr.

Fix/Implementation:
-------------------

1. Framework-field initialization:
   - ds_ctxt_t::pipe_ctxt gets an in-class default of nullptr (matches
     the existing fs_support_punch_hole = false default), so every
     new ds_ctxt_t site starts with a sane pipe_ctxt.
   - The sinks that allocate ds_ctxt_t via my_malloc (local, stdout,
     xbstream, buffer, tmpfile) now pass MY_FAE | MY_ZEROFILL, so
     those framework fields start at zero/false too.
   - xbstream_init() then sets fs_support_punch_hole = true because
     the xbstream wire format carries sparse chunks natively and can
     faithfully round-trip holes regardless of the extraction
     filesystem.
   - ds_create()'s normalization block is replaced with a short
     comment explaining the new contract: every init() is responsible
     for advertising its own capabilities; the framework no longer
     overrides them.

2. local_close() adds 1 to ds_local_ctxt_t::bytes_written right
   after the successful 1-byte trailing write, so backup_size stays
   authoritative for sparse files under --target-dir.

3. "Finsh" -> "Finish"; the report_backup_size() doc comment is
   reflowed so the closing parenthesis lands on the same line as
   the example it belongs to.

4. sum_file_bytes() early-returns 0 when its argument is not a
   directory, eliminating the stray find(1) error and matching the
   "0 if the directory is empty or does not exist" contract in the
   helper's own comment.

Test:
-----

- bld/ (Debug) rebuild clean.
- t/backup_size_basic.sh: passed.
- t/backup_size_compress.sh: passed.

Author: satya-bodapati

storage/innobase/xtrabackup/src/backup_copy.cc

@@ -1603,10 +1603,9 @@ and the compression ratio) to the error log.  A successful backup run
 must have produced on-disk output, so the leaf counter must be
 non-zero; under --compress at least one top-level ds_open_track_uncomp()
 must have enabled an uncomp_bytes counter, so xb_uncomp_bytes_counter
-must be non-zero too.  A
-zero value therefore indicates a silent reporting bug: assert in
-debug, warn and skip in release to avoid emitting misleading numbers
-(e.g. "Compression ratio: inf"). */
+must be non-zero too.  A zero value therefore indicates a silent
+reporting bug: assert in debug, warn and skip in release to avoid
+emitting misleading numbers (e.g. "Compression ratio: inf"). */
 static void report_backup_size() {
   const unsigned long long backup_size = get_final_backup_size();
 
@@ -1643,7 +1642,7 @@ static void report_backup_size() {
              << ratio << "x";
 }
 
-/* Finsh the backup. Release all locks. Write down backup metadata.
+/* Finish the backup. Release all locks. Write down backup metadata.
 @return true if success. */
 bool backup_finish(Backup_context &context) {
   /* release all locks */

storage/innobase/xtrabackup/src/datasink.cc

@@ -96,12 +96,13 @@ ds_ctxt_t *ds_create(const char *root, ds_type_t type) {
   ctxt = ds->init(root);
   if (ctxt != NULL) {
     ctxt->datasink = ds;
-    /* Per-datasink init() allocates ds_ctxt_t with my_malloc(), which
-    does not zero memory.  Normalize framework-level fields the
-    per-sink init() is not expected to set.  Do NOT touch fields that
-    a leaf's init() is responsible for (e.g. fs_support_punch_hole is
-    probed by local_init() and must be preserved). */
-    ctxt->pipe_ctxt = nullptr;
+    /* Every per-datasink init() is expected to leave the framework-level
+    fields of ds_ctxt_t in a well-defined state: ds_ctxt_t's in-class
+    initializers cover the `new ds_ctxt_t` call sites, and the `my_malloc`
+    call sites use MY_ZEROFILL.  pipe_ctxt therefore starts NULL, and
+    fs_support_punch_hole starts false; each sink then advertises its
+    own capability (local probes the filesystem, xbstream sets it to
+    true because its wire format carries sparse chunks natively). */
   } else {
     msg("Error: failed to initialize datasink.\n");
     exit(EXIT_FAILURE);

storage/innobase/xtrabackup/src/datasink.h

@@ -71,7 +71,12 @@ typedef struct ds_ctxt {
   datasink_t *datasink;
   char *root;
   void *ptr;
-  struct ds_ctxt *pipe_ctxt;
+  /* pipe_ctxt chains a datasink to the next stage downstream.
+  Default to nullptr so ds_leaf() never walks off into garbage when a
+  datasink init() forgets to set it (e.g. terminal sinks that do not
+  forward).  The my_malloc-based inits use MY_ZEROFILL to match, and
+  the `new ds_ctxt_t` inits rely on this in-class initializer. */
+  struct ds_ctxt *pipe_ctxt = nullptr;
   bool fs_support_punch_hole = false;
 } ds_ctxt_t;
 

storage/innobase/xtrabackup/src/ds_buffer.cc

@@ -66,9 +66,13 @@ static ds_ctxt_t *buffer_init(const char *root) {
   ds_ctxt_t *ctxt;
   ds_buffer_ctxt_t *buffer_ctxt;
 
-  ctxt = static_cast<ds_ctxt *>(
-      my_malloc(PSI_NOT_INSTRUMENTED,
-                sizeof(ds_ctxt_t) + sizeof(ds_buffer_ctxt_t), MYF(MY_FAE)));
+  /* MY_ZEROFILL so framework-level fields (pipe_ctxt, fs_support_punch_hole)
+  start at well-defined defaults.  buffer is a pass-through wrapper that
+  forwards to its pipe_ctxt; it owns no filesystem or stream of its own,
+  so fs_support_punch_hole stays false. */
+  ctxt = static_cast<ds_ctxt *>(my_malloc(
+      PSI_NOT_INSTRUMENTED, sizeof(ds_ctxt_t) + sizeof(ds_buffer_ctxt_t),
+      MYF(MY_FAE | MY_ZEROFILL)));
   buffer_ctxt = (ds_buffer_ctxt_t *)(ctxt + 1);
   buffer_ctxt->buffer_size = DS_DEFAULT_BUFFER_SIZE;
 

storage/innobase/xtrabackup/src/ds_local.cc

@@ -105,8 +105,12 @@ static ds_ctxt_t *local_init(const char *root) {
     return NULL;
   }
 
-  ctxt = static_cast<ds_ctxt_t *>(
-      my_malloc(PSI_NOT_INSTRUMENTED, sizeof(ds_ctxt_t), MYF(MY_FAE)));
+  /* MY_ZEROFILL so framework-level fields (pipe_ctxt, fs_support_punch_hole)
+  start at well-defined defaults; is_fallocate_punch_hole_supported() below
+  then flips fs_support_punch_hole on if the target filesystem supports
+  FALLOC_FL_PUNCH_HOLE, leaving it false otherwise. */
+  ctxt = static_cast<ds_ctxt_t *>(my_malloc(
+      PSI_NOT_INSTRUMENTED, sizeof(ds_ctxt_t), MYF(MY_FAE | MY_ZEROFILL)));
 
   ctxt->ptr = new ds_local_ctxt_t{};
   ctxt->root = my_strdup(PSI_NOT_INSTRUMENTED, root, MYF(MY_FAE));
@@ -241,6 +245,12 @@ static int local_close(ds_file_t *file) {
     if (rc != 0) {
       return 1;
     }
+    /* The trailing-hole fix above emits exactly one byte to the leaf.  Keep
+    bytes_written authoritative for backup_size by accounting for it here;
+    otherwise sparse files under --target-dir would under-report by 1 byte
+    per sparse file. */
+    auto local_ctxt = static_cast<ds_local_ctxt_t *>(file->ctxt->ptr);
+    local_ctxt->bytes_written.fetch_add(1, std::memory_order_relaxed);
   }
 
   my_free(file);

storage/innobase/xtrabackup/src/ds_stdout.cc

@@ -52,8 +52,11 @@ datasink_t datasink_stdout = {
 static ds_ctxt_t *stdout_init(const char *root) {
   ds_ctxt_t *ctxt;
 
-  ctxt = static_cast<ds_ctxt_t *>(
-      my_malloc(PSI_NOT_INSTRUMENTED, sizeof(ds_ctxt_t), MYF(MY_FAE)));
+  /* MY_ZEROFILL so framework-level fields (pipe_ctxt, fs_support_punch_hole)
+  start at their well-defined zero/false default; stdout does not write to a
+  filesystem, so it leaves fs_support_punch_hole = false. */
+  ctxt = static_cast<ds_ctxt_t *>(my_malloc(
+      PSI_NOT_INSTRUMENTED, sizeof(ds_ctxt_t), MYF(MY_FAE | MY_ZEROFILL)));
 
   ctxt->ptr = new ds_stdout_ctxt_t{};
   ctxt->root = my_strdup(PSI_NOT_INSTRUMENTED, root, MYF(MY_FAE));

storage/innobase/xtrabackup/src/ds_tmpfile.cc

@@ -62,9 +62,13 @@ static ds_ctxt_t *tmpfile_init(const char *root) {
   ds_ctxt_t *ctxt;
   ds_tmpfile_ctxt_t *tmpfile_ctxt;
 
-  ctxt = static_cast<ds_ctxt_t *>(
-      my_malloc(PSI_NOT_INSTRUMENTED,
-                sizeof(ds_ctxt_t) + sizeof(ds_tmpfile_ctxt_t), MYF(MY_FAE)));
+  /* MY_ZEROFILL so framework-level fields (pipe_ctxt, fs_support_punch_hole)
+  start at well-defined defaults.  tmpfile stages bytes in a scratch file
+  that is later copied into pipe_ctxt; it does not own the final sink and
+  leaves fs_support_punch_hole at false. */
+  ctxt = static_cast<ds_ctxt_t *>(my_malloc(
+      PSI_NOT_INSTRUMENTED, sizeof(ds_ctxt_t) + sizeof(ds_tmpfile_ctxt_t),
+      MYF(MY_FAE | MY_ZEROFILL)));
   tmpfile_ctxt = (ds_tmpfile_ctxt_t *)(ctxt + 1);
   tmpfile_ctxt->file_list = NULL;
   ctxt->ptr = tmpfile_ctxt;

storage/innobase/xtrabackup/src/ds_xbstream.cc

@@ -86,11 +86,16 @@ static ds_ctxt_t *xbstream_init(const char *root __attribute__((unused))) {
   ds_parallel_stream_ctxt_t *parallel_stream_ctxt =
       new ds_parallel_stream_ctxt_t;
 
+  /* MY_ZEROFILL so framework-level fields (pipe_ctxt, fs_support_punch_hole)
+  start at well-defined defaults; we then flip fs_support_punch_hole to true
+  below because the xbstream wire format encodes sparse chunks natively and
+  can faithfully round-trip holes regardless of the extraction filesystem. */
   ctxt = static_cast<ds_ctxt_t *>(
       my_malloc(PSI_NOT_INSTRUMENTED,
                 sizeof(ds_ctxt_t) + sizeof(ds_parallel_stream_ctxt_t) +
                     (sizeof(ds_stream_ctxt_t) * (xtrabackup_fifo_streams + 1)),
-                MYF(MY_FAE)));
+                MYF(MY_FAE | MY_ZEROFILL)));
+  ctxt->fs_support_punch_hole = true;
 
   for (uint i = 0; i < xtrabackup_fifo_streams; i++) {
     ds_stream_ctxt_t *stream_ctxt = new ds_stream_ctxt_t;

storage/innobase/xtrabackup/test/inc/common.sh

@@ -109,7 +109,12 @@ file_size() { stat -c '%s' "$1"; }
 # sum_file_bytes <dir>
 #   Echo the sum of the sizes of every regular file under <dir>
 #   (recursive).  0 if the directory is empty or does not exist.
-sum_file_bytes() { find "$1" -type f -printf '%s\n' | awk '{s+=$1} END{print s+0}'; }
+#   The explicit -d guard keeps find from spraying a "No such file"
+#   message to stderr when callers probe a path that may be absent.
+sum_file_bytes() {
+  [ -d "$1" ] || { echo 0; return; }
+  find "$1" -type f -printf '%s\n' | awk '{s+=$1} END{print s+0}'
+}
 
 # find_info_file <dir>
 #   Locate xtrabackup_info (or one of its compressed/encrypted variants)