pgsql: Minor pg_dump improvements

Stephen Frost <sfrost@snowman.net> writes:

Minor pg_dump improvements

I'm pretty sure you broke _CloseArchive with this hunk:

@@ -708,6 +708,9 @@ _CloseArchive(ArchiveHandle *AH)
    {
        WriteHead(AH);
        tpos = ftello(AH->FH);
+       if (tpos < 0 || errno)
+           exit_horribly(modulename, "could not determine seek position in archive file: %s\n",
+                         strerror(errno));
        WriteToc(AH);
        ctx->dataStart = _getFilePos(AH, ctx);

There's no reason to assume errno is zero at entry, and in any case
it should not be necessary to test for anything except tpos < 0.

I'm not sure why _ReopenArchive is coded like it was; seems like
tpos < 0 should be a sufficient test there too, and we shouldn't have to
reset errno beforehand.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

Minor pg_dump improvements

I'm pretty sure you broke _CloseArchive with this hunk:

That'd be pretty frustrating as my testing didn't exhibit any issues.

@@ -708,6 +708,9 @@ _CloseArchive(ArchiveHandle *AH)
{
WriteHead(AH);
tpos = ftello(AH->FH);
+       if (tpos < 0 || errno)
+           exit_horribly(modulename, "could not determine seek position in archive file: %s\n",
+                         strerror(errno));
WriteToc(AH);
ctx->dataStart = _getFilePos(AH, ctx);

There's no reason to assume errno is zero at entry, and in any case
it should not be necessary to test for anything except tpos < 0.

Good point.

I'm not sure why _ReopenArchive is coded like it was; seems like
tpos < 0 should be a sufficient test there too, and we shouldn't have to
reset errno beforehand.

Agreed, I'll update that also.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

I'm pretty sure you broke _CloseArchive with this hunk:

That'd be pretty frustrating as my testing didn't exhibit any issues.

It's possible that errno is accidentally zero when we get there, but
the code as-committed surely can't be called bulletproof.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

I'm pretty sure you broke _CloseArchive with this hunk:

That'd be pretty frustrating as my testing didn't exhibit any issues.

It's possible that errno is accidentally zero when we get there, but
the code as-committed surely can't be called bulletproof.

Agreed, oversight on my part.

The only remaining place we still clear errno in pg_dump is in
pg_backup_archive.c:checkSeek() around a similar ftello call, perhaps
that should be changed to check the result instead also?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

The only remaining place we still clear errno in pg_dump is in
pg_backup_archive.c:checkSeek() around a similar ftello call, perhaps
that should be changed to check the result instead also?

Agreed; we should be using the same coding pattern wherever we call
ftello.

I suspect that this code may be left over from coping with some ancient
non-spec-compliant version of ftello? Probably not worth digging in
the archives to find out. The Single Unix Spec v2 says that the result
is (off_t) -1 on error, and we generally assume that platforms are at
least compliant with that.

grep shows me a couple of other places where the result of ftello doesn't
seem to be getting checked for error. Odd that Coverity didn't notice
those.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

The only remaining place we still clear errno in pg_dump is in
pg_backup_archive.c:checkSeek() around a similar ftello call, perhaps
that should be changed to check the result instead also?

Agreed; we should be using the same coding pattern wherever we call
ftello.

Ok, I'll take care of that in a couple of hours (have to step out for a
bit).

I suspect that this code may be left over from coping with some ancient
non-spec-compliant version of ftello? Probably not worth digging in
the archives to find out. The Single Unix Spec v2 says that the result
is (off_t) -1 on error, and we generally assume that platforms are at
least compliant with that.

I had been wondering the same, but agree w/ your assessment.

grep shows me a couple of other places where the result of ftello doesn't
seem to be getting checked for error. Odd that Coverity didn't notice
those.

I'll try to figure out why it didn't, I would have expected it to. It's
possible I just hadn't gotten to those yet or someone decided they were
non-issues.

Thanks,

Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

grep shows me a couple of other places where the result of ftello doesn't
seem to be getting checked for error. Odd that Coverity didn't notice
those.

At least two of those are in a #if 0 block... since 2001
(pg_backup_tar.c:_tarGetHeader). I'm thinking we may be better off
removing that code rather than continuing to pull it along (and update
it- there were at least three commits fixing things in that block
after it had been #if 0'd out). Another technically had a check, but
it was late, I've got a patch to improve that.

The last is inside a snprintf() which is just building a string to call
exit_horribly() with- seems like that's safe enough?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

grep shows me a couple of other places where the result of ftello doesn't
seem to be getting checked for error. Odd that Coverity didn't notice
those.

At least two of those are in a #if 0 block... since 2001
(pg_backup_tar.c:_tarGetHeader). I'm thinking we may be better off
removing that code rather than continuing to pull it along

Works for me.

The last is inside a snprintf() which is just building a string to call
exit_horribly() with- seems like that's safe enough?

Agreed. Printing -1 seems like appropriate behavior there.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 2014-02-09 02:29:25 +0000, Stephen Frost wrote:

Minor pg_dump improvements

Improve pg_dump by checking results on various fgetc() calls which
previously were unchecked, ditto for ftello. Also clean up a couple
of very minor memory leaks by waiting to allocate structures until
after the initial check(s).

This triggers clang to emit:

/home/andres/src/postgresql/src/bin/pg_dump/pg_backup_archiver.c:1950:32: warning: comparison of constant -1 with expression of type
'ArchiveFormat' (aka 'enum _archiveFormat') is always false [-Wtautological-constant-out-of-range-compare]
if ((AH->format = fgetc(fh)) == EOF)
~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~
1 warning generated.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

--On 9. Februar 2014 13:55:03 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:

Agreed; we should be using the same coding pattern wherever we call
ftello.

I suspect that this code may be left over from coping with some ancient
non-spec-compliant version of ftello? Probably not worth digging in
the archives to find out. The Single Unix Spec v2 says that the result
is (off_t) -1 on error, and we generally assume that platforms are at
least compliant with that.

grep shows me a couple of other places where the result of ftello doesn't
seem to be getting checked for error. Odd that Coverity didn't notice
those.

It stroke me today that there's still something broken. pg_dump fails when
used in custom archive mode and piping to e.g. pg_restore:

pg_dump -Fc -p 5447 regression | pg_restore
pg_dump: [custom archiver] could not determine seek position in archive
file: Illegal seek
pg_restore: [custom archiver] unexpected end of file

pg_dump fails in _CloseArchive() with this hunk:

+		if (tpos < 0 || errno)
+			exit_horribly(modulename, "could not determine seek position in archive 
file: %s\n",
+						  strerror(errno));

errno is set to 29, which is ESPIPE and tpos was set to -1. If I read the
manpage on OSX here correctly, ftell[o] will always fail if used with a
pipe in this case:

[ESPIPE] The file descriptor underlying stream is associated
with a pipe or FIFO or file-position indicator
value is unspecified (see ungetc(3)).

A quick check shows that Debian has the same issue.

--
Thanks

Bernd

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Bernd Helmle <mailings@oopsware.de> writes:

It stroke me today that there's still something broken. pg_dump fails when
used in custom archive mode and piping to e.g. pg_restore:

pg_dump -Fc -p 5447 regression | pg_restore
pg_dump: [custom archiver] could not determine seek position in archive
file: Illegal seek

Ugh. Yeah, pg_dump to a pipe fails for me as well, and only in HEAD not
older versions. Will look.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers