Re: [HACKERS] sequence data type

2017-04-13 Thread Vitaly Burovoy 
On 4/4/17, Peter Eisentraut  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


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

Re: [HACKERS] sequence data type

2017-04-04 Thread Peter Eisentraut 
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


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

Re: [HACKERS] sequence data type

2017-03-30 Thread Vitaly Burovoy 
On 3/30/17, Vitaly Burovoy  wrote:
> On 3/29/17, Vitaly Burovoy  wrote:
>> On 3/29/17, Michael Paquier  wrote:
>>> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>>>  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


0002-Fix-overriding-max-min-value-of-a-sequence-to-1-on-A.patch
Description: Binary data

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

Re: [HACKERS] sequence data type

2017-03-30 Thread Vitaly Burovoy 
On 3/30/17, Vitaly Burovoy  wrote:
> On 3/29/17, Vitaly Burovoy  wrote:
>> On 3/29/17, Michael Paquier  wrote:
>>> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>>>  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


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

Re: [HACKERS] sequence data type

2017-03-30 Thread Vitaly Burovoy 
On 3/29/17, Vitaly Burovoy  wrote:
> On 3/29/17, Michael Paquier  wrote:
>> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>>  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


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

Re: [HACKERS] sequence data type

2017-03-29 Thread Vitaly Burovoy 
On 3/29/17, Michael Paquier  wrote:
> On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
>  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


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

Re: [HACKERS] sequence data type

2017-03-29 Thread Michael Paquier 
On Thu, Mar 30, 2017 at 11:18 AM, Vitaly Burovoy
 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


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

Re: [HACKERS] sequence data type

2017-03-29 Thread Vitaly Burovoy 
On 3/29/17, Peter Eisentraut  wrote:
> Over at
> 
> 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 2 INCREMENT -1;
ERROR:  MINVALUE (0) must be less than MAXVALUE (-1)

postgres=# CREATE SEQUENCE abc AS smallint MINVALUE 0 START 2
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


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

Re: [HACKERS] sequence data type

2017-03-29 Thread Michael Paquier 
On Thu, Mar 30, 2017 at 2:36 AM, Peter Eisentraut
 wrote:
> Over at
> 
> 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


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

Re: [HACKERS] sequence data type

2017-03-29 Thread Peter Eisentraut 
Over at

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


0001-Adjust-min-max-values-when-changing-sequence-type.patch
Description: invalid/octet-stream

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

Re: [HACKERS] sequence data type

2017-02-10 Thread Peter Eisentraut 
On 2/1/17 10:01 PM, Michael Paquier wrote:
> On Wed, Feb 1, 2017 at 10:02 AM, Michael Paquier
>  wrote:
>> On Wed, Feb 1, 2017 at 1:11 AM, Peter Eisentraut
>>  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


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

Re: [HACKERS] sequence data type

2017-02-01 Thread Michael Paquier 
On Wed, Feb 1, 2017 at 10:02 AM, Michael Paquier
 wrote:
> On Wed, Feb 1, 2017 at 1:11 AM, Peter Eisentraut
>  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


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

Re: [HACKERS] sequence data type

2017-01-31 Thread Michael Paquier 
On Wed, Feb 1, 2017 at 1:11 AM, Peter Eisentraut
 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


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

Re: [HACKERS] sequence data type

2017-01-31 Thread Peter Eisentraut 
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
>From ce2680ef072a9a4dc2cb879a70610d71ad24 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 27 Jan 2017 23:52:53 -0500
Subject: [PATCH v5] Add CREATE SEQUENCE AS  clause

This stores a data type, required to be an integer type, with the
sequence.  The sequences min and max values default to the range
supported by the type, and they cannot be set to values exceeding that
range.  The internal implementation of the sequence is not affected.

Change the serial types to create sequences of the appropriate type.
This makes sure that the min and max values of the sequence for a serial
column match the range of values supported by the table column.  So the
sequence can no longer overflow the table column.

This also makes monitoring for sequence exhaustion/wraparound easier,
which currently requires various contortions to cross-reference the
sequences with the table columns they are used with.

This commit also effectively reverts the pg_sequence column reordering
in f3b421da5f4addc95812b9db05a24972b8fd9739, because the new seqtypid
column allows us to fill the hole in the struct and create a more
natural overall column ordering.
---
 doc/src/sgml/catalogs.sgml  |  14 +++-
 doc/src/sgml/information_schema.sgml|   4 +-
 doc/src/sgml/ref/alter_sequence.sgml|  30 ++--
 doc/src/sgml/ref/create_sequence.sgml   |  36 ++
 src/backend/catalog/information_schema.sql  |   4 +-
 src/backend/commands/sequence.c |  89 +---
 src/backend/parser/gram.y   |   6 +-
 src/backend/parser/parse_utilcmd.c  |   2 +-
 src/bin/pg_dump/pg_dump.c   | 103 +++-
 src/bin/pg_dump/t/002_pg_dump.pl|   2 +
 src/include/catalog/catversion.h|   2 +-
 src/include/catalog/pg_proc.h   |   2 +-
 src/include/catalog/pg_sequence.h   |   8 ++-
 src/test/modules/test_pg_dump/t/001_base.pl |   1 +
 src/test/regress/expected/sequence.out  |  49 +
 src/test/regress/sql/sequence.sql   |  13 
 16 files changed, 266 insertions(+), 99 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 086fafc694..f57937a17a 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -5774,10 +5774,11 @@ pg_sequence Columns
  
 
  
-  seqcycle
-  bool
+  seqtypid
+  oid
+  pg_type.oid
   
-  Whether the sequence cycles
+  Data type of the sequence
  
 
  
@@ -5814,6 +5815,13 @@ pg_sequence Columns
   
   Cache size of the sequence
  
+
+ 
+  seqcycle
+  bool
+  
+  Whether the sequence cycles
+ 
 

   
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325d06..a3a19ce8ce 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4653,9 +4653,7 @@ sequences Columns
   data_type
   character_data
   
-   The data type of the sequence.  In
-   PostgreSQL, this is currently always
-   bigint.
+   The data type of the sequence.
   
  
 
diff --git a/doc/src/sgml/ref/alter_sequence.sgml b/doc/src/sgml/ref/alter_sequence.sgml
index 3b52e875e3..252a668189 100644
--- a/doc/src/sgml/ref/alter_sequence.sgml
+++ b/doc/src/sgml/ref/alter_sequence.sgml
@@ -23,7 +23,9 @@
 
  
 
-ALTER SEQUENCE [ IF EXISTS ] name [ INCREMENT [ BY ] increment ]
+ALTER SEQUENCE [ IF EXISTS ] name
+[ AS data_type ]
+[ INCREMENT [ BY ] increment ]
 [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ]
 [ START [ WITH ] start ]
 [ RESTART [ [ WITH ] restart ] ]
@@ -81,6 +83,26 @@ Parameters
  
 
  
+  data_type
+  
+   
+The optional
+clause AS data_type
+changes the data type of the sequence.  Valid types are
+are smallint, integer,
+and bigint.
+   
+
+   
+Note that changing the data type does not automatically change the
+minimum and maximum values.  You can use the clauses NO
+MINVALUE and NO MAXVALUE to adjust the
+minimum and maximum values to the range of the new data type.
+   
+  
+ 
+
+ 
   increment
   

@@ -102,7 +124,7 @@ Parameters
 class="parameter">minvalue determines
 the minimum value a sequence can generate. If NO
 MINVALUE is specified, the defaults of 1 and
--263 for ascending and descending sequences,
+the minimum value of the data type for ascending and descending sequences,
 respectively, will be used.  If neither option is

Re: [HACKERS] sequence data type

2017-01-30 Thread Peter Eisentraut 
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


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

Re: [HACKERS] sequence data type

2017-01-29 Thread Michael Paquier 
On Sat, Jan 28, 2017 at 2:49 AM, Peter Eisentraut
 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))
>> +   {
>> +   charbufm[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


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

Re: [HACKERS] sequence data type

2017-01-27 Thread Peter Eisentraut 
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))
> +   {
> +   charbufm[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
From 533c4dcd04ffab9b8177307039f6c9ebcd6808b9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 27 Jan 2017 12:43:14 -0500
Subject: [PATCH] Additional test coverage for sequences

---
 src/test/regress/expected/sequence.out | 231 -
 src/test/regress/sql/sequence.sql  |  95 --
 2 files changed, 286 insertions(+), 40 deletions(-)

diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index ad03a31a4e..c19b5f5ef6 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -1,3 +1,33 @@
+--
+-- CREATE SEQUENCE
+--
+-- various error cases
+CREATE UNLOGGED SEQUENCE sequence_testx;
+ERROR:  unlogged sequences are not supported
+CREATE SEQUENCE sequence_testx INCREMENT BY 0;
+ERROR:  INCREMENT must not be zero
+CREATE SEQUENCE sequence_testx INCREMENT BY -1 MINVALUE 20;
+ERROR:  MINVALUE (20) must be less than MAXVALUE (-1)
+CREATE SEQUENCE sequence_testx INCREMENT BY 1 MAXVALUE -20;
+ERROR:  MINVALUE (1) must be less than MAXVALUE (-20)
+CREATE SEQUENCE sequence_testx INCREMENT BY -1 START 10;
+ERROR:  START value (10) cannot be greater than MAXVALUE (-1)
+CREATE SEQUENCE sequence_testx INCREMENT BY 1 START -10;
+ERROR:  START value (-10) cannot be less than MINVALUE (1)
+CREATE SEQUENCE sequence_testx CACHE 0;
+ERROR:  CACHE (0) must be greater than zero
+-- OWNED BY errors
+CREATE SEQUENCE sequence_testx OWNED BY nobody;  -- nonsense word
+ERROR:  invalid OWNED BY option
+HINT:  Specify OWNED BY table.column or OWNED BY NONE.
+CREATE SEQUENCE sequence_testx OWNED BY pg_tables.tablename;  -- not a table
+ERROR:  referenced relation "pg_tables" is not a table or foreign table
+CREATE SEQUENCE sequence_testx OWNED BY pg_class.relname;  -- not same schema
+ERROR:  sequence must be in same schema as table it is linked to
+CREATE TABLE sequence_test_table (a int);
+CREATE SEQUENCE sequence_testx OWNED BY sequence_test_table.b;  -- wrong column
+ERROR:  column "b" of relation "sequence_test_table" does not exist
+DROP TABLE sequence_test_table;
 ---
 --- test creation of SERIAL column
 ---
@@ -242,17 +272,38 @@ DROP SEQUENCE myseq2;
 -- Alter sequence
 --
 ALTER SEQUENCE IF EXISTS

Re: [HACKERS] sequence data type

2017-01-25 Thread Michael Paquier 
On Wed, Jan 25, 2017 at 11:53 PM, Peter Eisentraut
 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))
