Re: [HACKERS] sequence data type

On Sat, Jan 28, 2017 at 2:49 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> 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.)

Okay. Fine for me.

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

Sure. Thanks for looking into that and getting a patch out. Oh, I have
just noticed that sequence_1.out has been removed by 9c18104c. That's
nice.

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

OK, that sounds good.

Looking at the patch adding some new tests, the coverage really
increases (I did not run make coverage to be honest, but that's
clearly an improvement).

Another test that could be added is about nextval() and setval() that
only work for temporary sequences in a read-only transaction:
create sequence foo;
create temp sequence footemp;
begin read only;
select nextval('footemp'); -- ok
select nextval('foo'); -- error
rollback;
begin read only;
select setval('footemp', 1); -- ok
select setval('foo', 1); -- error
rollback

But it is a bit funky I agree.
-- 
Michael