Off-by-one oddity in minval for decreasing sequences
Hi,
When testing the patch at https://commitfest.postgresql.org/12/768/
("sequence data type" by Peter E.), I notice that there's a preexisting
oddity in the fact that sequences created with a negative increment
in current releases initialize the minval to -(2^63)+1 instead of -2^63,
the actual lowest value for a bigint.
postgres=# CREATE SEQUENCE s INCREMENT BY -1;
CREATE SEQUENCE
postgres=# SELECT seqmin,seqmin+pow(2::numeric,63)
FROM pg_sequence where seqrelid='s'::regclass;
seqmin | ?column?
----------------------+--------------------
-9223372036854775807 | 1.0000000000000000
But it's still possible to set it to -2^63 manually either by
altering the sequence or by specifying it explicitly at CREATE time
with CREATE SEQUENCE s MINVALUE -9223372036854775808
so it's inconsistent with the potential argument that we couldn't
store this value for some reason.
postgres=# ALTER SEQUENCE s minvalue -9223372036854775808;
ALTER SEQUENCE
postgres=# select seqmin,seqmin+pow(2::numeric,63)
from pg_sequence where seqrelid='s'::regclass;
seqmin | ?column?
----------------------+--------------------
-9223372036854775808 | 0.0000000000000000
The defaults comes from these definitions, in include/pg_config_manual.h
/*
* Set the upper and lower bounds of sequence values.
*/
#define SEQ_MAXVALUE PG_INT64_MAX
#define SEQ_MINVALUE (-SEQ_MAXVALUE)
with no comment as to why SEQ_MINVALUE is not PG_INT64_MIN.
When using other types than bigint, Peter's patch fixes the inconsistency
but also makes it worse by ISTM applying the rule that the lowest value
is forbidden for int2 and int4 in addition to int8.
I'd like to suggest that we don't do that starting with HEAD, by
setting seqmin to the real minimum of the supported range, because
wasting that particular value seems silly and a hazard if
someone wants to use a sequence to store any integer
as opposed to just calling nextval().
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
On Fri, Jan 6, 2017 at 2:15 PM, Daniel Verite <daniel@manitou-mail.org> wrote:
When testing the patch at https://commitfest.postgresql.org/12/768/
("sequence data type" by Peter E.), I notice that there's a preexisting
oddity in the fact that sequences created with a negative increment
in current releases initialize the minval to -(2^63)+1 instead of -2^63,
the actual lowest value for a bigint.postgres=# CREATE SEQUENCE s INCREMENT BY -1;
CREATE SEQUENCEpostgres=# SELECT seqmin,seqmin+pow(2::numeric,63)
FROM pg_sequence where seqrelid='s'::regclass;
seqmin | ?column?
----------------------+--------------------
-9223372036854775807 | 1.0000000000000000But it's still possible to set it to -2^63 manually either by
altering the sequence or by specifying it explicitly at CREATE time
with CREATE SEQUENCE s MINVALUE -9223372036854775808
so it's inconsistent with the potential argument that we couldn't
store this value for some reason.postgres=# ALTER SEQUENCE s minvalue -9223372036854775808;
ALTER SEQUENCE
postgres=# select seqmin,seqmin+pow(2::numeric,63)
from pg_sequence where seqrelid='s'::regclass;
seqmin | ?column?
----------------------+--------------------
-9223372036854775808 | 0.0000000000000000The defaults comes from these definitions, in include/pg_config_manual.h
/*
* Set the upper and lower bounds of sequence values.
*/
#define SEQ_MAXVALUE PG_INT64_MAX
#define SEQ_MINVALUE (-SEQ_MAXVALUE)with no comment as to why SEQ_MINVALUE is not PG_INT64_MIN.
When using other types than bigint, Peter's patch fixes the inconsistency
but also makes it worse by ISTM applying the rule that the lowest value
is forbidden for int2 and int4 in addition to int8.I'd like to suggest that we don't do that starting with HEAD, by
setting seqmin to the real minimum of the supported range, because
wasting that particular value seems silly and a hazard if
someone wants to use a sequence to store any integer
as opposed to just calling nextval().
This seems like a sensible argument to me, but maybe somebody's got a
contrary viewpoint?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/10/17 8:07 AM, Robert Haas wrote:
This seems like a sensible argument to me, but maybe somebody's got a
contrary viewpoint?
I suspect the number of users that use negative sequence values is so
small that this is unlikely to be noticed. I can't think of any risk to
"closing the hole" that you can end up with now. I agree it makes sense
to sen the minimum value correctly.
Not sure if this necessitates changes in pg_upgrade...
--
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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 01/12/2017 03:12 PM, Jim Nasby wrote:
On 1/10/17 8:07 AM, Robert Haas wrote:
This seems like a sensible argument to me, but maybe somebody's got a
contrary viewpoint?I suspect the number of users that use negative sequence values is so
small that this is unlikely to be noticed. I can't think of any risk
to "closing the hole" that you can end up with now. I agree it makes
sense to sen the minimum value correctly.Not sure if this necessitates changes in pg_upgrade...
FTR I used them extensively in $previous_job to get out of a nasty problem.
cheers
andrew
--
Andrew Dunstan https://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 7, 2017 at 4:15 AM, Daniel Verite <daniel@manitou-mail.org> wrote:
The defaults comes from these definitions, in include/pg_config_manual.h
/*
* Set the upper and lower bounds of sequence values.
*/
#define SEQ_MAXVALUE PG_INT64_MAX
#define SEQ_MINVALUE (-SEQ_MAXVALUE)with no comment as to why SEQ_MINVALUE is not PG_INT64_MIN.
Bruce likely does not remember 8000fdd4 from 2003. I stopped tracking there.
When using other types than bigint, Peter's patch fixes the inconsistency
but also makes it worse by ISTM applying the rule that the lowest value
is forbidden for int2 and int4 in addition to int8.
I think that mapping a sequence to a data type gives a good argument
to have the range mapping with the data type involved. That's less
surprise for the users, and less code to think about range boundaries.
Also, restoring from a dump sequences won't result in an error for
sequence definitions using PG_INT**_MIN as minimum value.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 1/6/17 2:15 PM, Daniel Verite wrote:
I notice that there's a preexisting
oddity in the fact that sequences created with a negative increment
in current releases initialize the minval to -(2^63)+1 instead of -2^63,
the actual lowest value for a bigint.
I think that had to do with that we had to play games to work around the
lack of proper int64 support, and various weird code has developed over
time because of that. I think we should fix it if we can.
The attached patch fixes the default minimum value to use the proper
int64 min value.
With this patch, when upgrading with pg_dump, descending sequences with
the previous default minimum value would be kept with that
now-not-default value. We could alternative adjust those sequences to
the new default value.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-default-minimum-value-for-descending-sequences.patchtext/x-patch; name=0001-Fix-default-minimum-value-for-descending-sequences.patchDownload
From 5c0223292c1ad065d73294c99fae3da60407f001 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 20 Jan 2017 19:58:56 -0500
Subject: [PATCH] Fix default minimum value for descending sequences
For some reason that is lost in history, a descending sequence would
default its minimum value to -2^63+1 (-PG_INT64_MAX) instead of
-2^63 (PG_INT64_MIN), even though explicitly specifying a minimum value
of -2^63 would work. Fix this inconsistency by using the full range by
default.
found by Daniel Verite <daniel@manitou-mail.org>
---
doc/src/sgml/ref/create_sequence.sgml | 2 +-
src/backend/commands/sequence.c | 4 ++--
src/bin/pg_dump/pg_dump.c | 4 ++--
src/include/pg_config_manual.h | 6 ------
4 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/ref/create_sequence.sgml b/doc/src/sgml/ref/create_sequence.sgml
index 62ae379226..86ff018c4b 100644
--- a/doc/src/sgml/ref/create_sequence.sgml
+++ b/doc/src/sgml/ref/create_sequence.sgml
@@ -133,7 +133,7 @@ <title>Parameters</title>
the minimum value a sequence can generate. If this clause is not
supplied or <option>NO MINVALUE</option> is specified, then
defaults will be used. The defaults are 1 and
- -2<superscript>63</>-1 for ascending and descending sequences,
+ -2<superscript>63</> for ascending and descending sequences,
respectively.
</para>
</listitem>
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 2de46c270e..bafa95ce8f 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1352,7 +1352,7 @@ init_params(ParseState *pstate, List *options, bool isInit,
else if (isInit || max_value != NULL)
{
if (seqform->seqincrement > 0)
- seqform->seqmax = SEQ_MAXVALUE; /* ascending seq */
+ seqform->seqmax = PG_INT64_MAX; /* ascending seq */
else
seqform->seqmax = -1; /* descending seq */
seqdataform->log_cnt = 0;
@@ -1369,7 +1369,7 @@ init_params(ParseState *pstate, List *options, bool isInit,
if (seqform->seqincrement > 0)
seqform->seqmin = 1; /* ascending seq */
else
- seqform->seqmin = SEQ_MINVALUE; /* descending seq */
+ seqform->seqmin = PG_INT64_MIN; /* descending seq */
seqdataform->log_cnt = 0;
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 0bb363957a..3a698afac5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15894,8 +15894,8 @@ dumpSequence(Archive *fout, TableInfo *tbinfo)
/* Make sure we are in proper schema */
selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
- snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
- snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);
+ snprintf(bufm, sizeof(bufm), INT64_FORMAT, PG_INT64_MIN);
+ snprintf(bufx, sizeof(bufx), INT64_FORMAT, PG_INT64_MAX);
if (fout->remoteVersion >= 100000)
{
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index c07907145a..f3b35297d1 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -51,12 +51,6 @@
#define PARTITION_MAX_KEYS 32
/*
- * Set the upper and lower bounds of sequence values.
- */
-#define SEQ_MAXVALUE PG_INT64_MAX
-#define SEQ_MINVALUE (-SEQ_MAXVALUE)
-
-/*
* When we don't have native spinlocks, we use semaphores to simulate them.
* Decreasing this value reduces consumption of OS resources; increasing it
* may improve performance, but supplying a real spinlock implementation is
--
2.11.0
On Sat, Jan 21, 2017 at 10:20 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 1/6/17 2:15 PM, Daniel Verite wrote:
I notice that there's a preexisting
oddity in the fact that sequences created with a negative increment
in current releases initialize the minval to -(2^63)+1 instead of -2^63,
the actual lowest value for a bigint.I think that had to do with that we had to play games to work around the
lack of proper int64 support, and various weird code has developed over
time because of that. I think we should fix it if we can.The attached patch fixes the default minimum value to use the proper
int64 min value.With this patch, when upgrading with pg_dump, descending sequences with
the previous default minimum value would be kept with that
now-not-default value. We could alternative adjust those sequences to
the new default value.
This patch looks acceptable to me.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers