Re: sequence data type

On 1/25/17 11:57 PM, Michael Paquier wrote:
> @@ -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.

Generally, we don't add default values, to keep the dumps from being too
verbose.

(I also think that being able to restore dumps to older versions would
be nice, but that's another discussion.)

> 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

Yeah, I had taken some notes along the way to add more test coverage, so
since you're interested, attached is a patch. It's independent of the
current patch and overlaps slightly. The example you show isn't really
a new code path, just triggered differently, but the enhanced tests will
cover it nonetheless.

> =# 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.

Yeah, I think that should be rephrased as something like "out of
bounds", which is the term used elsewhere.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services