Обсуждение: pg_dump sometimes misses sequence parameters

The bug affects REL_10_STABLE and master branches.
9.4..9.6 are unaffected.

To reproduce:
psql -c 'DROP SEQUENCE IF EXISTS foo'
psql -c 'CREATE SEQUENCE foo INCREMENT BY -1 MINVALUE 
-9223372036854775808 MAXVALUE 9223372036854775807'
pg_dump -t foo > tmp
psql -c 'DROP SEQUENCE foo'
psql <tmp

The last psql call fails with "START value (9223372036854775807) cannot 
be greater than MAXVALUE (-1)",
as pg_dump does not record MAXVALUE properly.

The reason is atoi is used and those large numbers are interpreted as 0 
and -1 respectively.
I'd propose to use atol instead of atoi, please see the patch attached.

Best,
   Alexey

Alexey Bashtanov <bashtanov@imap.cc> writes:
> The bug affects REL_10_STABLE and master branches.
> 9.4..9.6 are unaffected.

> To reproduce:
> psql -c 'DROP SEQUENCE IF EXISTS foo'
> psql -c 'CREATE SEQUENCE foo INCREMENT BY -1 MINVALUE 
> -9223372036854775808 MAXVALUE 9223372036854775807'
> pg_dump -t foo > tmp
> psql -c 'DROP SEQUENCE foo'
> psql <tmp

> The last psql call fails with "START value (9223372036854775807) cannot 
> be greater than MAXVALUE (-1)",
> as pg_dump does not record MAXVALUE properly.

Ugh.

> The reason is atoi is used and those large numbers are interpreted as 0 
> and -1 respectively.
> I'd propose to use atol instead of atoi, please see the patch attached.

That will only fix it on platforms where long is 64 bits.  I think that
converting these strings to integer at all is a dumb idea.  It would be
much better to write the first two tests like

    if (is_ascending && strcmp(minv, "1") == 0)
        minv = NULL;
    if (!is_ascending && strcmp(maxv, "-1") == 0)
        maxv = NULL;

It's tempting to try to make the remaining tests look similar, but
I'm not quite sure how without writing out the exact decimal values
of the constants, which probably isn't better.  I experimented with
stuff like "strcmp(minv, CppAsString2(PG_INT32_MIN))" but that
doesn't quite do what we need.

            regards, tom lane

Hi Tom,

That will only fix it on platforms where long is 64 bits.

Oops. I erroneously thought its size could drift the other way.

I think that converting these strings to integer at all is a dumb idea.  It would be
much better to write the first two tests like

    if (is_ascending && strcmp(minv, "1") == 0)
        minv = NULL;
    if (!is_ascending && strcmp(maxv, "-1") == 0)
        maxv = NULL;

Agree for those two. However, I don't see it as a substantial speedup
or simplification if we anyway do the conversions below.

It's tempting to try to make the remaining tests look similar, but
I'm not quite sure how without writing out the exact decimal values
of the constants, which probably isn't better.  I experimented with
stuff like "strcmp(minv, CppAsString2(PG_INT32_MIN))" but that
doesn't quite do what we need.

I didn't manage to find a snprintf-like thing implemented in C
preprocessor language either, and I don't think it would be easy
to write one.

Unwilling to define the decimal representations constants, we have to
convert those strings representations to binary ones
(in case of smallint/integer sequences) or visa versa (bigint).

Current code looks somewhat fragile to me, as it takes some time to assure
it doesn't pass already nulled out minv or maxv to atoi or strcmp again.

I've rewritten it in a bit more clear way, so each comparison and nulling out
happens only once. Choosing between atol/atoll vs snprintf+strcmp,
I preferred the former, as the latter is slower and uglier to me.

Should we need any further speedup, I'd go for decimal representation
constants really.

BTW what should I do when submitting a patch if my autoconf adds
some irrelevant changes?

Best regards,
  Alexey Bashtanov

Alexey Bashtanov <bashtanov@imap.cc> writes:
> Current code looks somewhat fragile to me, as it takes some time to assure
> it doesn't pass already nulled out minv or maxv to atoi or strcmp again.
> I've rewritten it in a bit more clear way, so each comparison and 
> nulling out happens only once.

Good idea ...

> Choosing between atol/atoll vs snprintf+strcmp,
> I preferred the former, as the latter is slower and uglier to me.

... but I was not thrilled with the way you did that.  I don't like
having to back-patch new portability requirements with no time for them
to go through beta testing.  What's more, this is randomly unlike the way
we're doing things elsewhere.  I call your attention to pg_strtouint64
in src/backend/utils/adt/numutils.c.

What I did for the moment was to implement the comparison using
snprintf+strcmp, since that seemed like a very safely back-patchable
approach.  While there's nothing particularly wrong with leaving
it like that, we could imagine creating a src/port module that
contains pg_strtouint64 and a corresponding implementation of
pg_strtoint64, and then fixing pg_dump to use pg_strtoint64 here.
I'd only consider doing that in HEAD though.

> BTW what should I do when submitting a patch if my autoconf adds
> some irrelevant changes?

The easy way is just to omit the configure file from the submitted
patch, noting that the committer must run autoconf.  Most committers
probably would do that anyway (I certainly would).

However, if you want to do it right, or if you're someday given a
commit bit and you have to do it right, our project policy is that
we use exactly the GNU-released autoconf of the appropriate version,
so that anyone can reproduce the results, independently of patches
that their distribution might install into their platform's copy of
autoconf.  So just pull the right autoconf tarball from any handy
GNU archive mirror, "configure --prefix=/something; make; make install",
prepend /something/bin to your PATH, and away you go.  Personally
I keep autoconf N.NN in /usr/local/autoconf-N.NN/ so that I can use
whichever version is appropriate for the PG branch I'm working on.

            regards, tom lane