Thread: sequence data type

Here is a patch that adds the notion of a data type to a sequence.  So
it might be CREATE SEQUENCE foo AS integer.  The types are restricted to
int{2,4,8} as now.

The main point of this is to make monitoring sequences less complicated.
 Right now, a serial column creates an int4 column but creates the
sequence with a max value for int8.  So in order to correctly answer the
question, is the sequence about to run out, you need to look not only at
the sequence but also any columns it is associated with.  check_postgres
figures this out, but it's complicated and slow, and not easy to do
manually.

If you tell the sequence the data type you have in mind, it
automatically sets appropriate min and max values.  Serial columns also
make use of this, so the sequence type automatically matches the column
type.

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

On 08/31/2016 06:22 AM, Peter Eisentraut wrote:
> Here is a patch that adds the notion of a data type to a sequence.  So
> it might be CREATE SEQUENCE foo AS integer.  The types are restricted to
> int{2,4,8} as now.

This patch does not apply cleanly to current master (=600dc4c).
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On 04/09/16 06:41, Vik Fearing wrote:
> On 08/31/2016 06:22 AM, Peter Eisentraut wrote:
>> Here is a patch that adds the notion of a data type to a sequence.  So
>> it might be CREATE SEQUENCE foo AS integer.  The types are restricted to
>> int{2,4,8} as now.
> This patch does not apply cleanly to current master (=600dc4c).

Must admit I first thought of: "2, 4, 8, who do we appreciate!"


MORE SERIOUSLY:

Would a possibly future expansion be to include numeric?

Of hand, I can't see any value for using other than integers of 2, 4, & 
8 bytes (not a criticism! - may simply be a failure of imagination on my 
part).

I am curious as to the use cases for other possibilities.


Cheers,
Gavin

On 9/3/16 2:41 PM, Vik Fearing wrote:
> On 08/31/2016 06:22 AM, Peter Eisentraut wrote:
>> Here is a patch that adds the notion of a data type to a sequence.  So
>> it might be CREATE SEQUENCE foo AS integer.  The types are restricted to
>> int{2,4,8} as now.
>
> This patch does not apply cleanly to current master (=600dc4c).

Updated patch attached.

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

On 9/3/16 6:01 PM, Gavin Flower wrote:
> I am curious as to the use cases for other possibilities.

A hex or base64 type might be interesting, should those ever get created...
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461

On 9/10/16, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 9/3/16 6:01 PM, Gavin Flower wrote:
>> I am curious as to the use cases for other possibilities.
>
> A hex or base64 type might be interesting, should those ever get created...
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX

Hex or base64 are not data types. They are just different
representation types of binary sequences.
Even for bigints these representations are done after writing numbers
as byte sequences.

-- 
Best regards,
Vitaly Burovoy

On Sun, Sep 11, 2016 at 12:38 PM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
> On 9/10/16, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
>> On 9/3/16 6:01 PM, Gavin Flower wrote:
>>> I am curious as to the use cases for other possibilities.
>>
>> A hex or base64 type might be interesting, should those ever get created...
>> --
>> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
>
> Hex or base64 are not data types. They are just different
> representation types of binary sequences.
> Even for bigints these representations are done after writing numbers
> as byte sequences.

Moved to next CF. The patch did not actually receive that much reviews.
-- 
Michael

On 9/8/16 4:06 PM, Peter Eisentraut wrote:
> On 9/3/16 2:41 PM, Vik Fearing wrote:
>> On 08/31/2016 06:22 AM, Peter Eisentraut wrote:
>>> Here is a patch that adds the notion of a data type to a sequence.  So
>>> it might be CREATE SEQUENCE foo AS integer.  The types are restricted to
>>> int{2,4,8} as now.
>>
>> This patch does not apply cleanly to current master (=600dc4c).
> 
> Updated patch attached.

Another updated patch, with quite a bit of rebasing and some minor code
polishing.

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

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

On 12/31/2016 01:27 AM, Peter Eisentraut wrote:
> Another updated patch, with quite a bit of rebasing and some minor 
> code polishing.
>

Patch applies cleanly and the tests pass
The feature seems to work as expected.

I've tried this out and it behaves as expected and desired. I also 
tested the PG dump changes when dumping from both 8.3 and 9.5 and tables 
with serial types and standalone sequences restore as I would expect.


The only concern I have with the functionality is that there isn't a way 
to change the type of a sequence.

For example

create table foo(id serial4);
--hmm I"m running out of id space
alter table foo alter column id type int8;
alter sequence foo_id_seq maxvalue
9223372036854775807;

2017-01-08 14:29:27.073 EST [4935] STATEMENT:  alter sequence foo_id_seq 
maxvalue    9223372036854775807;
ERROR:  MAXVALUE (9223372036854775807) is too large for sequence data 
type integer

Since we allow for min/maxvalue to be changed I feel we should also 
allow the type to be changed.




@@ -1236,7 +1239,15 @@ init_params(ParseState *pstate, List *options, 
bool isInit,        {                DefElem    *defel = (DefElem *) lfirst(option);

-               if (strcmp(defel->defname, "increment") == 0)
+               if (strcmp(defel->defname, "as") == 0)
+               {
+                       if (as_type)
+                               ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+                                                errmsg("conflicting or 
redundant options")));
+                       as_type = defel;
+               }
+               else if (strcmp(defel->defname, "increment") == 0)

Should we including parser_errposition(pstate, defel->location)));  like 
for the other errors below this?


Other than that the patch looks good

On 1/8/17 2:39 PM, Steve Singer wrote:
> The only concern I have with the functionality is that there isn't a way 
> to change the type of a sequence.

If we implement the NEXT VALUE FOR expression (or anything similar that
returns a value from the sequence), then the return type of that
expression would be the type of the sequence.  Then, changing the type
of the sequence would require reparsing all expressions involving the
sequence.  This could probably be sorted out somehow, but I don't want
to be too lax now and cause problems for later features.  There is a
similar case, namely changing the return type of a function, which we
also prohibit.

> @@ -1236,7 +1239,15 @@ init_params(ParseState *pstate, List *options, 
> bool isInit,
>          {
>                  DefElem    *defel = (DefElem *) lfirst(option);
> 
> -               if (strcmp(defel->defname, "increment") == 0)
> +               if (strcmp(defel->defname, "as") == 0)
> +               {
> +                       if (as_type)
> +                               ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> +                                                errmsg("conflicting or 
> redundant options")));
> +                       as_type = defel;
> +               }
> +               else if (strcmp(defel->defname, "increment") == 0)
> 
> Should we including parser_errposition(pstate, defel->location)));  like 
> for the other errors below this?

Yes, good catch.

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

On Tue, Jan 10, 2017 at 5:03 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/8/17 2:39 PM, Steve Singer wrote:
>> The only concern I have with the functionality is that there isn't a way
>> to change the type of a sequence.
>
> If we implement the NEXT VALUE FOR expression (or anything similar that
> returns a value from the sequence), then the return type of that
> expression would be the type of the sequence.  Then, changing the type
> of the sequence would require reparsing all expressions involving the
> sequence.  This could probably be sorted out somehow, but I don't want
> to be too lax now and cause problems for later features.  There is a
> similar case, namely changing the return type of a function, which we
> also prohibit.
>
>> @@ -1236,7 +1239,15 @@ init_params(ParseState *pstate, List *options,
>> bool isInit,
>>          {
>>                  DefElem    *defel = (DefElem *) lfirst(option);
>>
>> -               if (strcmp(defel->defname, "increment") == 0)
>> +               if (strcmp(defel->defname, "as") == 0)
>> +               {
>> +                       if (as_type)
>> +                               ereport(ERROR,
>> + (errcode(ERRCODE_SYNTAX_ERROR),
>> +                                                errmsg("conflicting or
>> redundant options")));
>> +                       as_type = defel;
>> +               }
>> +               else if (strcmp(defel->defname, "increment") == 0)
>>
>> Should we including parser_errposition(pstate, defel->location)));  like
>> for the other errors below this?
>
> Yes, good catch.

+           ereport(ERROR,
+               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                errmsg("MINVALUE (%s) is too large for sequence data type %s",
+                       bufm, format_type_be(seqform->seqtypid))));
"too large" for a minimum value, really? You may want to add a comment
to say that the _MAX values are used for symmetry.

+           /* We use the _MAX constants for symmetry. */
+           if (seqform->seqtypid == INT2OID)
+               seqform->seqmin = -PG_INT16_MAX;
+           else if (seqform->seqtypid == INT4OID)
+               seqform->seqmin = -PG_INT32_MAX;
+           else
+               seqform->seqmin = -PG_INT64_MAX;
Hm. Is symmetry an important properly for sequences? It seems to me
that if we map with the data types we had better use the MIN values
instead for consistency. So the concept of this patch is rather weird
and would introduce an exception with the rest of the system just for
sequences.

(There are no tests for cycles).
-- 
Michael

    Peter Eisentraut wrote:

> This could probably be sorted out somehow, but I don't want
> to be too lax now and cause problems for later features.  There is a
> similar case, namely changing the return type of a function, which we
> also prohibit.

Consider the case of a table with a SERIAL column which later
has to become a BIGINT due to growth.
Currently a user would just alter the column's type and does
need to do anything with the sequence.

With the patch, it becomes a problem because

- ALTER SEQUENCE seqname MAXVALUE new_value
will fail because new_value is beyond the range of INT4.

- ALTER SEQUENCE seqname TYPE BIGINT
does not exist (yet?)

- DROP SEQUENCE seqname  (with the idea of recreating the
sequence immediately after) will be rejected because the table
depends on the sequence.

What should a user do to upgrade the SERIAL column?

BTW, I notice that a direct UPDATE of pg_sequence happens
to work (now that we have pg_sequence thanks to your other
recent contributions on sequences), but I guess it falls under the
rule mentioned in
https://www.postgresql.org/docs/devel/static/catalogs.html

"You can drop and recreate the tables, add columns, insert and update values,
and severely mess up your system that way. Normally, one should not change
the system catalogs by hand, there are normally SQL commands to do that"

Previously, UPDATE seqname SET property=value was rejected with
a specific error "cannot change sequence".

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

    Michael Paquier wrote:

> Hm. Is symmetry an important properly for sequences? It seems to me
> that if we map with the data types we had better use the MIN values
> instead for consistency. So the concept of this patch is rather weird
> and would introduce an exception with the rest of the system just for
> sequences.

Besides there's a related compatibility break in that,
if a sequence is created in an existing release like this:

CREATE SEQUENCE s MINVALUE -9223372036854775808;

And then it's dumped/reloaded on a backend that has
the patch applied, it fails with:
MINVALUE (-9223372036854775808) is too large for sequence data type bigint

This value (-2^63) is legal in current releases, although it happens
to be off-by-1 compared to the default minvalue for a sequence going
downwards. Arguably it's the default that is weird.

I've started the thread at [1] to discuss whether it makes sense
in the first place that our CREATE SEQUENCE takes -(2^63)+1 as the
default minvalue rather than -2^63, independantly of this patch.

I think the current patch transforms this oddity into an actual
issue  by making it impossible to reach the real minimum of
a sequence with regard to the type tied to it (-2^15 for smallint,
-2^31 for int, -2^63 for bigint), whereas in HEAD you can still
adjust minvalue to fix the off-by-1 against the bigint range, if you
happen to care about it, the only problem being that you first
need to figure this out by yourself.


[1]
https://www.postgresql.org/message-id/4865a75e-f490-4e9b-b8e7-3d78694c3178@manitou-mail.org


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

    Peter Eisentraut wrote:

> So in order to correctly answer the question, is the sequence about to
> run out, you need to look not only at
> the sequence but also any columns it is associated with.  check_postgres
> figures this out, but it's complicated and slow, and not easy to do
> manually.

It strikes me that this is a compelling argument for setting a sensible
MAXVALUE when creating a sequence for a SERIAL, rather than binding
the sequence to a datatype.

In existing releases the SERIAL code sets the maxvalue to 2^63 even
though it knows that the column is limited to 2^31.
It looks like setting it to 2^31 would be enough for the sake of
monitoring.

More generally, what is of interest for the monitoring is how close
the sequence's last_value is from its max_value, independently of an
underlying type.

2^{15,31,63} are specific cases of particular interest, but there's
no reason to only check for these limits when you can do it
for every possible limit.

For instance if a user has a business need to limit an ID to 1 billion,
they should alter the sequence to a MAXVALUE of 1 billion, and be
interested in how close they are from that, not from 2^31.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

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.

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

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

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

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

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

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

On 1/30/17 12:42 AM, Michael Paquier wrote:
> 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.

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

Looks useful to me.  I have committed the tests with your addition.

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

And here is a rebased patch for the original feature.  I think this
addresses all raised concerns and suggestions now.

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

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

On Wed, Feb 1, 2017 at 1:11 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> And here is a rebased patch for the original feature.  I think this
> addresses all raised concerns and suggestions now.

Thanks for the new version. That looks good to me after an extra lookup.
-- 
Michael

On Wed, Feb 1, 2017 at 10:02 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 1, 2017 at 1:11 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> And here is a rebased patch for the original feature.  I think this
>> addresses all raised concerns and suggestions now.
>
> Thanks for the new version. That looks good to me after an extra lookup.

Moved to CF 2017-03.
-- 
Michael

On 2/1/17 10:01 PM, Michael Paquier wrote:
> On Wed, Feb 1, 2017 at 10:02 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Feb 1, 2017 at 1:11 AM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> And here is a rebased patch for the original feature.  I think this
>>> addresses all raised concerns and suggestions now.
>>
>> Thanks for the new version. That looks good to me after an extra lookup.
> 
> Moved to CF 2017-03.

committed, thanks

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

Over at
<https://www.postgresql.org/message-id/CAKOSWNnXmM6YBXNzGnXtZQMPjDgJF+a3Wx53Wzmrq5wqDyRX7Q@mail.gmail.com>
is is being discussed that maybe the behavior when altering the sequence
type isn't so great, because it currently doesn't update the min/max
values of the sequence at all.  So here is a patch to update the min/max
values when the old min/max values were the min/max values of the data type.

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

On Thu, Mar 30, 2017 at 2:36 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Over at
> <https://www.postgresql.org/message-id/CAKOSWNnXmM6YBXNzGnXtZQMPjDgJF+a3Wx53Wzmrq5wqDyRX7Q@mail.gmail.com>
> is is being discussed that maybe the behavior when altering the sequence
> type isn't so great, because it currently doesn't update the min/max
> values of the sequence at all.  So here is a patch to update the min/max
> values when the old min/max values were the min/max values of the data type.

Looks sane to me. The only comment I have would be to add a test to
trigger as well the code path of reset_min_value with MINVALUE.
-- 
Michael

On 3/29/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> Over at
> <https://www.postgresql.org/message-id/CAKOSWNnXmM6YBXNzGnXtZQMPjDgJF+a3Wx53Wzmrq5wqDyRX7Q@mail.gmail.com>
> is is being discussed that maybe the behavior when altering the sequence
> type isn't so great, because it currently doesn't update the min/max
> values of the sequence at all.  So here is a patch to update the min/max
> values when the old min/max values were the min/max values of the data
> type.
>
> --
> Peter Eisentraut              http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

It seems almost good for me except a single thing (I'm sorry, I missed
the previous discussion).
Why is min_value set to 1 (or -1 for negative INCREMENTs) by default
for all sequence types?
With the committed patch it leads to the extra "MINVALUE" option
besides the "START" one; and it is not the worst thing.

It leads to strange error for countdown sequences:
postgres=# CREATE SEQUENCE abc AS smallint MINVALUE 0 START 20000 INCREMENT -1;
ERROR:  MINVALUE (0) must be less than MAXVALUE (-1)

postgres=# CREATE SEQUENCE abc AS smallint MINVALUE 0 START 20000
INCREMENT -1 NO MAXVALUE; -- does not help
ERROR:  MINVALUE (0) must be less than MAXVALUE (-1)


With the proposed patch users can impact with the next error:

postgres=# CREATE TABLE tbl(i smallserial);
CREATE TABLE
postgres=# SELECT * FROM pg_sequences;schemaname | sequencename | sequenceowner | data_type | start_value |
min_value | max_value | increment_by | cycle | cache_size | last_value

------------+--------------+---------------+-----------+-------------+-----------+-----------+--------------+-------+------------+------------public
   | tbl_i_seq    | burovoy_va    | smallint  |           1 |       1 |     32767 |            1 | f     |          1
|
(1 row)

postgres=# -- min_value for smallint is "1"? Ok, I want to use the whole range:
postgres=# ALTER SEQUENCE tbl_i_seq MINVALUE -32768 START -32768 RESTART -32768;
ALTER SEQUENCE
postgres=# -- after a while I realized the range is not enough. Try to
enlarge it:
postgres=# ALTER SEQUENCE tbl_i_seq AS integer;
ERROR:  START value (-32768) cannot be less than MINVALUE (1)

It is not an expected behavior.

I think min_value and max_value should not be set to "1" or "-1" but
to real min/max of the type by default.

I recommend to add to the docs explicit phrase that START value is not
changed even if it matches the bound of the original type.

Also it is good to have regressions like:
CREATE SEQUENCE sequence_test10 AS smallint  MINVALUE -1000 MAXVALUE 1000;
ALTER SEQUENCE sequence_test10 AS int NO MINVALUE NO MAXVALUE INCREMENT 1;
ALTER SEQUENCE sequence_test10 AS bigint NO MINVALUE NO MAXVALUE INCREMENT -1;

CREATE SEQUENCE sequence_test11 AS smallint  MINVALUE -32768 MAXVALUE 32767;
ALTER SEQUENCE sequence_test11 AS int NO MINVALUE NO MAXVALUE INCREMENT 1;
ALTER SEQUENCE sequence_test11 AS int NO MINVALUE NO MAXVALUE INCREMENT -1;

-- 
Best regards,
Vitaly Burovoy

On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
> I think min_value and max_value should not be set to "1" or "-1" but
> to real min/max of the type by default.

This is the default behavior for ages, since e8647c45 to be exact. So
you would change 20 years of history?
-- 
Michael

On 3/29/17, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
> <vitaly.burovoy@gmail.com> wrote:
>> I think min_value and max_value should not be set to "1" or "-1" but
>> to real min/max of the type by default.
>
> This is the default behavior for ages, since e8647c45 to be exact. So
> you would change 20 years of history?
> --
> Michael
>

Unfortunately yes, because the current patch has appeared because of
another patch with the "IDENTITY COLUMN" feature.
Since sequences used for such columns are completely hidden (they do
not appear in DEFAULTs like "serial" do) and a new option (sequence
data type) is supported with its altering (which was absent
previously), new errors appear because of that.

Be honest I did not checked the "countdown" case at the current
release, but the most important part is the second one because it is a
direct analogue of what happens (in the parallel thread) on "ALTER
TABLE tbl ALTER col TYPE new_type" where "col" is an identity column.

Since the commit 2ea5b06c7a7056dca0af1610aadebe608fbcca08 declares
"The data type determines the default minimum and maximum values of
the sequence." is it a wrong way to keep historical minimum as "1" by
default: it is not a minimum of any of supported type.

I don't think something will break if min_value is set lesser than
before, but it will decrease astonishment of users in cases I wrote in
the previous email.

-- 
Best regards,
Vitaly Burovoy

On 3/29/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> On 3/29/17, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>> <vitaly.burovoy@gmail.com> wrote:
>>> I think min_value and max_value should not be set to "1" or "-1" but
>>> to real min/max of the type by default.
>>
>> This is the default behavior for ages, since e8647c45 to be exact. So
>> you would change 20 years of history?
>
> ... is it a wrong way to keep historical minimum as "1" by
> default: it is not a minimum of any of supported type.

I've read the standard about "minvalue", "maxvalue" and "start".
OK, I was wrong. Since "start" should be equal to "minvalue" unless
defined explicitly, the only bug left from my first email here is
resetting "minvalue" back to 1 when data type changes and if the value
matches the bound of the old type (the last case there).

P.S.: the same thing with "maxvalue" when "increment" is negative.

-- 
Best regards,
Vitaly Burovoy

On 3/30/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> On 3/29/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
>> On 3/29/17, Michael Paquier <michael.paquier@gmail.com> wrote:
>>> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>>> <vitaly.burovoy@gmail.com> wrote:
>>>> I think min_value and max_value should not be set to "1" or "-1" but
>>>> to real min/max of the type by default.
>>>
>>> This is the default behavior for ages, since e8647c45 to be exact. So
>>> you would change 20 years of history?
>>
>> ... is it a wrong way to keep historical minimum as "1" by
>> default: it is not a minimum of any of supported type.
>
> I've read the standard about "minvalue", "maxvalue" and "start".
> OK, I was wrong. Since "start" should be equal to "minvalue" unless
> defined explicitly, the only bug left from my first email here is
> resetting "minvalue" back to 1 when data type changes and if the value
> matches the bound of the old type (the last case there).
>
> P.S.: the same thing with "maxvalue" when "increment" is negative.

It seemed not very hard to fix it.
Please find attached patch.

I've added more tests to cover different cases of changing bounds when
data type is changed.

-- 
Best regards,
Vitaly Burovoy

On 3/30/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> On 3/29/17, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
>> On 3/29/17, Michael Paquier <michael.paquier@gmail.com> wrote:
>>> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>>> <vitaly.burovoy@gmail.com> wrote:
>>>> I think min_value and max_value should not be set to "1" or "-1" but
>>>> to real min/max of the type by default.
>>>
>>> This is the default behavior for ages, since e8647c45 to be exact. So
>>> you would change 20 years of history?
>>
>> ... is it a wrong way to keep historical minimum as "1" by
>> default: it is not a minimum of any of supported type.
>
> I've read the standard about "minvalue", "maxvalue" and "start".
> OK, I was wrong. Since "start" should be equal to "minvalue" unless
> defined explicitly, the only bug left from my first email here is
> resetting "minvalue" back to 1 when data type changes and if the value
> matches the bound of the old type (the last case there).
>
> P.S.: the same thing with "maxvalue" when "increment" is negative.

It seemed not very hard to fix it.
Please find attached patch to be applied on top of your one.

I've added more tests to cover different cases of changing bounds when
data type is changed.

-- 
Best regards,
Vitaly Burovoy

On 3/30/17 22:47, Vitaly Burovoy wrote:
> It seemed not very hard to fix it.
> Please find attached patch to be applied on top of your one.
> 
> I've added more tests to cover different cases of changing bounds when
> data type is changed.

Committed all that.  Thanks!

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

On 4/4/17, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> On 3/30/17 22:47, Vitaly Burovoy wrote:
>> It seemed not very hard to fix it.
>> Please find attached patch to be applied on top of your one.
>>
>> I've added more tests to cover different cases of changing bounds when
>> data type is changed.
>
> Committed all that.  Thanks!

Thank you very much!

-- 
Best regards,
Vitaly Burovoy