pg_dump, gzwrite, and errno

On 2020-Jun-11, Justin Pryzby wrote:

--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -347,8 +347,12 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen)
lclContext *ctx = (lclContext *) AH->formatData;

if (dLen > 0 && cfwrite(data, dLen, ctx->dataFH) != dLen)
+       {
+               if (errno == 0)
+                       errno = ENOSPC;
fatal("could not write to output file: %s",
get_cfp_error(ctx->dataFH));
+       }
}

This seems correct to me. (I spent a long time looking at zlib sources
to convince myself that it does work with compressed files too). There
are more calls to cfwrite in pg_backup_directory.c though -- we should
patch them all.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2020-Jun-11, Justin Pryzby wrote:

--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -347,8 +347,12 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen)
lclContext *ctx = (lclContext *) AH->formatData;

if (dLen > 0 && cfwrite(data, dLen, ctx->dataFH) != dLen)
+       {
+               if (errno == 0)
+                       errno = ENOSPC;
fatal("could not write to output file: %s",
get_cfp_error(ctx->dataFH));
+       }
}

This seems correct to me.

Surely it's insufficient as-is, because there is no reason to suppose
that errno is zero at entry. You'd need to set errno = 0 first.

Also it's fairly customary in our sources to include a comment about
this machination; so the full ritual is usually more like

errno = 0;
if (pg_pwrite(fd, data, len, xlrec->offset) != len)
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
errno = ENOSPC;
ereport ...

(I spent a long time looking at zlib sources
to convince myself that it does work with compressed files too)

Yeah, it's not obvious that gzwrite has the same behavior w.r.t. errno
as a plain write. But there's not much we can do to improve matters
if it does not, so we might as well assume it does.

regards, tom lane

On 2020-Jun-18, Tom Lane wrote:

Surely it's insufficient as-is, because there is no reason to suppose
that errno is zero at entry. You'd need to set errno = 0 first.

Oh, right.

Also it's fairly customary in our sources to include a comment about
this machination; so the full ritual is usually more like

Yeah, I had that in my local copy. Done like that in all the most
obvious places. But there are more places that are still wrong: I
believe every single place that calls WRITE_ERROR_EXIT is doing the
wrong thing.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services