Horton

Thread

Commits

  1. Fix pg_dump's errno checking for zlib I/O

  2. Add pg_basebackup tool for streaming base backups

  1. [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Kunshchikov Vladimir <vladimir.kunshchikov@infotecs.ru> — 2017-06-22T08:46:21Z

    Hello,

    our testing team has noticed apparently wrong backup/restore error messages like this:


pg_restore: [compress_io] could not read from input file: success
pg_dump: [directory archiver] could not write to output file: success



Such "success" messages are caused by calling strerror() after gzread()/gzwrite() failures.

In order to properly decode errors, there should be used  gzerror() instead of strerror():
 http://refspecs.linuxbase.org/LSB_2.1.0/LSB-generic/LSB-generic/zlib-gzerror-1.html

Errors should be like this:
pg_restore: [compress_io] could not read from input file: d3/2811.dat.gz: invalid distance too far back

Attached small fix for this issue.

You can view that patch  online on our github:
https://github.com/Infotecs/postgres/commit/1578f5011ad22d78ae059a4ef0924426fd6db762


--
Best regards,
Vladimir Kunschikov
Lead software developer
IDS project
InfoTeCS JSC

<http://portal.infotecs.int/company/structure/Pages/detailsDepartment.aspx?idDepartment=235&idCompany=17>

  2. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-06-22T15:40:04Z

    Kunshchikov Vladimir wrote:

Hi,

>     our testing team has noticed apparently wrong backup/restore error messages like this:
> 
> 
> pg_restore: [compress_io] could not read from input file: success
> pg_dump: [directory archiver] could not write to output file: success
> 
> 
> 
> Such "success" messages are caused by calling strerror() after gzread()/gzwrite() failures.

Yeah, I've complained about this before but never got around to actually
fixing it.  Thanks for the patch, I'll see about putting it on all
branches soon.

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

  3. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-07-25T22:02:34Z

    Kunshchikov Vladimir wrote:

> Errors should be like this:
> pg_restore: [compress_io] could not read from input file: d3/2811.dat.gz: invalid distance too far back
> 
> Attached small fix for this issue.

After looking at this patch, I don't like it very much.  In particular,
I don't like the way you've handled the WRITE_ERROR_EXIT macro in
pg_backup_directory.c by undef'ing the existing one and creating it
anew.  The complete and correct definition should reside in one place
(pg_backup_archiver.h), instead of being split in two and being defined
differently.

Another point is that you broke the comment on the definition of struct
cfp "this is opaque to callers" by moving it to the header file
precisely with the point of making it transparent to callers.  We need
some better idea there.  I think it can be done by making compress_io.c
responsible of handing over the error message through some new function
(something very simple like "if compressedfp then return get_gz_error
else strerror" should suffice).

Also, I needed to rebase your patch over a recent pgindent run, though
that's a pretty small change.

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

  4. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Kunshchikov Vladimir <vladimir.kunshchikov@infotecs.ru> — 2017-07-26T15:32:33Z

     Hello Alvaro, thanks for the feedback, fixed all of your points.
Attached new version of patch.

--
Best regards,
Vladimir Kunschikov
Lead software developer
IDS project
InfoTeCS JSC
________________________________________
From: Alvaro Herrera <alvherre@2ndquadrant.com>
Sent: Wednesday, July 26, 2017 1:02 AM
To: Kunshchikov Vladimir
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap

Kunshchikov Vladimir wrote:

> Errors should be like this:
> pg_restore: [compress_io] could not read from input file: d3/2811.dat.gz: invalid distance too far back
>
> Attached small fix for this issue.

After looking at this patch, I don't like it very much.  In particular,
I don't like the way you've handled the WRITE_ERROR_EXIT macro in
pg_backup_directory.c by undef'ing the existing one and creating it
anew.  The complete and correct definition should reside in one place
(pg_backup_archiver.h), instead of being split in two and being defined
differently.

Another point is that you broke the comment on the definition of struct
cfp "this is opaque to callers" by moving it to the header file
precisely with the point of making it transparent to callers.  We need
some better idea there.  I think it can be done by making compress_io.c
responsible of handing over the error message through some new function
(something very simple like "if compressedfp then return get_gz_error
else strerror" should suffice).

Also, I needed to rebase your patch over a recent pgindent run, though
that's a pretty small change.

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

  5. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-07-26T16:40:20Z

    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

  6. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Kunshchikov Vladimir <vladimir.kunshchikov@infotecs.ru> — 2017-07-27T07:34:22Z

    Hello, Alvaro,

  thanks for the suggestions,  attached version #3 with all of your requirements met.



--
С уважением,
Владимир Кунщиков
Ведущий программист

Отдел разработки систем обнаружения и предотвращения компьютерных атак

Компания "ИнфоТеКС"
<http://portal.infotecs.int/company/structure/Pages/detailsDepartment.aspx?idDepartment=235&idCompany=17>
________________________________
From: Alvaro Herrera <alvherre@2ndquadrant.com>
Sent: Wednesday, July 26, 2017 7:40:20 PM
To: Kunshchikov Vladimir
Cc: pgsql-hackers@postgresql.org
Subject: 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

  7. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Kunshchikov Vladimir <vladimir.kunshchikov@infotecs.ru> — 2017-07-28T08:29:01Z

    Hello Alvaro,

here goes v4 version: removed unused header.

Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce any warnings.

--
Best regards,
Vladimir Kunschikov
Lead software developer
IDS project
InfoTeCS JSC

________________________________
From: Kunshchikov Vladimir
Sent: Thursday, July 27, 2017 10:34:22 AM
To: Alvaro Herrera
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap



Hello, Alvaro,

  thanks for the suggestions,  attached version #3 with all of your requirements met.



--
С уважением,
Владимир Кунщиков
Ведущий программист

Отдел разработки систем обнаружения и предотвращения компьютерных атак

Компания "ИнфоТеКС"
<http://portal.infotecs.int/company/structure/Pages/detailsDepartment.aspx?idDepartment=235&idCompany=17>
________________________________
From: Alvaro Herrera <alvherre@2ndquadrant.com>
Sent: Wednesday, July 26, 2017 7:40:20 PM
To: Kunshchikov Vladimir
Cc: pgsql-hackers@postgresql.org
Subject: 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

  8. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-07-28T16:25:52Z

    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

  9. Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-08-02T22:36:00Z

    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