+   {
+   charbufm[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


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

Re: [HACKERS] sequence data type

2017-01-25 Thread Peter Eisentraut 
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
>From 5aff9dbe7a94417aa0faf56dab37187fcbec23f5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 25 Jan 2017 09:35:58 -0500
Subject: [PATCH v4] Add CREATE SEQUENCE AS  clause

This stores a data type, required to be an integer type, with the
sequence.  The sequences min and max values default to the range
supported by the type, and they cannot be set to values exceeding that
range.  The internal implementation of the sequence is not affected.

Change the serial types to create sequences of the appropriate type.
This makes sure that the min and max values of the sequence for a serial
column match the range of values supported by the table column.  So the
sequence can no longer overflow the table column.

This also makes monitoring for sequence exhaustion/wraparound easier,
which currently requires various contortions to cross-reference the
sequences with the table columns they are used with.

This commit also effectively reverts the pg_sequence column reordering
in f3b421da5f4addc95812b9db05a24972b8fd9739, because the new seqtypid
column allows us to fill the hole in the struct and create a more
natural overall column ordering.
---
 doc/src/sgml/catalogs.sgml  |  14 +++-
 doc/src/sgml/information_schema.sgml|   4 +-
 doc/src/sgml/ref/alter_sequence.sgml|  30 ++--
 doc/src/sgml/ref/create_sequence.sgml   |  36 ++
 src/backend/catalog/information_schema.sql  |   4 +-
 src/backend/commands/sequence.c |  89 +---
 src/backend/parser/gram.y   |   6 +-
 src/backend/parser/parse_utilcmd.c  |   2 +-
 src/bin/pg_dump/pg_dump.c   | 103 +++-
 src/bin/pg_dump/t/002_pg_dump.pl|   2 +
 src/include/catalog/catversion.h|   2 +-
 src/include/catalog/pg_proc.h   |   2 +-
 src/include/catalog/pg_sequence.h   |   8 ++-
 src/test/modules/test_pg_dump/t/001_base.pl |   1 +
 src/test/regress/expected/sequence.out  |  52 ++
 src/test/regress/expected/sequence_1.out|  52 ++
 src/test/regress/sql/sequence.sql   |  25 +--
 17 files changed, 312 insertions(+), 120 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 524180e011..162975746d 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -5774,10 +5774,11 @@ pg_sequence Columns
  
 
  
-  seqcycle
-  bool
+  seqtypid
+  oid
+  pg_type.oid
   
-  Whether the sequence cycles
+  Data type of the sequence
  
 
  
@@ -5814,6 +5815,13 @@ pg_sequence Columns
   
   Cache size of the sequence
  
+
+ 
+  seqcycle
+  bool
+  
+  Whether the sequence cycles
+ 
 

   
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325d06..a3a19ce8ce 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4653,9 +4653,7 @@ sequences Columns
   data_type
   character_data
   
-   The data type of the sequence.  In
-   PostgreSQL, this is currently always
-   bigint.
+   The data type of the sequence.
   
  
 
diff --git a/doc/src/sgml/ref/alter_sequence.sgml b/doc/src/sgml/ref/alter_sequence.sgml
index 3b52e875e3..252a668189 100644
--- a/doc/src/sgml/ref/alter_sequence.sgml
+++ b/doc/src/sgml/ref/alter_sequence.sgml
@@ -23,7 +23,9 @@
 
  
 
-ALTER SEQUENCE [ IF EXISTS ] name [ INCREMENT [ BY ] increment ]
+ALTER SEQUENCE [ IF EXISTS ] name
+[ AS data_type ]
+[ INCREMENT [ BY ] increment ]
 [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ]
 [ START [ WITH ] start ]
 [ RESTART [ [ WITH ] restart ] ]
@@ -81,6 +83,26 @@ Parameters
  
 
  
+  data_type
+  
+   
+The optional
+clause AS data_type
+changes the data type of the sequence.  Valid types are
+are smallint, integer,
+and bigint.
+   
+
+   
+Note that changing the data type does not automatically change the
+minimum and maximum values.  You can use the clauses NO
+MINVALUE and NO MAXVALUE to adjust the
+minimum and maximum values to the range of the new data type.
+   
+  
+ 
+
+ 
   increment
   

@@ -102,7 +124,7 @@ Parameters
 class="parameter">minvalue determines
 the minimum value a sequence can generate. If NO
 MINVALUE is specified, the defaults of 1 and
--263 for ascending and descending sequences,
+the minimum value of the data

Re: [HACKERS] sequence data type

2017-01-13 Thread Daniel Verite 
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


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

Re: [HACKERS] sequence data type

2017-01-13 Thread Daniel Verite 
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-3d78694c3...@manitou-mail.org


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


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

Re: [HACKERS] sequence data type

2017-01-10 Thread Andrew Gierth 
> "Daniel" == Daniel Verite  writes:

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

 Daniel> With the patch, it becomes a problem because

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

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

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

 Daniel> What should a user do to upgrade the SERIAL column?

Something along the lines of:

begin;
alter table tablename alter column id drop default;
alter sequence tablename_id_seq owned by none;
create sequence tablename_id_seq2 as bigint owned by tablename.id;
select setval('tablename_id_seq2', last_value, is_called) from tablename_id_seq;
drop sequence tablename_id_seq;
alter table tablename alter column id type bigint;
alter table tablename alter column id set default nextval('tablename_id_seq2');
commit;

Not impossible, but not at all obvious and quite involved. (And -1 for
this feature unless this issue is addressed.)

-- 
Andrew (irc:RhodiumToad)


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

Re: [HACKERS] sequence data type

2017-01-10 Thread Daniel Verite 
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


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

Re: [HACKERS] sequence data type

2017-01-10 Thread Michael Paquier 
On Tue, Jan 10, 2017 at 5:03 AM, Peter Eisentraut
 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


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

Re: [HACKERS] sequence data type

2017-01-09 Thread Peter Eisentraut 
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


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

Re: [HACKERS] sequence data type

2017-01-08 Thread Steve Singer 

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



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

Re: [HACKERS] sequence data type

2016-12-30 Thread Peter Eisentraut 
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
>From 62486c9092f21a1afc1bd9cfa50f570e9e3e92c1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 23 Dec 2016 12:00:00 -0500
Subject: [PATCH v3] Add CREATE SEQUENCE AS  clause

This stores a data type, required to be an integer type, with the
sequence.  The sequences min and max values default to the range
supported by the type, and they cannot be set to values exceeding that
range.  The internal implementation of the sequence is not affected.

Change the serial types to create sequences of the appropriate type.
This makes sure that the min and max values of the sequence for a serial
column match the range of values supported by the table column.  So the
sequence can no longer overflow the table column.

This also makes monitoring for sequence exhaustion/wraparound easier,
which currently requires various contortions to cross-reference the
sequences with the table columns they are used with.

This commit also effectively reverts the pg_sequence column reordering
in f3b421da5f4addc95812b9db05a24972b8fd9739, because the new seqtypid
column allows us to fill the hole in the struct and create a more
natural overall column ordering.
---
 doc/src/sgml/catalogs.sgml  |  14 +++-
 doc/src/sgml/information_schema.sgml|   4 +-
 doc/src/sgml/ref/create_sequence.sgml   |  37 ++
 src/backend/catalog/information_schema.sql  |   4 +-
 src/backend/commands/sequence.c |  95 ++---
 src/backend/parser/gram.y   |   6 +-
 src/backend/parser/parse_utilcmd.c  |   2 +-
 src/bin/pg_dump/pg_dump.c   | 105 +++-
 src/bin/pg_dump/t/002_pg_dump.pl|   2 +
 src/include/catalog/catversion.h|   2 +-
 src/include/catalog/pg_proc.h   |   2 +-
 src/include/catalog/pg_sequence.h   |   8 ++-
 src/include/pg_config_manual.h  |   6 --
 src/test/modules/test_pg_dump/t/001_base.pl |   1 +
 src/test/regress/expected/sequence.out  |  51 ++
 src/test/regress/expected/sequence_1.out|  51 ++
 src/test/regress/sql/sequence.sql   |  24 +--
 17 files changed, 291 insertions(+), 123 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 493050618d..765bc12c51 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -5628,10 +5628,11 @@ pg_sequence Columns
  
 
  
-  seqcycle
-  bool
+  seqtypid
+  oid
+  pg_type.oid
   
-  Whether the sequence cycles
+  Data type of the sequence
  
 
  
@@ -5668,6 +5669,13 @@ pg_sequence Columns
   
   Cache size of the sequence
  
+
+ 
+  seqcycle
+  bool
+  
+  Whether the sequence cycles
+ 
 

   
diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325d06..a3a19ce8ce 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4653,9 +4653,7 @@ sequences Columns
   data_type
   character_data
   
-   The data type of the sequence.  In
-   PostgreSQL, this is currently always
-   bigint.
+   The data type of the sequence.
   
  
 
diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index 62ae379226..f31b59569e 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -21,7 +21,9 @@
 
  
 
-CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name [ INCREMENT [ BY ] increment ]
+CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name
+[ AS data_type ]
+[ INCREMENT [ BY ] increment ]
 [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ]
 [ START [ WITH ] start ] [ CACHE cache ] [ [ NO ] CYCLE ]
 [ OWNED BY { table_name.column_name | NONE } ]
@@ -111,6 +113,21 @@ Parameters

 

+data_type
+
+ 
+  The optional
+  clause AS data_type
+  specifies the data type of the sequence.  Valid types are
+  are smallint, integer,
+  and bigint.  bigint is the
+  default.  The data type determines the default minimum and maximum
+  values of the sequence.
+ 
+
+   
+
+   
 increment
 
  
@@ -132,9 +149,9 @@ Parameters

Re: [HACKERS] sequence data type

2016-12-04 Thread Haribabu Kommi 
On Tue, Nov 22, 2016 at 10:54 PM, Haribabu Kommi 
wrote:

> Hi Vik and Vinayak,
>
> This is a gentle reminder.
>
> you both are assigned as reviewer's to the current patch in the 11-2016
> commitfest.
> But you haven't shared your review yet. Please share your review about
> the patch. This will help us in smoother operation of commitfest.
>
> Please Ignore if you already shared your review.
>
>
As the patch doesn't received a full review and it is not applying to HEAD
properly.
Moved to next CF with "waiting on author" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] sequence data type

2016-11-22 Thread Haribabu Kommi 
Hi Vik and Vinayak,

This is a gentle reminder.

you both are assigned as reviewer's to the current patch in the 11-2016
commitfest.
But you haven't shared your review yet. Please share your review about
the patch. This will help us in smoother operation of commitfest.

Please Ignore if you already shared your review.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] sequence data type

2016-10-02 Thread Michael Paquier 
On Sun, Sep 11, 2016 at 12:38 PM, Vitaly Burovoy
 wrote:
> On 9/10/16, Jim Nasby  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


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

Re: [HACKERS] sequence data type

2016-09-10 Thread Vitaly Burovoy 
On 9/10/16, Jim Nasby  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


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

Re: [HACKERS] sequence data type

2016-09-10 Thread Jim Nasby 

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


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

Re: [HACKERS] sequence data type

2016-09-08 Thread Peter Eisentraut 
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
>From 693bba6619d6e3283c6f7973f932044df2b4f695 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 8 Sep 2016 12:00:00 -0400
Subject: [PATCH v2] Add CREATE SEQUENCE AS  clause

This stores a data type, required to be an integer type, with the
sequence.  The sequences min and max values default to the range
supported by the type, and they cannot be set to values exceeding that
range.  The internal implementation of the sequence is not affected.

Change the serial types to create sequences of the appropriate type.
This makes sure that the min and max values of the sequence for a serial
column match the range of values supported by the table column.  So the
sequence can no longer overflow the table column.

This also makes monitoring for sequence exhaustion/wraparound easier,
which currently requires various contortions to cross-reference the
sequences with the table columns they are used with.
---
 doc/src/sgml/information_schema.sgml  |  4 +-
 doc/src/sgml/ref/create_sequence.sgml | 37 +++
 src/backend/catalog/information_schema.sql|  4 +-
 src/backend/commands/sequence.c   | 92 --
 src/backend/parser/gram.y |  6 +-
 src/backend/parser/parse_utilcmd.c|  2 +-
 src/bin/pg_dump/pg_dump.c | 94 +--
 src/bin/pg_dump/t/002_pg_dump.pl  |  2 +
 src/include/catalog/pg_proc.h |  2 +-
 src/include/commands/sequence.h   |  6 +-
 src/include/pg_config_manual.h|  6 --
 src/test/modules/test_pg_dump/t/001_base.pl   |  1 +
 src/test/regress/expected/sequence.out| 45 +
 src/test/regress/expected/sequence_1.out  | 45 +
 src/test/regress/expected/updatable_views.out |  3 +-
 src/test/regress/sql/sequence.sql | 20 +-
 16 files changed, 269 insertions(+), 100 deletions(-)

diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325..a3a19ce 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4653,9 +4653,7 @@ sequences Columns
   data_type
   character_data
   
-   The data type of the sequence.  In
-   PostgreSQL, this is currently always
-   bigint.
+   The data type of the sequence.
   
  
 
diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index 62ae379..f31b595 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -21,7 +21,9 @@
 
  
 
-CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name [ INCREMENT [ BY ] increment ]
+CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name
+[ AS data_type ]
+[ INCREMENT [ BY ] increment ]
 [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ]
 [ START [ WITH ] start ] [ CACHE cache ] [ [ NO ] CYCLE ]
 [ OWNED BY { table_name.column_name | NONE } ]
@@ -111,6 +113,21 @@ Parameters

 

+data_type
+
+ 
+  The optional
+  clause AS data_type
+  specifies the data type of the sequence.  Valid types are
+  are smallint, integer,
+  and bigint.  bigint is the
+  default.  The data type determines the default minimum and maximum
+  values of the sequence.
+ 
+
+   
+
+   
 increment
 
  
@@ -132,9 +149,9 @@ Parameters
   class="parameter">minvalue determines
   the minimum value a sequence can generate. If this clause is not
   supplied or NO MINVALUE is specified, then
-  defaults will be used.  The defaults are 1 and
-  -263-1 for ascending and descending sequences,
-  respectively.
+  defaults will be used.  The default for an ascending sequence is 1.  The
+  default for a descending sequence is the minimum value of the data type
+  plus 1.
  
 

