Off-by-one oddity in minval for decreasing sequences

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

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

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