Thread: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column
BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column
From
PG Bug reporting form
Date:
The following bug has been logged on the website: Bug reference: 15096 Logged by: Evgeny Email address: evgeni-klimov@yandex.ru PostgreSQL version: 10.2 Operating system: Windows 7 x64 Description: CREATE TABLE public.test (id BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, s TEXT); CREATE TABLE public.test_copy (LIKE public.test INCLUDING ALL); ERROR: MINVALUE (1) must be less than MAXVALUE (-1) However, if I change the id column type to INT, the second command succeeds. Checked on 10.3 and 10.2.
=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> CREATE TABLE public.test (id BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY
> KEY, s TEXT);
> CREATE TABLE public.test_copy (LIKE public.test INCLUDING ALL);
> ERROR: MINVALUE (1) must be less than MAXVALUE (-1)
Hm ... works fine for me on a 64-bit Linux machine ... but it fails
as described on 32-bit. Something in the LIKE code path is shoving
the sequence's maxvalue through a variable with platform-dependent
width, probably of type "long". No time right now to locate the
exact problem.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> ERROR: MINVALUE (1) must be less than MAXVALUE (-1) Tom> Hm ... works fine for me on a 64-bit Linux machine ... but it Tom> fails as described on 32-bit. Something in the LIKE code path is Tom> shoving the sequence's maxvalue through a variable with Tom> platform-dependent width, probably of type "long". No time right Tom> now to locate the exact problem. The Value node's "ival" field is declared as a long. sequence_options thinks it can put all the sequence parameters through makeInteger and have them come out intact. -- Andrew (irc:RhodiumToad)
On Sat, Mar 03, 2018 at 07:55:41PM +0000, Andrew Gierth wrote: > sequence_options thinks it can put all the sequence parameters through > makeInteger and have them come out intact. Yeah, on 32b machines "long" is 4 bytes... Perhaps it would be the occasion to introduce a T_Integer64 type for Value which gets stored as a string? And as far as I can see defGetInt64 is only used by sequences. -- Michael
Attachment
>>>>> "Michael" == Michael Paquier <michael@paquier.xyz> writes: > On Sat, Mar 03, 2018 at 07:55:41PM +0000, Andrew Gierth wrote: >> sequence_options thinks it can put all the sequence parameters >> through makeInteger and have them come out intact. Michael> Yeah, on 32b machines "long" is 4 bytes... And on 64-bit Windows, too. Michael> Perhaps it would be the occasion to introduce a T_Integer64 Michael> type for Value which gets stored as a string? And as far as I Michael> can see defGetInt64 is only used by sequences. The slightly misnamed T_Float is what's currently used for Value nodes which contain numeric values as strings. So there'd be no point in a new type tag if you're still going to store the value as a string. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Michael" == Michael Paquier <michael@paquier.xyz> writes:
> Michael> Perhaps it would be the occasion to introduce a T_Integer64
> Michael> type for Value which gets stored as a string? And as far as I
> Michael> can see defGetInt64 is only used by sequences.
> The slightly misnamed T_Float is what's currently used for Value nodes
> which contain numeric values as strings. So there'd be no point in a new
> type tag if you're still going to store the value as a string.
Going forward, maybe we should change the T_Integer case to either int64
or int32, so that it's not got a platform-dependent range. That's not a
workable solution for back-patching into v10, though (and neither is
T_Integer64, really).
regards, tom lane
On Mon, Mar 05, 2018 at 12:44:32AM -0500, Tom Lane wrote: > Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >> "Michael" == Michael Paquier <michael@paquier.xyz> writes: >> Michael> Perhaps it would be the occasion to introduce a T_Integer64 >> Michael> type for Value which gets stored as a string? And as far as I >> Michael> can see defGetInt64 is only used by sequences. > >> The slightly misnamed T_Float is what's currently used for Value nodes >> which contain numeric values as strings. So there'd be no point in a new >> type tag if you're still going to store the value as a string. > > Going forward, maybe we should change the T_Integer case to either int64 > or int32, so that it's not got a platform-dependent range. Serial columns using bigint as type would need int64 anyway, no? Why int32? > That's not a workable solution for back-patching into v10, though (and > neither is T_Integer64, really). Sure. For v10, using just T_Float should be doable at quick glance. I have not checked though. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Mar 05, 2018 at 12:44:32AM -0500, Tom Lane wrote:
>> Going forward, maybe we should change the T_Integer case to either int64
>> or int32, so that it's not got a platform-dependent range.
> Serial columns using bigint as type would need int64 anyway, no? Why
> int32?
int32 might be less work to make fully portable. I don't recall exactly
what-all we do with T_Integer, but if we try to do sscanf() to produce the
value for instance, that's a problem. (Note that configure's tests for
64-bit support only cover sprintf, not sscanf.)
Certainly int64 would be a more forward-looking choice, it just seems
like possibly more work.
regards, tom lane
Here is a patch that fixes this bug, and a second patch (not for backpatching) that changes the Value node to use int instead of long. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Wed, Mar 07, 2018 at 03:16:44PM -0500, Peter Eisentraut wrote:
> Here is a patch that fixes this bug, and a second patch (not for
> backpatching) that changes the Value node to use int instead of long.
Thanks Peter for diving in. Patch 1 looks fine to me.
Here are some comments for patch 2.
+ if (endptr != token + length || errno == ERANGE ||
+ /* check for overflow of int4 */
+ val != (long) ((int32) val))
return T_Float;
It would be nice to have this check be consistent with the new
definition of ival and int32, One suggestion is to use directly int32 or
just have a static assertion that sizeof(int) == sizeof(int32)? Or
that's too much nannyism?
By the way, why do you remove HAVE_LONG_INT_64? On platforms where long
is 4 bytes this would still be a no-op. If you care about those, you
could also remove the ones in interval.c and datetime.c...
The comment block on top of the definition of Value in value.h still
mentions "long", while it should mention "int" per your patch.
--
Michael
Attachment
On 3/8/18 03:08, Michael Paquier wrote: > Here are some comments for patch 2. > > + if (endptr != token + length || errno == ERANGE || > + /* check for overflow of int4 */ > + val != (long) ((int32) val)) > return T_Float; > It would be nice to have this check be consistent with the new > definition of ival and int32, One suggestion is to use directly int32 or > just have a static assertion that sizeof(int) == sizeof(int32)? Or > that's too much nannyism? Fixed in the attached next version. > By the way, why do you remove HAVE_LONG_INT_64? On platforms where long > is 4 bytes this would still be a no-op. Right, but the compiler can optimize it away then. No need to have #ifdefs. > The comment block on top of the definition of Value in value.h still > mentions "long", while it should mention "int" per your patch. done > If you care about those, you > could also remove the ones in interval.c and datetime.c... Actually, we could just use the strtoint() defined there and apply it everywhere, so avoid repeating these patterns. Done so in an additional patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Mon, Mar 12, 2018 at 01:45:26PM -0400, Peter Eisentraut wrote: > On 3/8/18 03:08, Michael Paquier wrote: >> Here are some comments for patch 2. >> >> + if (endptr != token + length || errno == ERANGE || >> + /* check for overflow of int4 */ >> + val != (long) ((int32) val)) >> return T_Float; >> It would be nice to have this check be consistent with the new >> definition of ival and int32, One suggestion is to use directly int32 or >> just have a static assertion that sizeof(int) == sizeof(int32)? Or >> that's too much nannyism? > > Fixed in the attached next version. Thanks, this looks good to me. >> If you care about those, you >> could also remove the ones in interval.c and datetime.c... > > Actually, we could just use the strtoint() defined there and apply it > everywhere, so avoid repeating these patterns. Done so in an additional > patch. OK, that's a good idea. +/* + * strtoint --- just like strtol, but returns int not long + */ It would be nice to document that caller should check for errno for sanity checks, in which case caller should not rely on the returned result. Worth noting that pgbench has its own view of things with strtoint64. Not worth bothering anyway. -- Michael
Attachment
On 3/13/18 02:22, Michael Paquier wrote: > Thanks, this looks good to me. committed (0001 to PG10, 0001-0003 to master) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services