Re: [HACKERS] sequence data type

On Wed, Jan 25, 2017 at 11:53 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Here is an updated patch that allows changing the sequence type.  This
> was clearly a concern for reviewers, and the presented use cases seemed
> convincing.

I have been torturing this patch and it looks rather solid to me. Here
are a couple of comments:

@@ -15984,6 +15992,9 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)                     "CREATE SEQUENCE %s\n",
              fmtId(tbinfo->dobj.name));
 

+   if (strcmp(seqtype, "bigint") != 0)
+       appendPQExpBuffer(query, "    AS %s\n", seqtype);
Wouldn't it be better to assign that unconditionally? There is no
reason that a dump taken from pg_dump version X will work on X - 1 (as
there is no reason to not make the life of users uselessly difficult
as that's your point), but that seems better to me than rely on the
sequence type hardcoded in all the pre-10 dump queries for sequences.
That would bring also more consistency to the CREATE SEQUENCE queries
of test_pg_dump/t/001_base.pl.

Could you increase the regression test coverage to cover some of the
new code paths? For example such cases are not tested:
=# create sequence toto as smallint;
CREATE SEQUENCE
=# alter sequence toto as smallint maxvalue 1000;
ERROR:  22023: RESTART value (2147483646) cannot be greater than MAXVALUE (1000)
LOCATION:  init_params, sequence.c:1537
=# select setval('toto', 1);setval
--------     1
(1 row)
=# alter sequence toto as smallint;
ERROR:  22023: MAXVALUE (2147483647) is too large for sequence data
type smallint
LOCATION:  init_params, sequence.c:1407

+   if ((seqform->seqtypid == INT2OID && seqform->seqmin < PG_INT16_MIN)
+       || (seqform->seqtypid == INT4OID && seqform->seqmin < PG_INT32_MIN)
+       || (seqform->seqtypid == INT8OID && seqform->seqmin < PG_INT64_MIN))
+   {
+       char        bufm[100];
+
+       snprintf(bufm, sizeof(bufm), INT64_FORMAT, seqform->seqmin);
+
+       ereport(ERROR,
+               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                errmsg("MINVALUE (%s) is too large for sequence data type %s",
+                       bufm, format_type_be(seqform->seqtypid))));
+   }
"large" does not apply to values lower than the minimum, no? The int64
path is never going to be reached (same for the max value), it doesn't
hurt to code it I agree.

Testing serial columns, the changes are consistent with the previous releases.
-- 
Michael