pg_dump: Fix gzip compression of empty data

The pg_dump Compressor API has three basic callbacks - Allocate, Write
and End.  The gzip implementation (since e9960732a) wrongly assumed the
Write function would always be called, and deferred the initialization
of the internal compression system until the first such call.  But when
there's no data to compress (e.g. for empty LO), this would result in
not finalizing the compression state (because it was not actually
initialized), producing invalid dump.

Fixed by initializing the internal compression system in the Allocate
call, whenever the caller provides the Write.  For decompression the
state is not needed, so we leave the private_data member unpopulated.

Introduces a pg_dump TAP test compressing an empty large object.

This also rearranges the functions to their original order, to make
diffs against older code simpler to understand.  Finally, replace an
unreachable pg_fatal() with a simple assert check.

Reported-by: Justin Pryzby
Author: Justin Pryzby, Georgios Kokolatos
Reviewed-by: Georgios Kokolatos, Tomas Vondra
https://postgr.es/m/20230228235834.GC30529%40telsasoft.com

diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c
index cec0b41fcea4c39e1251d3348554d3c5e7c97695..63dfd9668c17ee687f0061e30f7edf6fff18ea00 100644 (file)
--- a/src/bin/pg_dump/compress_gzip.c
+++ b/src/bin/pg_dump/compress_gzip.c
@@ -33,8 +33,73 @@ typedef struct GzipCompressorState
 } GzipCompressorState;
 
 /* Private routines that support gzip compressed data I/O */
+static void DeflateCompressorInit(CompressorState *cs);
+static void DeflateCompressorEnd(ArchiveHandle *AH, CompressorState *cs);
+static void DeflateCompressorCommon(ArchiveHandle *AH, CompressorState *cs,
+                                   bool flush);
+static void EndCompressorGzip(ArchiveHandle *AH, CompressorState *cs);
+static void WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs,
+                                  const void *data, size_t dLen);
+static void ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs);
+
+static void
+DeflateCompressorInit(CompressorState *cs)
+{
+   GzipCompressorState *gzipcs;
+   z_streamp   zp;
+
+   gzipcs = (GzipCompressorState *) pg_malloc0(sizeof(GzipCompressorState));
+   zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream));
+   zp->zalloc = Z_NULL;
+   zp->zfree = Z_NULL;
+   zp->opaque = Z_NULL;
+
+   /*
+    * outsize is the buffer size we tell zlib it can output to.  We actually
+    * allocate one extra byte because some routines want to append a trailing
+    * zero byte to the zlib output.
+    */
+   gzipcs->outsize = DEFAULT_IO_BUFFER_SIZE;
+   gzipcs->outbuf = pg_malloc(gzipcs->outsize + 1);
+
+   /* -Z 0 uses the "None" compressor -- not zlib with no compression */
+   Assert(cs->compression_spec.level != 0);
+
+   if (deflateInit(zp, cs->compression_spec.level) != Z_OK)
+       pg_fatal("could not initialize compression library: %s", zp->msg);
+
+   /* Just be paranoid - maybe End is called after Start, with no Write */
+   zp->next_out = gzipcs->outbuf;
+   zp->avail_out = gzipcs->outsize;
+
+   /* Keep track of gzipcs */
+   cs->private_data = gzipcs;
+}
+
+static void
+DeflateCompressorEnd(ArchiveHandle *AH, CompressorState *cs)
+{
+   GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
+   z_streamp   zp;
+
+   zp = gzipcs->zp;
+   zp->next_in = NULL;
+   zp->avail_in = 0;
+
+   /* Flush any remaining data from zlib buffer */
+   DeflateCompressorCommon(AH, cs, true);
+
+   if (deflateEnd(zp) != Z_OK)
+       pg_fatal("could not close compression stream: %s", zp->msg);
+
+   pg_free(gzipcs->outbuf);
+   pg_free(gzipcs->zp);
+   pg_free(gzipcs);
+   cs->private_data = NULL;
+}
+
 static void
-DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush)
+DeflateCompressorCommon(ArchiveHandle *AH, CompressorState *cs, bool flush)
 {
    GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
    z_streamp   zp = gzipcs->zp;
@@ -78,27 +143,9 @@ DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush)
 static void
 EndCompressorGzip(ArchiveHandle *AH, CompressorState *cs)
 {
-   GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
-   z_streamp   zp;
-
-   if (gzipcs->zp)
-   {
-       zp = gzipcs->zp;
-       zp->next_in = NULL;
-       zp->avail_in = 0;
-
-       /* Flush any remaining data from zlib buffer */
-       DeflateCompressorGzip(AH, cs, true);
-
-       if (deflateEnd(zp) != Z_OK)
-           pg_fatal("could not close compression stream: %s", zp->msg);
-
-       pg_free(gzipcs->outbuf);
-       pg_free(gzipcs->zp);
-   }
-
-   pg_free(gzipcs);
-   cs->private_data = NULL;
+   /* If deflation was initialized, finalize it */
+   if (cs->private_data)
+       DeflateCompressorEnd(AH, cs);
 }
 
 static void
@@ -106,41 +153,10 @@ WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs,
                       const void *data, size_t dLen)
 {
    GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data;
-   z_streamp   zp;
-
-   if (!gzipcs->zp)
-   {
-       zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream));
-       zp->zalloc = Z_NULL;
-       zp->zfree = Z_NULL;
-       zp->opaque = Z_NULL;
-
-       /*
-        * outsize is the buffer size we tell zlib it can output to.  We
-        * actually allocate one extra byte because some routines want to
-        * append a trailing zero byte to the zlib output.
-        */
-       gzipcs->outsize = DEFAULT_IO_BUFFER_SIZE;
-       gzipcs->outbuf = pg_malloc(gzipcs->outsize + 1);
-
-       /*
-        * A level of zero simply copies the input one block at the time. This
-        * is probably not what the user wanted when calling this interface.
-        */
-       if (cs->compression_spec.level == 0)
-           pg_fatal("requested to compress the archive yet no level was specified");
-
-       if (deflateInit(zp, cs->compression_spec.level) != Z_OK)
-           pg_fatal("could not initialize compression library: %s", zp->msg);
-
-       /* Just be paranoid - maybe End is called after Start, with no Write */
-       zp->next_out = gzipcs->outbuf;
-       zp->avail_out = gzipcs->outsize;
-   }
 
    gzipcs->zp->next_in = (void *) unconstify(void *, data);
    gzipcs->zp->avail_in = dLen;
-   DeflateCompressorGzip(AH, cs, false);
+   DeflateCompressorCommon(AH, cs, false);
 }
 
 static void
@@ -214,17 +230,19 @@ void
 InitCompressorGzip(CompressorState *cs,
                   const pg_compress_specification compression_spec)
 {
-   GzipCompressorState *gzipcs;
-
    cs->readData = ReadDataFromArchiveGzip;
    cs->writeData = WriteDataToArchiveGzip;
    cs->end = EndCompressorGzip;
 
    cs->compression_spec = compression_spec;
 
-   gzipcs = (GzipCompressorState *) pg_malloc0(sizeof(GzipCompressorState));
-
-   cs->private_data = gzipcs;
+   /*
+    * If the caller has defined a write function, prepare the necessary
+    * state.  Note that if the data is empty, End may be called immediately
+    * after Init, without ever calling Write.
+    */
+   if (cs->writeF)
+       DeflateCompressorInit(cs);
 }
 
 

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index a22f27f300f06bf186f6c96807421aa9aa1f51d9..42215f82f7aa4894e856638a9c8afd189b99cbd6 100644 (file)
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1265,6 +1265,29 @@ my %tests = (
        },
    },
 
+   'LO create (with no data)' => {
+       create_sql =>
+         'SELECT pg_catalog.lo_create(0);',
+       regexp => qr/^
+           \QSELECT pg_catalog.lo_open\E \('\d+',\ \d+\);\n
+           \QSELECT pg_catalog.lo_close(0);\E
+           /xm,
+       like   => {
+           %full_runs,
+           column_inserts         => 1,
+           data_only              => 1,
+           inserts                => 1,
+           section_data           => 1,
+           test_schema_plus_large_objects => 1,
+       },
+       unlike => {
+           binary_upgrade         => 1,
+           no_large_objects       => 1,
+           schema_only            => 1,
+           section_pre_data       => 1,
+       },
+   },
+
    'COMMENT ON DATABASE postgres' => {
        regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,