@@ -148,9 +165,9 @@ Parameters
   class="parameter">maxvalue determines
   the maximum value for the sequence. If this clause is not
   supplied or NO MAXVALUE is specified, then
-  default values will be used.  The defaults are
-  263-1 and -1 for ascending and descending
-  sequences, respectively.
+  default values will be used.  The default for an ascending sequence is
+  the maximum value of the data type.  The default for a descending
+  sequence is -1.
  
 

@@ -349,12 +366,6 @@

Re: [HACKERS] sequence data type

2016-09-03 Thread Gavin Flower 

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



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

Re: [HACKERS] sequence data type

2016-09-03 Thread Vik Fearing 
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


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

[HACKERS] sequence data type

2016-08-30 Thread Peter Eisentraut 
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
From 97246197cfe3a69d14af1eb98f894946a2b8122d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 23 Aug 2016 12:00:00 -0400
Subject: [PATCH] Add CREATE SEQUENCE AS  clause

This stores a data type, required to be an integer type, with the
sequence.  The sequences min and max values default to the range
supported by the type, and they cannot be set to values exceeding that
range.  The internal implementation of the sequence is not affected.

Change the serial types to create sequences of the appropriate type.
This makes sure that the min and max values of the sequence for a serial
column match the range of values supported by the table column.  So the
sequence can no longer overflow the table column.

This also makes monitoring for sequence exhaustion/wraparound easier,
which currently requires various contortions to cross-reference the
sequences with the table columns they are used with.
---
 doc/src/sgml/information_schema.sgml  |  4 +-
 doc/src/sgml/ref/create_sequence.sgml | 37 +++
 src/backend/catalog/information_schema.sql|  4 +-
 src/backend/commands/sequence.c   | 92 --
 src/backend/parser/gram.y |  6 +-
 src/backend/parser/parse_utilcmd.c|  2 +-
 src/bin/pg_dump/pg_dump.c | 94 +--
 src/bin/pg_dump/t/002_pg_dump.pl  |  2 +
 src/include/catalog/pg_proc.h |  2 +-
 src/include/commands/sequence.h   |  6 +-
 src/include/pg_config_manual.h|  6 --
 src/test/modules/test_pg_dump/t/001_base.pl   |  1 +
 src/test/regress/expected/sequence.out| 45 +
 src/test/regress/expected/sequence_1.out  | 45 +
 src/test/regress/expected/updatable_views.out |  3 +-
 src/test/regress/sql/sequence.sql | 20 +-
 16 files changed, 269 insertions(+), 100 deletions(-)

diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
index c43e325..a3a19ce 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4653,9 +4653,7 @@ sequences Columns
   data_type
   character_data
   
-   The data type of the sequence.  In
-   PostgreSQL, this is currently always
-   bigint.
+   The data type of the sequence.
   
  
 
diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index c959146..f31b595 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -21,7 +21,9 @@
 
  
 
-CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name [ INCREMENT [ BY ] increment ]
+CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name
+[ AS data_type ]
+[ INCREMENT [ BY ] increment ]
 [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ]
 [ START [ WITH ] start ] [ CACHE cache ] [ [ NO ] CYCLE ]
 [ OWNED BY { table_name.column_name | NONE } ]
@@ -111,6 +113,21 @@ Parameters

 

+data_type
+
+ 
+  The optional
+  clause AS data_type
+  specifies the data type of the sequence.  Valid types are
+  are smallint, integer,
+  and bigint.  bigint is the
+  default.  The data type determines the default minimum and maximum
+  values of the sequence.
+ 
+
+   
+
+   
 increment
 
  
@@ -132,9 +149,9 @@ Parameters
   class="parameter">minvalue determines
   the minimum value a sequence can generate. If this clause is not
   supplied or NO MINVALUE is specified, then
-  defaults will be used.  The defaults are 1 and
-  -263-1 for ascending and descending sequences,
-  respectively.
+  defaults will be used.  The default for an ascending sequence is 1.  The
+  default for a descending sequence is the minimum value of the data type
+  plus 1.
  
 

@@ -148,9 +165,9 @@ Parameters
   class="parameter">maxvalue determines
   the