Обсуждение: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror()mishap

Kunshchikov Vladimir wrote:
>  Hello Alvaro, thanks for the feedback, fixed all of your points.
> Attached new version of patch.

Looks great -- it's a lot smaller than the original even.  One final
touch -- see cfread(), where we have an #ifdef where we test for
fp->compressedfp; the "#else" branch uses the same code as the
!fp->compressedfp.  I think your get_cfp_error can be written more
simply using that form.  (Also, your version probably errors or warns
about "code before declaration" in that routine, which is not allowed in
C89.)

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

Kunshchikov Vladimir wrote:
> Hello Alvaro,
> 
> here goes v4 version: removed unused header.
> 
> Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce any warnings.

Great, thanks.

+const char *
+get_cfp_error(cfp* fp)
+{
+#ifdef HAVE_LIBZ
+    if(fp->compressedfp){
+        int errnum;
+        static const char fallback[] = "Zlib error";
+        const int maxlen = 255;
+        const char *errmsg = gzerror(fp->compressedfp, &errnum);
+        if(!errmsg || !memchr(errmsg, 0, maxlen))
+            errmsg = fallback;
+
+        return errnum == Z_ERRNO ? strerror(errno) : errmsg;
+    }#endif
+    return strerror(errno);
+}

This "maxlen" business and the fallback error message are strange.  We
have roughly equivalent code in pg_basebackup.c, which has been working
since 2011 (commit 048d148fe631), and it looks like this:

#ifdef HAVE_LIBZ
static const char *
get_gz_error(gzFile gzf)
{int            errnum;const char *errmsg;
errmsg = gzerror(gzf, &errnum);if (errnum == Z_ERRNO)    return strerror(errno);else    return errmsg;
}
#endif

Perhaps you can drop the memchr/fallback tricks and adopt the
pg_basebackup coding?  Or is there a specific reason to have the memchr
check?

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

Kunshchikov Vladimir wrote:
> Hello Alvaro,
> 
> here goes v4 version: removed unused header.
> 
> Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce any warnings.

Great, thanks!  I have pushed this to all branches since 9.4.  Would you
please give it a look?  Please let me know if you find any problems
(particularly since you seem to have a test rig to verify it on
corrupted files).

I noticed that with this patch we no longer use WRITE_ERROR_EXIT in
certain cases but instead do the printing/exiting directly.  I think
that's fine, but it would be neater to improve the WRITE_ERROR_EXIT
macro so that it takes the cfp as an argument, and then the macro is in
charge of calling get_cfp_error.  But then I noticed that it wasn't very
easy to improve things that way.  I also noticed that the usage of mixed
compressed/uncompressed file pointers in pg_dump is not very consistent,
and it would take rather a lot of effort to clean up.  So I gave up for
now, particularly as a backpatchable bugfix.  If you're interested in
mop-up work, I think we can improve pg_dump some more there.

At least, I think most common cases should correctly report any zlib
problems now.  Thanks for the patch!

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