BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

PG Bug reporting form
The following bug has been logged on the website:

Bug reference:      15096
Logged by:          Evgeny
Email address:      [hidden email]
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.

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Tom Lane-2
=?utf-8?q?PG_Bug_reporting_form?= <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Andrew Gierth
>>>>> "Tom" == Tom Lane <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Andrew Gierth
>>>>> "Michael" == Michael Paquier <[hidden email]> 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)

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Tom Lane-2
Andrew Gierth <[hidden email]> writes:
> "Michael" == Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Michael Paquier-2
On Mon, Mar 05, 2018 at 12:44:32AM -0500, Tom Lane wrote:

> Andrew Gierth <[hidden email]> writes:
>> "Michael" == Michael Paquier <[hidden email]> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Tom Lane-2
Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Peter Eisentraut-6
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

0001-Fix-CREATE-TABLE-LIKE-with-bigint-identity-column.patch (8K) Download Attachment
0002-Change-internal-integer-representation-of-Value-node.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Peter Eisentraut-6
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

v2-0001-Fix-CREATE-TABLE-LIKE-with-bigint-identity-column.patch (6K) Download Attachment
v2-0002-Change-internal-integer-representation-of-Value-n.patch (5K) Download Attachment
v2-0003-Move-strtoint-to-common.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: BUG #15096: Unable to CREATE TABLE LIKE with bigint identity column

Peter Eisentraut-6
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

Previous Thread Next Thread