------- Original Message -------
On Thursday, March 16th, 2023 at 10:20 PM, Tomas Vondra
<tomas.von...@enterprisedb.com> wrote:
>
>
>
>
> On 3/16/23 18:04, gkokola...@pm.me wrote:
>
> > ------- Original Message -------
> > On Tuesday, March 14th, 2023 at 4:32 PM, Tomas Vondra
> > tomas.von...@enterprisedb.com wrote:
> >
> > > On 3/14/23 16:18, gkokola...@pm.me wrote:
> > >
> > > > ...> Would you mind me trying to come with a patch to address your
> > > > points?
> > >
> > > That'd be great, thanks. Please keep it split into smaller patches - two
> > > might work, with one patch for "cosmetic" changes and the other tweaking
> > > the API error-handling stuff.
> >
> > Please find attached a set for it. I will admit that the splitting in the
> > series might not be ideal and what you requested. It is split on what
> > seemed as a logical units. Please advice how a better split can look like.
> >
> > 0001 is unifying types and return values on the API
> > 0002 is addressing the constant definitions
> > 0003 is your previous 0004 adding comments
>
>
> Thanks. I think the split seems reasonable - the goal was to not mix
> different changes, and from that POV it works.
>
> I'm not sure I understand the Gzip_read/Gzip_write changes in 0001. I
> mean, gzread/gzwrite returns int, so how does renaming the size_t
> variable solve the issue of negative values for errors? I mean, this
>
> - size_t ret;
> + size_t gzret;
>
> - ret = gzread(gzfp, ptr, size);
> + gzret = gzread(gzfp, ptr, size);
>
> means we still lost the information gzread() returned a negative value,
> no? We'll still probably trigger an error, but it's a bit weird.
You are obviously correct. My bad, I miss-read the return type of gzread().
Please find an amended version attached.
> Unless I'm missing something, if gzread() ever returns -1 or some other
> negative error value, we'll cast it to size_t, while condition will
> evaluate to "true" and we'll happily chew on some random chunk of data.
>
> So the confusion is (at least partially) a preexisting issue ...
>
> For gzwrite() it seems to be fine, because that only returns 0 on error.
> OTOH it's defined to take 'int size' but then we happily pass size_t
> values to it.
>
> As I wrote earlier, this apparently assumes we never need to deal with
> buffers larger than int, and I don't think we have the ambition to relax
> that (I'm not sure it's even needed / possible).
Agreed.
> I see the read/write functions are now defined as int, but we only ever
> return 0/1 from them, and then interpret that as bool. Why not to define
> it like that? I don't think we need to adhere to the custom that
> everything returns "int". This is an internal API. Or if we want to
> stick to int, I'd define meaningful "nice" constants for 0/1.
The return types are now booleans and the callers have been made aware.
> 0002 seems fine to me. I see you've ditched the idea of having two
> separate buffers, and replaced them with DEFAULT_IO_BUFFER_SIZE. Fine
> with me, although I wonder if this might have negative impact on
> performance or something (but I doubt that).
>
I doubt that too. Thank you.
> 0003 seems fine too.
Thank you.
> > As far as the error handling is concerned, you had said upthread:
> >
> > > I think the right approach is to handle all library errors and not just
> > > let them through. So Gzip_write() needs to check the return value, and
> > > either call pg_fatal() or translate it to an error defined by the API.
> >
> > While working on it, I thought it would be clearer and more consistent
> > for the pg_fatal() to be called by the caller of the individual functions.
> > Each individual function can keep track of the specifics of the error
> > internally. Then the caller upon detecting that there was an error by
> > checking the return value, can call pg_fatal() with a uniform error
> > message and then add the specifics by calling the get_error_func().
>
>
> I agree it's cleaner the way you did it.
>
> I was thinking that with each compression function handling error
> internally, the callers would not need to do that. But I haven't
> realized there's logic to detect ENOSPC and so on, and we'd need to
> duplicate that in every compression func.
>
If you agree, I can prepare a patch to improve on the error handling
aspect of the API as a separate thread, since here we are trying to
focus on correctness.
Cheers,
//Georgios
>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
From eeac82b647dc4021e1dcf22d8cc59840fbde8847 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos <gkokola...@pm.me>
Date: Fri, 17 Mar 2023 15:29:05 +0000
Subject: [PATCH v3 3/3] Improve compress_lz4 documentation.
Author: Tomas Vondra
---
src/bin/pg_dump/compress_lz4.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 2f3e552f51..fc2f4e116d 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -185,12 +185,15 @@ LZ4File_get_error(CompressFileHandle *CFH)
}
/*
- * Prepare an already alloc'ed LZ4File struct for subsequent calls.
+ * Prepare an already alloc'ed LZ4File struct for subsequent calls (either
+ * compression or decompression).
*
- * It creates the necessary contexts for the operations. When compressing,
- * it additionally writes the LZ4 header in the output stream.
+ * It creates the necessary contexts for the operations. When compressing data
+ * (indicated by compressing=true), it additionally writes the LZ4 header in the
+ * output stream.
*
- * Returns true on success and false on error.
+ * Returns true on success. In case of a failure returns false, and stores the
+ * error code in fs->errcode.
*/
static bool
LZ4File_init(LZ4File *fs, int size, bool compressing)
@@ -203,9 +206,15 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
fs->compressing = compressing;
fs->inited = true;
+ /* When compressing, write LZ4 header to the output stream. */
if (fs->compressing)
{
fs->buflen = LZ4F_compressBound(DEFAULT_IO_BUFFER_SIZE, &fs->prefs);
+
+ /*
+ * LZ4F_compressBegin requires a buffer that is greater or equal to
+ * LZ4F_HEADER_SIZE_MAX. Verify that the requirement is met.
+ */
if (fs->buflen < LZ4F_HEADER_SIZE_MAX)
fs->buflen = LZ4F_HEADER_SIZE_MAX;
@@ -255,9 +264,12 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
/*
* Read already decompressed content from the overflow buffer into 'ptr' up to
* 'size' bytes, if available. If the eol_flag is set, then stop at the first
- * occurrence of the new line char prior to 'size' bytes.
+ * occurrence of the newline char prior to 'size' bytes.
*
* Any unread content in the overflow buffer is moved to the beginning.
+ *
+ * Returns the number of bytes read from the overflow buffer (and copied into
+ * the 'ptr' buffer), or 0 if the overflow buffer is empty.
*/
static int
LZ4File_read_overflow(LZ4File *fs, void *ptr, int size, bool eol_flag)
@@ -297,6 +309,9 @@ LZ4File_read_overflow(LZ4File *fs, void *ptr, int size, bool eol_flag)
* at an overflow buffer within LZ4File. Of course, when the function is
* called, it will first try to consume any decompressed content already
* present in the overflow buffer, before decompressing new content.
+ *
+ * Returns the number of bytes of decompressed data copied into the ptr
+ * buffer, or -1 in case of error.
*/
static int
LZ4File_read_internal(LZ4File *fs, void *ptr, int ptrsize, bool eol_flag)
--
2.34.1
From 4d8643e3e0081973c001e56cc9a5ecd4ff0083b4 Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos <gkokola...@pm.me>
Date: Fri, 17 Mar 2023 15:06:22 +0000
Subject: [PATCH v3 2/3] Clean up constants in pg_dump's compression API.
Prior to the introduction of the API, pg_dump would use the ZLIB_[IN|OUT]_SIZE
constants to handle buffer sizes throughout. This behaviour is confusing after
the introduction of the API. Ammend it by introducing a DEFAULT_IO_BUFFER_SIZE
constant to use when appropriate while giving the opportunity to specific
compression implementations to use their own.
With the help and guidance of Tomas Vondra.
---
src/bin/pg_dump/compress_gzip.c | 22 +++++++++++-----------
src/bin/pg_dump/compress_io.h | 5 ++---
src/bin/pg_dump/compress_lz4.c | 11 ++++-------
src/bin/pg_dump/compress_none.c | 4 ++--
src/bin/pg_dump/pg_backup_directory.c | 4 ++--
5 files changed, 21 insertions(+), 25 deletions(-)
diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c
index 79ed2cf371..61d15907eb 100644
--- a/src/bin/pg_dump/compress_gzip.c
+++ b/src/bin/pg_dump/compress_gzip.c
@@ -120,8 +120,8 @@ WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs,
* actually allocate one extra byte because some routines want to
* append a trailing zero byte to the zlib output.
*/
- gzipcs->outbuf = pg_malloc(ZLIB_OUT_SIZE + 1);
- gzipcs->outsize = ZLIB_OUT_SIZE;
+ 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
@@ -158,10 +158,10 @@ ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
zp->zfree = Z_NULL;
zp->opaque = Z_NULL;
- buf = pg_malloc(ZLIB_IN_SIZE);
- buflen = ZLIB_IN_SIZE;
+ buflen = DEFAULT_IO_BUFFER_SIZE;
+ buf = pg_malloc(buflen);
- out = pg_malloc(ZLIB_OUT_SIZE + 1);
+ out = pg_malloc(DEFAULT_IO_BUFFER_SIZE + 1);
if (inflateInit(zp) != Z_OK)
pg_fatal("could not initialize compression library: %s",
@@ -176,14 +176,14 @@ ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
while (zp->avail_in > 0)
{
zp->next_out = (void *) out;
- zp->avail_out = ZLIB_OUT_SIZE;
+ zp->avail_out = DEFAULT_IO_BUFFER_SIZE;
res = inflate(zp, 0);
if (res != Z_OK && res != Z_STREAM_END)
pg_fatal("could not uncompress data: %s", zp->msg);
- out[ZLIB_OUT_SIZE - zp->avail_out] = '\0';
- ahwrite(out, 1, ZLIB_OUT_SIZE - zp->avail_out, AH);
+ out[DEFAULT_IO_BUFFER_SIZE - zp->avail_out] = '\0';
+ ahwrite(out, 1, DEFAULT_IO_BUFFER_SIZE - zp->avail_out, AH);
}
}
@@ -192,13 +192,13 @@ ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs)
while (res != Z_STREAM_END)
{
zp->next_out = (void *) out;
- zp->avail_out = ZLIB_OUT_SIZE;
+ zp->avail_out = DEFAULT_IO_BUFFER_SIZE;
res = inflate(zp, 0);
if (res != Z_OK && res != Z_STREAM_END)
pg_fatal("could not uncompress data: %s", zp->msg);
- out[ZLIB_OUT_SIZE - zp->avail_out] = '\0';
- ahwrite(out, 1, ZLIB_OUT_SIZE - zp->avail_out, AH);
+ out[DEFAULT_IO_BUFFER_SIZE - zp->avail_out] = '\0';
+ ahwrite(out, 1, DEFAULT_IO_BUFFER_SIZE - zp->avail_out, AH);
}
if (inflateEnd(zp) != Z_OK)
diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index 7c2f9b5668..fd8752db0d 100644
--- a/src/bin/pg_dump/compress_io.h
+++ b/src/bin/pg_dump/compress_io.h
@@ -17,9 +17,8 @@
#include "pg_backup_archiver.h"
-/* Initial buffer sizes used in zlib compression. */
-#define ZLIB_OUT_SIZE 4096
-#define ZLIB_IN_SIZE 4096
+/* Default size used for IO buffers */
+#define DEFAULT_IO_BUFFER_SIZE 4096
extern char *supports_compression(const pg_compress_specification compression_spec);
diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 278f262162..2f3e552f51 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -20,9 +20,6 @@
#include <lz4.h>
#include <lz4frame.h>
-#define LZ4_OUT_SIZE (4 * 1024)
-#define LZ4_IN_SIZE (16 * 1024)
-
/*
* LZ4F_HEADER_SIZE_MAX first appeared in v1.7.5 of the library.
* Redefine it for installations with a lesser version.
@@ -57,7 +54,7 @@ ReadDataFromArchiveLZ4(ArchiveHandle *AH, CompressorState *cs)
size_t buflen;
size_t cnt;
- buflen = LZ4_IN_SIZE;
+ buflen = DEFAULT_IO_BUFFER_SIZE;
buf = pg_malloc(buflen);
decbuf = pg_malloc(buflen);
@@ -208,7 +205,7 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
if (fs->compressing)
{
- fs->buflen = LZ4F_compressBound(LZ4_IN_SIZE, &fs->prefs);
+ fs->buflen = LZ4F_compressBound(DEFAULT_IO_BUFFER_SIZE, &fs->prefs);
if (fs->buflen < LZ4F_HEADER_SIZE_MAX)
fs->buflen = LZ4F_HEADER_SIZE_MAX;
@@ -244,7 +241,7 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
return false;
}
- fs->buflen = size > LZ4_OUT_SIZE ? size : LZ4_OUT_SIZE;
+ fs->buflen = Max(size, DEFAULT_IO_BUFFER_SIZE);
fs->buffer = pg_malloc(fs->buflen);
fs->overflowalloclen = fs->buflen;
@@ -423,7 +420,7 @@ LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH)
while (remaining > 0)
{
- int chunk = remaining < LZ4_IN_SIZE ? remaining : LZ4_IN_SIZE;
+ int chunk = Min(remaining, DEFAULT_IO_BUFFER_SIZE);
remaining -= chunk;
diff --git a/src/bin/pg_dump/compress_none.c b/src/bin/pg_dump/compress_none.c
index 18f3514d11..736a7957bc 100644
--- a/src/bin/pg_dump/compress_none.c
+++ b/src/bin/pg_dump/compress_none.c
@@ -33,8 +33,8 @@ ReadDataFromArchiveNone(ArchiveHandle *AH, CompressorState *cs)
char *buf;
size_t buflen;
- buf = pg_malloc(ZLIB_OUT_SIZE);
- buflen = ZLIB_OUT_SIZE;
+ buflen = DEFAULT_IO_BUFFER_SIZE;
+ buf = pg_malloc(buflen);
while ((cnt = cs->readF(AH, &buf, &buflen)))
{
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index 525dbf9bf0..abaaa3b10e 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -394,8 +394,8 @@ _PrintFileData(ArchiveHandle *AH, char *filename)
if (!CFH)
pg_fatal("could not open input file \"%s\": %m", filename);
- buf = pg_malloc(ZLIB_OUT_SIZE);
- buflen = ZLIB_OUT_SIZE;
+ buflen = DEFAULT_IO_BUFFER_SIZE;
+ buf = pg_malloc(buflen);
while (CFH->read_func(buf, buflen, &cnt, CFH) && cnt > 0)
{
--
2.34.1
From a174cdff4ec8aad59f5bcc7e8d52218a14fe56fc Mon Sep 17 00:00:00 2001
From: Georgios Kokolatos <gkokola...@pm.me>
Date: Fri, 17 Mar 2023 14:45:58 +0000
Subject: [PATCH v3 1/3] Improve type handling in pg_dump's compress file API
The function LZ4File_gets() was storing the return value of
LZ4File_read_internal in a variable of the wrong type, disregarding sign-es.
As a consequence, LZ4File_gets() would not take the error path when it should.
In an attempt to improve readability and code uniformity, change the return type
of the API's read and write functions to bool from size_t. Along with it,
homogenize the return values of the relevant functions of this API.
This change, helps the specific compression implementations handle the return
types of their corresponding libraries internally and not expose them to the
API caller.
In passing save the appropriate errno in LZ4File_open_write in case that the
caller is not using the API's get_error_func.
With the help and guidance of Tomas Vondra.
Reported-by: Alexander Lakhin <exclus...@gmail.com>
---
src/bin/pg_dump/compress_gzip.c | 37 ++++++------
src/bin/pg_dump/compress_io.c | 8 +--
src/bin/pg_dump/compress_io.h | 38 +++++++++---
src/bin/pg_dump/compress_lz4.c | 85 +++++++++++++++------------
src/bin/pg_dump/compress_none.c | 41 ++++++++-----
src/bin/pg_dump/pg_backup_archiver.c | 18 ++----
src/bin/pg_dump/pg_backup_directory.c | 36 ++++++------
7 files changed, 148 insertions(+), 115 deletions(-)
diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c
index 0af65afeb4..79ed2cf371 100644
--- a/src/bin/pg_dump/compress_gzip.c
+++ b/src/bin/pg_dump/compress_gzip.c
@@ -233,14 +233,14 @@ InitCompressorGzip(CompressorState *cs,
*----------------------
*/
-static size_t
-Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH)
+static bool
+Gzip_read(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH)
{
gzFile gzfp = (gzFile) CFH->private_data;
- size_t ret;
+ int gzret;
- ret = gzread(gzfp, ptr, size);
- if (ret != size && !gzeof(gzfp))
+ gzret = gzread(gzfp, ptr, size);
+ if (gzret <= 0 && !gzeof(gzfp))
{
int errnum;
const char *errmsg = gzerror(gzfp, &errnum);
@@ -249,15 +249,18 @@ Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH)
errnum == Z_ERRNO ? strerror(errno) : errmsg);
}
- return ret;
+ if (rsize)
+ *rsize = (size_t) gzret;
+
+ return true;
}
-static size_t
+static bool
Gzip_write(const void *ptr, size_t size, CompressFileHandle *CFH)
{
gzFile gzfp = (gzFile) CFH->private_data;
- return gzwrite(gzfp, ptr, size);
+ return gzwrite(gzfp, ptr, size) > 0;
}
static int
@@ -287,22 +290,22 @@ Gzip_gets(char *ptr, int size, CompressFileHandle *CFH)
return gzgets(gzfp, ptr, size);
}
-static int
+static bool
Gzip_close(CompressFileHandle *CFH)
{
gzFile gzfp = (gzFile) CFH->private_data;
CFH->private_data = NULL;
- return gzclose(gzfp);
+ return gzclose(gzfp) == Z_OK;
}
-static int
+static bool
Gzip_eof(CompressFileHandle *CFH)
{
gzFile gzfp = (gzFile) CFH->private_data;
- return gzeof(gzfp);
+ return gzeof(gzfp) == 1;
}
static const char *
@@ -319,7 +322,7 @@ Gzip_get_error(CompressFileHandle *CFH)
return errmsg;
}
-static int
+static bool
Gzip_open(const char *path, int fd, const char *mode, CompressFileHandle *CFH)
{
gzFile gzfp;
@@ -342,19 +345,19 @@ Gzip_open(const char *path, int fd, const char *mode, CompressFileHandle *CFH)
gzfp = gzopen(path, mode_compression);
if (gzfp == NULL)
- return 1;
+ return false;
CFH->private_data = gzfp;
- return 0;
+ return true;
}
-static int
+static bool
Gzip_open_write(const char *path, const char *mode, CompressFileHandle *CFH)
{
char *fname;
- int ret;
int save_errno;
+ bool ret;
fname = psprintf("%s.gz", path);
ret = CFH->open_func(fname, -1, mode, CFH);
diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c
index ce06f1eac9..8f32cb4385 100644
--- a/src/bin/pg_dump/compress_io.c
+++ b/src/bin/pg_dump/compress_io.c
@@ -262,7 +262,7 @@ InitDiscoverCompressFileHandle(const char *path, const char *mode)
}
CFH = InitCompressFileHandle(compression_spec);
- if (CFH->open_func(fname, -1, mode, CFH))
+ if (!CFH->open_func(fname, -1, mode, CFH))
{
free_keep_errno(CFH);
CFH = NULL;
@@ -275,12 +275,12 @@ InitDiscoverCompressFileHandle(const char *path, const char *mode)
/*
* Close an open file handle and release its memory.
*
- * On failure, returns an error value and sets errno appropriately.
+ * On failure, returns false and sets errno appropriately.
*/
-int
+bool
EndCompressFileHandle(CompressFileHandle *CFH)
{
- int ret = 0;
+ bool ret = 0;
if (CFH->private_data)
ret = CFH->close_func(CFH);
diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index cdb15951ea..7c2f9b5668 100644
--- a/src/bin/pg_dump/compress_io.h
+++ b/src/bin/pg_dump/compress_io.h
@@ -100,8 +100,10 @@ struct CompressFileHandle
* Pass either 'path' or 'fd' depending on whether a file path or a file
* descriptor is available. 'mode' can be one of 'r', 'rb', 'w', 'wb',
* 'a', and 'ab'. Requires an already initialized CompressFileHandle.
+ *
+ * Returns true on success and false on error.
*/
- int (*open_func) (const char *path, int fd, const char *mode,
+ bool (*open_func) (const char *path, int fd, const char *mode,
CompressFileHandle *CFH);
/*
@@ -109,19 +111,27 @@ struct CompressFileHandle
*
* 'mode' can be one of 'w', 'wb', 'a', and 'ab'. Requires an already
* initialized CompressFileHandle.
+ *
+ * Returns true on success and false on error.
*/
- int (*open_write_func) (const char *path, const char *mode,
+ bool (*open_write_func) (const char *path, const char *mode,
CompressFileHandle *CFH);
/*
* Read 'size' bytes of data from the file and store them into 'ptr'.
+ * Optionally it will store the number of bytes read in 'rsize'.
+ *
+ * Returns true on success and throws an internal error otherwise.
*/
- size_t (*read_func) (void *ptr, size_t size, CompressFileHandle *CFH);
+ bool (*read_func) (void *ptr, size_t size, size_t *rsize,
+ CompressFileHandle *CFH);
/*
* Write 'size' bytes of data into the file from 'ptr'.
+ *
+ * Returns true on success and false on error.
*/
- size_t (*write_func) (const void *ptr, size_t size,
+ bool (*write_func) (const void *ptr, size_t size,
struct CompressFileHandle *CFH);
/*
@@ -130,28 +140,38 @@ struct CompressFileHandle
*
* Stop if an EOF or a newline is found first. 's' is always null
* terminated and contains the newline if it was found.
+ *
+ * Returns 's' on success, and NULL on error or when end of file occurs
+ * while no characters have been read.
*/
char *(*gets_func) (char *s, int size, CompressFileHandle *CFH);
/*
* Read the next character from the compress file handle as 'unsigned
* char' cast into 'int'.
+ *
+ * Returns the character read on success and throws an internal error
+ * otherwise. It treats EOF as error.
*/
int (*getc_func) (CompressFileHandle *CFH);
/*
* Test if EOF is reached in the compress file handle.
+ *
+ * Returns true if it is reached.
*/
- int (*eof_func) (CompressFileHandle *CFH);
+ bool (*eof_func) (CompressFileHandle *CFH);
/*
* Close an open file handle.
+ *
+ * Returns true on success and false on error.
*/
- int (*close_func) (CompressFileHandle *CFH);
+ bool (*close_func) (CompressFileHandle *CFH);
/*
- * Get a pointer to a string that describes an error that occurred during a
- * compress file handle operation.
+ * Get a pointer to a string that describes an error that occurred during
+ * a compress file handle operation.
*/
const char *(*get_error_func) (CompressFileHandle *CFH);
@@ -178,5 +198,5 @@ extern CompressFileHandle *InitCompressFileHandle(const pg_compress_specificatio
*/
extern CompressFileHandle *InitDiscoverCompressFileHandle(const char *path,
const char *mode);
-extern int EndCompressFileHandle(CompressFileHandle *CFH);
+extern bool EndCompressFileHandle(CompressFileHandle *CFH);
#endif
diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c
index 63e794cdc6..278f262162 100644
--- a/src/bin/pg_dump/compress_lz4.c
+++ b/src/bin/pg_dump/compress_lz4.c
@@ -165,7 +165,7 @@ typedef struct LZ4File
* decompressed output in the overflow buffer and the end of the backing file
* is reached.
*/
-static int
+static bool
LZ4File_eof(CompressFileHandle *CFH)
{
LZ4File *fs = (LZ4File *) CFH->private_data;
@@ -192,14 +192,16 @@ LZ4File_get_error(CompressFileHandle *CFH)
*
* It creates the necessary contexts for the operations. When compressing,
* it additionally writes the LZ4 header in the output stream.
+ *
+ * Returns true on success and false on error.
*/
-static int
+static bool
LZ4File_init(LZ4File *fs, int size, bool compressing)
{
size_t status;
if (fs->inited)
- return 0;
+ return true;
fs->compressing = compressing;
fs->inited = true;
@@ -214,7 +216,7 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
if (LZ4F_isError(status))
{
fs->errcode = status;
- return 1;
+ return false;
}
fs->buffer = pg_malloc(fs->buflen);
@@ -224,13 +226,13 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
if (LZ4F_isError(status))
{
fs->errcode = status;
- return 1;
+ return false;
}
if (fwrite(fs->buffer, 1, status, fs->fp) != status)
{
errno = (errno) ? errno : ENOSPC;
- return 1;
+ return false;
}
}
else
@@ -239,7 +241,7 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
if (LZ4F_isError(status))
{
fs->errcode = status;
- return 1;
+ return false;
}
fs->buflen = size > LZ4_OUT_SIZE ? size : LZ4_OUT_SIZE;
@@ -250,7 +252,7 @@ LZ4File_init(LZ4File *fs, int size, bool compressing)
fs->overflowlen = 0;
}
- return 0;
+ return true;
}
/*
@@ -302,15 +304,15 @@ LZ4File_read_overflow(LZ4File *fs, void *ptr, int size, bool eol_flag)
static int
LZ4File_read_internal(LZ4File *fs, void *ptr, int ptrsize, bool eol_flag)
{
- size_t dsize = 0;
- size_t rsize;
- size_t size = ptrsize;
+ int dsize = 0;
+ int rsize;
+ int size = ptrsize;
bool eol_found = false;
void *readbuf;
/* Lazy init */
- if (LZ4File_init(fs, size, false /* decompressing */ ))
+ if (!LZ4File_init(fs, size, false /* decompressing */ ))
return -1;
/* Verify that there is enough space in the outbuf */
@@ -398,17 +400,17 @@ LZ4File_read_internal(LZ4File *fs, void *ptr, int ptrsize, bool eol_flag)
fs->overflowlen += outlen;
}
}
- } while (rsize == size && dsize < size && eol_found == 0);
+ } while (rsize == size && dsize < size && eol_found == false);
pg_free(readbuf);
- return (int) dsize;
+ return dsize;
}
/*
* Compress size bytes from ptr and write them to the stream.
*/
-static size_t
+static bool
LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH)
{
LZ4File *fs = (LZ4File *) CFH->private_data;
@@ -416,8 +418,8 @@ LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH)
int remaining = size;
/* Lazy init */
- if (LZ4File_init(fs, size, true))
- return -1;
+ if (!LZ4File_init(fs, size, true))
+ return false;
while (remaining > 0)
{
@@ -430,33 +432,35 @@ LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH)
if (LZ4F_isError(status))
{
fs->errcode = status;
- return -1;
+ return false;
}
if (fwrite(fs->buffer, 1, status, fs->fp) != status)
{
errno = (errno) ? errno : ENOSPC;
- return 1;
+ return false;
}
}
- return size;
+ return true;
}
/*
* fread() equivalent implementation for LZ4 compressed files.
*/
-static size_t
-LZ4File_read(void *ptr, size_t size, CompressFileHandle *CFH)
+static bool
+LZ4File_read(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH)
{
LZ4File *fs = (LZ4File *) CFH->private_data;
int ret;
- ret = LZ4File_read_internal(fs, ptr, size, false);
- if (ret != size && !LZ4File_eof(CFH))
+ if ((ret = LZ4File_read_internal(fs, ptr, size, false)) < 0)
pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH));
- return ret;
+ if (rsize)
+ *rsize = (size_t) ret;
+
+ return true;
}
/*
@@ -468,7 +472,7 @@ LZ4File_getc(CompressFileHandle *CFH)
LZ4File *fs = (LZ4File *) CFH->private_data;
unsigned char c;
- if (LZ4File_read_internal(fs, &c, 1, false) != 1)
+ if (LZ4File_read_internal(fs, &c, 1, false) <= 0)
{
if (!LZ4File_eof(CFH))
pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH));
@@ -486,14 +490,14 @@ static char *
LZ4File_gets(char *ptr, int size, CompressFileHandle *CFH)
{
LZ4File *fs = (LZ4File *) CFH->private_data;
- size_t dsize;
+ int ret;
- dsize = LZ4File_read_internal(fs, ptr, size, true);
- if (dsize < 0)
+ ret = LZ4File_read_internal(fs, ptr, size, true);
+ if (ret < 0 || (ret == 0 && !LZ4File_eof(CFH)))
pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH));
/* Done reading */
- if (dsize == 0)
+ if (ret == 0)
return NULL;
return ptr;
@@ -503,13 +507,12 @@ LZ4File_gets(char *ptr, int size, CompressFileHandle *CFH)
* Finalize (de)compression of a stream. When compressing it will write any
* remaining content and/or generated footer from the LZ4 API.
*/
-static int
+static bool
LZ4File_close(CompressFileHandle *CFH)
{
FILE *fp;
LZ4File *fs = (LZ4File *) CFH->private_data;
size_t status;
- int ret;
fp = fs->fp;
if (fs->inited)
@@ -520,7 +523,7 @@ LZ4File_close(CompressFileHandle *CFH)
if (LZ4F_isError(status))
pg_fatal("failed to end compression: %s",
LZ4F_getErrorName(status));
- else if ((ret = fwrite(fs->buffer, 1, status, fs->fp)) != status)
+ else if (fwrite(fs->buffer, 1, status, fs->fp) != status)
{
errno = (errno) ? errno : ENOSPC;
WRITE_ERROR_EXIT;
@@ -545,10 +548,10 @@ LZ4File_close(CompressFileHandle *CFH)
pg_free(fs);
- return fclose(fp);
+ return fclose(fp) == 0;
}
-static int
+static bool
LZ4File_open(const char *path, int fd, const char *mode,
CompressFileHandle *CFH)
{
@@ -562,23 +565,27 @@ LZ4File_open(const char *path, int fd, const char *mode,
if (fp == NULL)
{
lz4fp->errcode = errno;
- return 1;
+ return false;
}
lz4fp->fp = fp;
- return 0;
+ return true;
}
-static int
+static bool
LZ4File_open_write(const char *path, const char *mode, CompressFileHandle *CFH)
{
char *fname;
- int ret;
+ int save_errno;
+ bool ret;
fname = psprintf("%s.lz4", path);
ret = CFH->open_func(fname, -1, mode, CFH);
+
+ save_errno = errno;
pg_free(fname);
+ errno = save_errno;
return ret;
}
diff --git a/src/bin/pg_dump/compress_none.c b/src/bin/pg_dump/compress_none.c
index ecbcf4b04a..18f3514d11 100644
--- a/src/bin/pg_dump/compress_none.c
+++ b/src/bin/pg_dump/compress_none.c
@@ -83,27 +83,36 @@ InitCompressorNone(CompressorState *cs,
* Private routines
*/
-static size_t
-read_none(void *ptr, size_t size, CompressFileHandle *CFH)
+static bool
+read_none(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH)
{
FILE *fp = (FILE *) CFH->private_data;
size_t ret;
if (size == 0)
- return 0;
+ return true;
ret = fread(ptr, 1, size, fp);
if (ret != size && !feof(fp))
pg_fatal("could not read from input file: %s",
strerror(errno));
- return ret;
+ if (rsize)
+ *rsize = ret;
+
+ return true;
}
-static size_t
+static bool
write_none(const void *ptr, size_t size, CompressFileHandle *CFH)
{
- return fwrite(ptr, 1, size, (FILE *) CFH->private_data);
+ size_t ret;
+
+ ret = fwrite(ptr, 1, size, (FILE *) CFH->private_data);
+ if (ret != size)
+ return false;
+
+ return true;
}
static const char *
@@ -136,7 +145,7 @@ getc_none(CompressFileHandle *CFH)
return ret;
}
-static int
+static bool
close_none(CompressFileHandle *CFH)
{
FILE *fp = (FILE *) CFH->private_data;
@@ -147,16 +156,16 @@ close_none(CompressFileHandle *CFH)
if (fp)
ret = fclose(fp);
- return ret;
+ return ret == 0;
}
-static int
+static bool
eof_none(CompressFileHandle *CFH)
{
- return feof((FILE *) CFH->private_data);
+ return feof((FILE *) CFH->private_data) != 0;
}
-static int
+static bool
open_none(const char *path, int fd, const char *mode, CompressFileHandle *CFH)
{
Assert(CFH->private_data == NULL);
@@ -167,21 +176,21 @@ open_none(const char *path, int fd, const char *mode, CompressFileHandle *CFH)
CFH->private_data = fopen(path, mode);
if (CFH->private_data == NULL)
- return 1;
+ return false;
- return 0;
+ return true;
}
-static int
+static bool
open_write_none(const char *path, const char *mode, CompressFileHandle *CFH)
{
Assert(CFH->private_data == NULL);
CFH->private_data = fopen(path, mode);
if (CFH->private_data == NULL)
- return 1;
+ return false;
- return 0;
+ return true;
}
/*
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 61ebb8fe85..6f3a85fe20 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -265,16 +265,13 @@ OpenArchive(const char *FileSpec, const ArchiveFormat fmt)
void
CloseArchive(Archive *AHX)
{
- int res = 0;
ArchiveHandle *AH = (ArchiveHandle *) AHX;
AH->ClosePtr(AH);
/* Close the output */
errno = 0;
- res = EndCompressFileHandle(AH->OF);
-
- if (res != 0)
+ if (!EndCompressFileHandle(AH->OF))
pg_fatal("could not close output file: %m");
}
@@ -1529,7 +1526,7 @@ SetOutput(ArchiveHandle *AH, const char *filename,
CFH = InitCompressFileHandle(compression_spec);
- if (CFH->open_func(filename, fn, mode, CFH))
+ if (!CFH->open_func(filename, fn, mode, CFH))
{
if (filename)
pg_fatal("could not open output file \"%s\": %m", filename);
@@ -1549,12 +1546,8 @@ SaveOutput(ArchiveHandle *AH)
static void
RestoreOutput(ArchiveHandle *AH, CompressFileHandle *savedOutput)
{
- int res;
-
errno = 0;
- res = EndCompressFileHandle(AH->OF);
-
- if (res != 0)
+ if (!EndCompressFileHandle(AH->OF))
pg_fatal("could not close output file: %m");
AH->OF = savedOutput;
@@ -1694,7 +1687,8 @@ ahwrite(const void *ptr, size_t size, size_t nmemb, ArchiveHandle *AH)
{
CompressFileHandle *CFH = (CompressFileHandle *) AH->OF;
- bytes_written = CFH->write_func(ptr, size * nmemb, CFH);
+ if (CFH->write_func(ptr, size * nmemb, CFH))
+ bytes_written = size * nmemb;
}
if (bytes_written != size * nmemb)
@@ -2243,7 +2237,7 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
/* Open stdout with no compression for AH output handle */
out_compress_spec.algorithm = PG_COMPRESSION_NONE;
CFH = InitCompressFileHandle(out_compress_spec);
- if (CFH->open_func(NULL, fileno(stdout), PG_BINARY_A, CFH))
+ if (!CFH->open_func(NULL, fileno(stdout), PG_BINARY_A, CFH))
pg_fatal("could not open stdout for appending: %m");
AH->OF = CFH;
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index 41c2b733e3..525dbf9bf0 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -217,7 +217,7 @@ InitArchiveFmt_Directory(ArchiveHandle *AH)
ReadToc(AH);
/* Nothing else in the file, so close it again... */
- if (EndCompressFileHandle(tocFH) != 0)
+ if (!EndCompressFileHandle(tocFH))
pg_fatal("could not close TOC file: %m");
ctx->dataFH = NULL;
}
@@ -328,7 +328,7 @@ _StartData(ArchiveHandle *AH, TocEntry *te)
ctx->dataFH = InitCompressFileHandle(AH->compression_spec);
- if (ctx->dataFH->open_write_func(fname, PG_BINARY_W, ctx->dataFH))
+ if (!ctx->dataFH->open_write_func(fname, PG_BINARY_W, ctx->dataFH))
pg_fatal("could not open output file \"%s\": %m", fname);
}
@@ -348,7 +348,7 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen)
CompressFileHandle *CFH = ctx->dataFH;
errno = 0;
- if (dLen > 0 && CFH->write_func(data, dLen, CFH) != dLen)
+ if (dLen > 0 && !CFH->write_func(data, dLen, CFH))
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
@@ -370,7 +370,7 @@ _EndData(ArchiveHandle *AH, TocEntry *te)
lclContext *ctx = (lclContext *) AH->formatData;
/* Close the file */
- if (EndCompressFileHandle(ctx->dataFH) != 0)
+ if (!EndCompressFileHandle(ctx->dataFH))
pg_fatal("could not close data file: %m");
ctx->dataFH = NULL;
@@ -382,7 +382,7 @@ _EndData(ArchiveHandle *AH, TocEntry *te)
static void
_PrintFileData(ArchiveHandle *AH, char *filename)
{
- size_t cnt;
+ size_t cnt = 0;
char *buf;
size_t buflen;
CompressFileHandle *CFH;
@@ -397,13 +397,13 @@ _PrintFileData(ArchiveHandle *AH, char *filename)
buf = pg_malloc(ZLIB_OUT_SIZE);
buflen = ZLIB_OUT_SIZE;
- while ((cnt = CFH->read_func(buf, buflen, CFH)))
+ while (CFH->read_func(buf, buflen, &cnt, CFH) && cnt > 0)
{
ahwrite(buf, 1, cnt, AH);
}
free(buf);
- if (EndCompressFileHandle(CFH) != 0)
+ if (!EndCompressFileHandle(CFH))
pg_fatal("could not close data file \"%s\": %m", filename);
}
@@ -468,7 +468,7 @@ _LoadLOs(ArchiveHandle *AH)
pg_fatal("error reading large object TOC file \"%s\"",
tocfname);
- if (EndCompressFileHandle(ctx->LOsTocFH) != 0)
+ if (!EndCompressFileHandle(ctx->LOsTocFH))
pg_fatal("could not close large object TOC file \"%s\": %m",
tocfname);
@@ -491,7 +491,7 @@ _WriteByte(ArchiveHandle *AH, const int i)
CompressFileHandle *CFH = ctx->dataFH;
errno = 0;
- if (CFH->write_func(&c, 1, CFH) != 1)
+ if (!CFH->write_func(&c, 1, CFH))
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
@@ -529,7 +529,7 @@ _WriteBuf(ArchiveHandle *AH, const void *buf, size_t len)
CompressFileHandle *CFH = ctx->dataFH;
errno = 0;
- if (CFH->write_func(buf, len, CFH) != len)
+ if (!CFH->write_func(buf, len, CFH))
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
@@ -554,7 +554,7 @@ _ReadBuf(ArchiveHandle *AH, void *buf, size_t len)
* If there was an I/O error, we already exited in readF(), so here we
* exit on short reads.
*/
- if (CFH->read_func(buf, len, CFH) != len)
+ if (!CFH->read_func(buf, len, NULL, CFH))
pg_fatal("could not read from input file: end of file");
}
@@ -589,7 +589,7 @@ _CloseArchive(ArchiveHandle *AH)
/* The TOC is always created uncompressed */
compression_spec.algorithm = PG_COMPRESSION_NONE;
tocFH = InitCompressFileHandle(compression_spec);
- if (tocFH->open_write_func(fname, PG_BINARY_W, tocFH))
+ if (!tocFH->open_write_func(fname, PG_BINARY_W, tocFH))
pg_fatal("could not open output file \"%s\": %m", fname);
ctx->dataFH = tocFH;
@@ -602,7 +602,7 @@ _CloseArchive(ArchiveHandle *AH)
WriteHead(AH);
AH->format = archDirectory;
WriteToc(AH);
- if (EndCompressFileHandle(tocFH) != 0)
+ if (!EndCompressFileHandle(tocFH))
pg_fatal("could not close TOC file: %m");
WriteDataChunks(AH, ctx->pstate);
@@ -654,7 +654,7 @@ _StartLOs(ArchiveHandle *AH, TocEntry *te)
/* The LO TOC file is never compressed */
compression_spec.algorithm = PG_COMPRESSION_NONE;
ctx->LOsTocFH = InitCompressFileHandle(compression_spec);
- if (ctx->LOsTocFH->open_write_func(fname, "ab", ctx->LOsTocFH))
+ if (!ctx->LOsTocFH->open_write_func(fname, "ab", ctx->LOsTocFH))
pg_fatal("could not open output file \"%s\": %m", fname);
}
@@ -672,7 +672,7 @@ _StartLO(ArchiveHandle *AH, TocEntry *te, Oid oid)
snprintf(fname, MAXPGPATH, "%s/blob_%u.dat", ctx->directory, oid);
ctx->dataFH = InitCompressFileHandle(AH->compression_spec);
- if (ctx->dataFH->open_write_func(fname, PG_BINARY_W, ctx->dataFH))
+ if (!ctx->dataFH->open_write_func(fname, PG_BINARY_W, ctx->dataFH))
pg_fatal("could not open output file \"%s\": %m", fname);
}
@@ -690,13 +690,13 @@ _EndLO(ArchiveHandle *AH, TocEntry *te, Oid oid)
int len;
/* Close the BLOB data file itself */
- if (EndCompressFileHandle(ctx->dataFH) != 0)
+ if (!EndCompressFileHandle(ctx->dataFH))
pg_fatal("could not close LO data file: %m");
ctx->dataFH = NULL;
/* register the LO in blobs.toc */
len = snprintf(buf, sizeof(buf), "%u blob_%u.dat\n", oid, oid);
- if (CFH->write_func(buf, len, CFH) != len)
+ if (!CFH->write_func(buf, len, CFH))
pg_fatal("could not write to LOs TOC file");
}
@@ -710,7 +710,7 @@ _EndLOs(ArchiveHandle *AH, TocEntry *te)
{
lclContext *ctx = (lclContext *) AH->formatData;
- if (EndCompressFileHandle(ctx->LOsTocFH) != 0)
+ if (!EndCompressFileHandle(ctx->LOsTocFH))
pg_fatal("could not close LOs TOC file: %m");
ctx->LOsTocFH = NULL;
}
--
2.34.1
