Re: pg_waldump: support decoding of WAL inside tarfile - Mailing list pgsql-hackers

Everything's pushed, although it looks like it'll be a few hours
before we see results from batta or hachi.

In the meantime, I have one more change that I'd like to propose in
archive_waldump.c: I think we should drop read_archive_file's "count"
parameter and just always try to fill archive_read_buf completely,
as attached.  I don't believe that using a variable count is buying
anything.  I get that the idea was to not load more data than we have
to before init_archive_reader can identify the segment size and start
filtering data we don't need.  But I doubt that that helps a whole
lot: I think most of the cost in this pipeline is going into
decompression and tar format parsing, not into copying data into the
hashtable.  Also, I noticed while I was investigating the bugs we
just fixed that the tar file very likely contains a ton of other
data before the first WAL file.  In the regression test scenarios
it seems to reliably have all of the cluster's actual table data
ahead of the WAL directory.  By using a small read count, we are
slowing down scanning of all that data.  Maybe not by much, I've
not tried to measure it.  But I think the case for using a variable
count is very weak.

Even if you don't like that change, I think we should apply the
parts of the attached that change archive_read_buf_size into an
always-present, always-filled field of XLogDumpPrivate.  Having
its presence be dependent on USE_ASSERT_CHECKING is just asking
for trouble, ie different .c files having a different opinion
of whether it's there.

            regards, tom lane

diff --git a/src/bin/pg_waldump/archive_waldump.c b/src/bin/pg_waldump/archive_waldump.c
index 50a5eb426fe..96f4d94449f 100644
--- a/src/bin/pg_waldump/archive_waldump.c
+++ b/src/bin/pg_waldump/archive_waldump.c
@@ -89,7 +89,7 @@ typedef struct astreamer_waldump

 static ArchivedWALFile *get_archive_wal_entry(const char *fname,
                                               XLogDumpPrivate *privateInfo);
-static bool read_archive_file(XLogDumpPrivate *privateInfo, Size count);
+static bool read_archive_file(XLogDumpPrivate *privateInfo);
 static void setup_tmpwal_dir(const char *waldir);
 static void cleanup_tmpwal_dir_atexit(void);

@@ -156,14 +156,11 @@ init_archive_reader(XLogDumpPrivate *privateInfo,
     privateInfo->archive_streamer = streamer;

     /*
-     * Allocate a buffer for reading the archive file to facilitate content
-     * decoding; read requests must not exceed the allocated buffer size.
+     * Allocate a buffer for reading the archive file to begin content
+     * decoding.
      */
     privateInfo->archive_read_buf = pg_malloc(READ_CHUNK_SIZE);
-
-#ifdef USE_ASSERT_CHECKING
     privateInfo->archive_read_buf_size = READ_CHUNK_SIZE;
-#endif

     /*
      * Hash table storing WAL entries read from the archive with an arbitrary
@@ -182,7 +179,7 @@ init_archive_reader(XLogDumpPrivate *privateInfo,
      */
     while (entry == NULL)
     {
-        if (!read_archive_file(privateInfo, XLOG_BLCKSZ))
+        if (!read_archive_file(privateInfo))
             pg_fatal("could not find WAL in archive \"%s\"",
                      privateInfo->archive_name);

@@ -402,7 +399,7 @@ read_archive_wal_page(XLogDumpPrivate *privateInfo, XLogRecPtr targetPagePtr,
                          fname, privateInfo->archive_name,
                          (long long int) (count - nbytes),
                          (long long int) count);
-            if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
+            if (!read_archive_file(privateInfo))
                 pg_fatal("unexpected end of archive \"%s\" while reading \"%s\": read %lld of %lld bytes",
                          privateInfo->archive_name, fname,
                          (long long int) (count - nbytes),
@@ -523,7 +520,7 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
         /*
          * Read more data.  If we reach EOF, the desired file is not present.
          */
-        if (!read_archive_file(privateInfo, READ_CHUNK_SIZE))
+        if (!read_archive_file(privateInfo))
             pg_fatal("could not find WAL \"%s\" in archive \"%s\"",
                      fname, privateInfo->archive_name);
     }
@@ -533,10 +530,6 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
  * Reads a chunk from the archive file and passes it through the streamer
  * pipeline for decompression (if needed) and tar member extraction.
  *
- * count is the maximum amount to try to read this time.  Note that it's
- * measured in raw file bytes, and may have little to do with how much
- * comes out of decompression/extraction.
- *
  * Returns true if successful, false if there is no more data.
  *
  * Callers must be aware that a single call may trigger multiple callbacks
@@ -548,19 +541,17 @@ get_archive_wal_entry(const char *fname, XLogDumpPrivate *privateInfo)
  * within the same call.
  */
 static bool
-read_archive_file(XLogDumpPrivate *privateInfo, Size count)
+read_archive_file(XLogDumpPrivate *privateInfo)
 {
     int            rc;

-    /* The read request must not exceed the allocated buffer size. */
-    Assert(privateInfo->archive_read_buf_size >= count);
-
     /* Fail if we already reached EOF in a prior call. */
     if (privateInfo->archive_fd_eof)
         return false;

     /* Try to read some more data. */
-    rc = read(privateInfo->archive_fd, privateInfo->archive_read_buf, count);
+    rc = read(privateInfo->archive_fd, privateInfo->archive_read_buf,
+              privateInfo->archive_read_buf_size);
     if (rc < 0)
         pg_fatal("could not read file \"%s\": %m",
                  privateInfo->archive_name);
diff --git a/src/bin/pg_waldump/pg_waldump.h b/src/bin/pg_waldump/pg_waldump.h
index ca0dfd97168..bd46d14f3a8 100644
--- a/src/bin/pg_waldump/pg_waldump.h
+++ b/src/bin/pg_waldump/pg_waldump.h
@@ -39,10 +39,7 @@ typedef struct XLogDumpPrivate

     astreamer  *archive_streamer;
     char       *archive_read_buf;    /* Reusable read buffer for archive I/O */
-
-#ifdef USE_ASSERT_CHECKING
     Size        archive_read_buf_size;
-#endif

     /*
      * The buffer for the WAL file the archive streamer is currently reading,