pgsql: Centralize definition of integer limits.

Started by Andres Freundalmost 11 years ago14 messages

andres@anarazel.de

almost 11 years ago

Centralize definition of integer limits.

Several submitted and even committed patches have run into the problem
that C89, our baseline, does not provide minimum/maximum values for
various integer datatypes. C99's stdint.h does, but we can't rely on
it.

Several parts of the code defined limits locally, so instead centralize
the definitions to c.h.

This patch also changes the more obvious usages of literal limit values;
there's more places that could be changed, but it's less clear whether
it's beneficial to change those.

Author: Andrew Gierth
Discussion: 87619tc5wc.fsf@news-spur.riddles.org.uk

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/83ff1618bc9d4e530d3ef2a668a71326784a753c

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

Michael Paquier

michael.paquier@gmail.com

almost 11 years ago

In reply to: Andres Freund (#1)

Re: pgsql: Centralize definition of integer limits.

On Thu, Mar 26, 2015 at 6:49 AM, Andres Freund <andres@anarazel.de> wrote:

Centralize definition of integer limits.

Several submitted and even committed patches have run into the problem
that C89, our baseline, does not provide minimum/maximum values for
various integer datatypes. C99's stdint.h does, but we can't rely on
it.

Several parts of the code defined limits locally, so instead centralize
the definitions to c.h.

This patch also changes the more obvious usages of literal limit values;
there's more places that could be changed, but it's less clear whether
it's beneficial to change those.

My OSX dev box is generating a couple of warnings since this commit:
pg_dump.c:14548:45: warning: format specifies type 'long' but the argument
has type 'long long' [-Wformat]
snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
../../../src/include/pg_config_manual.h:52:22: note: expanded from macro
'SEQ_MINVALUE'
#define SEQ_MINVALUE (-SEQ_MAXVALUE)
^
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
^
pg_dump.c:14549:45: warning: format specifies type 'long' but the argument
has type 'long long' [-Wformat]
snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../../../src/include/pg_config_manual.h:51:22: note: expanded from macro
'SEQ_MAXVALUE'
#define SEQ_MAXVALUE INT64_MAX
^~~~~~~~~
/usr/include/stdint.h:122:26: note: expanded from macro 'INT64_MAX'
#define INT64_MAX 9223372036854775807LL
^~~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)

Thoughts?
--
Michael

Michael Paquier

michael.paquier@gmail.com

almost 11 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: pgsql: Centralize definition of integer limits.

On Mon, Mar 30, 2015 at 2:01 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Thu, Mar 26, 2015 at 6:49 AM, Andres Freund <andres@anarazel.de> wrote:

Centralize definition of integer limits.

Several submitted and even committed patches have run into the problem
that C89, our baseline, does not provide minimum/maximum values for
various integer datatypes. C99's stdint.h does, but we can't rely on
it.

Several parts of the code defined limits locally, so instead centralize
the definitions to c.h.

This patch also changes the more obvious usages of literal limit values;
there's more places that could be changed, but it's less clear whether
it's beneficial to change those.

My OSX dev box is generating a couple of warnings since this commit:
pg_dump.c:14548:45: warning: format specifies type 'long' but the argument
has type 'long long' [-Wformat]
snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
../../../src/include/pg_config_manual.h:52:22: note: expanded from macro
'SEQ_MINVALUE'
#define SEQ_MINVALUE (-SEQ_MAXVALUE)
^
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
^
pg_dump.c:14549:45: warning: format specifies type 'long' but the argument
has type 'long long' [-Wformat]
snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../../../src/include/pg_config_manual.h:51:22: note: expanded from macro
'SEQ_MAXVALUE'
#define SEQ_MAXVALUE INT64_MAX
^~~~~~~~~
/usr/include/stdint.h:122:26: note: expanded from macro 'INT64_MAX'
#define INT64_MAX 9223372036854775807LL
^~~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)

Thoughts?

INT64_MODIFIER is "l" on OSX, causing this warning. Perhaps there is
something better to do instead of casting blindly to int64. Thoughts?
--
Michael

Attachments:

20150330_pgdump_fix_int64_cast.patchapplication/x-patch; name=20150330_pgdump_fix_int64_cast.patchDownload

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7da5c41..a15c875 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -14545,8 +14545,8 @@ dumpSequence(Archive *fout, DumpOptions *dopt, 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, (int64) SEQ_MINVALUE);
+	snprintf(bufx, sizeof(bufx), INT64_FORMAT, (int64) SEQ_MAXVALUE);
 
 	if (fout->remoteVersion >= 80400)
 	{

Andres Freund

andres@anarazel.de

almost 11 years ago

In reply to: Michael Paquier (#2)

Re: pgsql: Centralize definition of integer limits.

Hi,

On 2015-03-30 14:01:25 +0900, Michael Paquier wrote:

On Thu, Mar 26, 2015 at 6:49 AM, Andres Freund <andres@anarazel.de> wrote:

Centralize definition of integer limits.

Several submitted and even committed patches have run into the problem
that C89, our baseline, does not provide minimum/maximum values for
various integer datatypes. C99's stdint.h does, but we can't rely on
it.

Several parts of the code defined limits locally, so instead centralize
the definitions to c.h.

This patch also changes the more obvious usages of literal limit values;
there's more places that could be changed, but it's less clear whether
it's beneficial to change those.

My OSX dev box is generating a couple of warnings since this commit:
pg_dump.c:14548:45: warning: format specifies type 'long' but the argument
has type 'long long' [-Wformat]
snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
../../../src/include/pg_config_manual.h:52:22: note: expanded from macro
'SEQ_MINVALUE'
#define SEQ_MINVALUE (-SEQ_MAXVALUE)
^
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
^
pg_dump.c:14549:45: warning: format specifies type 'long' but the argument
has type 'long long' [-Wformat]
snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../../../src/include/pg_config_manual.h:51:22: note: expanded from macro
'SEQ_MAXVALUE'
#define SEQ_MAXVALUE INT64_MAX
^~~~~~~~~
/usr/include/stdint.h:122:26: note: expanded from macro 'INT64_MAX'
#define INT64_MAX 9223372036854775807LL
^~~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)

Hm. Can you send config.log and the generated pg_config.h?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

Andres Freund

andres@anarazel.de

almost 11 years ago

In reply to: Michael Paquier (#3)

Re: pgsql: Centralize definition of integer limits.

On 2015-03-30 14:27:20 +0900, Michael Paquier wrote:

INT64_MODIFIER is "l" on OSX, causing this warning. Perhaps there is
something better to do instead of casting blindly to int64. Thoughts?
--
Michael

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7da5c41..a15c875 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -14545,8 +14545,8 @@ dumpSequence(Archive *fout, DumpOptions *dopt, 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, (int64) SEQ_MINVALUE);
+	snprintf(bufx, sizeof(bufx), INT64_FORMAT, (int64) SEQ_MAXVALUE);

I think that's entirely the wrong approach. ISTM that, independent from
this patch, INT64CONST/HAVE_LL_CONSTANTS are wrong for your
environment. The HAVE_LL_CONSTANTS test is probably too simplistic.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

Michael Paquier

michael.paquier@gmail.com

almost 11 years ago

In reply to: Andres Freund (#4)

1 attachment(s)

Re: pgsql: Centralize definition of integer limits.

On Mon, Mar 30, 2015 at 7:48 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2015-03-30 14:01:25 +0900, Michael Paquier wrote:

On Thu, Mar 26, 2015 at 6:49 AM, Andres Freund <andres@anarazel.de>

wrote:

Centralize definition of integer limits.

Several submitted and even committed patches have run into the problem
that C89, our baseline, does not provide minimum/maximum values for
various integer datatypes. C99's stdint.h does, but we can't rely on
it.

Several parts of the code defined limits locally, so instead centralize
the definitions to c.h.

This patch also changes the more obvious usages of literal limit

values;

there's more places that could be changed, but it's less clear whether
it's beneficial to change those.

My OSX dev box is generating a couple of warnings since this commit:
pg_dump.c:14548:45: warning: format specifies type 'long' but the

argument

has type 'long long' [-Wformat]
snprintf(bufm, sizeof(bufm), INT64_FORMAT, SEQ_MINVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
../../../src/include/pg_config_manual.h:52:22: note: expanded from macro
'SEQ_MINVALUE'
#define SEQ_MINVALUE (-SEQ_MAXVALUE)
^
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)
^
pg_dump.c:14549:45: warning: format specifies type 'long' but the

argument

has type 'long long' [-Wformat]
snprintf(bufx, sizeof(bufx), INT64_FORMAT, SEQ_MAXVALUE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../../../src/include/pg_config_manual.h:51:22: note: expanded from macro
'SEQ_MAXVALUE'
#define SEQ_MAXVALUE INT64_MAX
^~~~~~~~~
/usr/include/stdint.h:122:26: note: expanded from macro 'INT64_MAX'
#define INT64_MAX 9223372036854775807LL
^~~~~~~~~~~~~~~~~~~~~
/usr/include/secure/_stdio.h:56:62: note: expanded from macro 'snprintf'
__builtin___snprintf_chk (str, len, 0, __darwin_obsz(str), __VA_ARGS__)

Hm. Can you send config.log and the generated pg_config.h?

Sure.
--
Michael

Tom Lane

tgl@sss.pgh.pa.us

almost 11 years ago

In reply to: Michael Paquier (#6)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

Michael Paquier <michael.paquier@gmail.com> writes:

On Mon, Mar 30, 2015 at 7:48 PM, Andres Freund <andres@anarazel.de> wrote:

On 2015-03-30 14:01:25 +0900, Michael Paquier wrote:

My OSX dev box is generating a couple of warnings since this commit:
pg_dump.c:14548:45: warning: format specifies type 'long' but the
argument has type 'long long' [-Wformat]

FWIW, I'm seeing the same thing on my OSX laptop. I think the explanation
is simple: both "long" and "long long" are 64 bits on this machine.
Our configure chooses the former to be "int64", but stdint.h is using
the latter. I'm not sure there's any good way to know what stdint.h
thinks is int64, but we can't do a half-baked integration job like this.
Either we revert 83ff1618b altogether, or we make int64/uint64 depend
on definitions from stdint.h rather than being chosen independently by
configure.

regards, tom lane

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

Andres Freund

andres@anarazel.de

almost 11 years ago

In reply to: Tom Lane (#7)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

On 2015-03-30 15:38:16 -0400, Tom Lane wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Mon, Mar 30, 2015 at 7:48 PM, Andres Freund <andres@anarazel.de> wrote:

On 2015-03-30 14:01:25 +0900, Michael Paquier wrote:

My OSX dev box is generating a couple of warnings since this commit:
pg_dump.c:14548:45: warning: format specifies type 'long' but the
argument has type 'long long' [-Wformat]

FWIW, I'm seeing the same thing on my OSX laptop. I think the explanation
is simple: both "long" and "long long" are 64 bits on this machine.
Our configure chooses the former to be "int64", but stdint.h is using
the latter. I'm not sure there's any good way to know what stdint.h
thinks is int64, but we can't do a half-baked integration job like this.
Either we revert 83ff1618b altogether, or we make int64/uint64 depend
on definitions from stdint.h rather than being chosen independently by
configure.

Obviously we need to fix this, but I'd actually say this isn't
originally the fault of that change. The disparity in types already
existed in a bunch of places (timestamp.c, pgbench.c), now it's more
central, that's all.

I'm too fried from the redeye back from pgconf nyc to do anything
complicated, but it seems quite possible to define int64/uint64 based
the stdint.h types if available. And generally a good idea too. I guess
I'll try that tomorrow; unless Andrew beats me to it.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

Andres Freund

andres@anarazel.de

almost 11 years ago

In reply to: Andres Freund (#8)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

Hi,

On 2015-03-30 21:50:09 +0200, Andres Freund wrote:

I'm too fried from the redeye back from pgconf nyc to do anything
complicated, but it seems quite possible to define int64/uint64 based
the stdint.h types if available. And generally a good idea too. I guess
I'll try that tomorrow; unless Andrew beats me to it.

It's possible to do that, but it's not as trivial as I'd hoped. For one
we'd need to include stdint.h in some places we don't today
(postgres_ext.h), for another we'd need some uglyness to determine the
correct printf modifier for int64_t (can't use PRId64 etc afaics).

I'm tempted to just prefix our limits with PG_ and define them
unconditionally, including appropriate casts to our types.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

#10

Tom Lane

tgl@sss.pgh.pa.us

almost 11 years ago

In reply to: Andres Freund (#9)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

Andres Freund <andres@anarazel.de> writes:

On 2015-03-30 21:50:09 +0200, Andres Freund wrote:

I'm too fried from the redeye back from pgconf nyc to do anything
complicated, but it seems quite possible to define int64/uint64 based
the stdint.h types if available. And generally a good idea too. I guess
I'll try that tomorrow; unless Andrew beats me to it.

It's possible to do that, but it's not as trivial as I'd hoped. For one
we'd need to include stdint.h in some places we don't today
(postgres_ext.h), for another we'd need some uglyness to determine the
correct printf modifier for int64_t (can't use PRId64 etc afaics).

Yeah, I thought the printf strings would be the sticking point :-(

I'm tempted to just prefix our limits with PG_ and define them
unconditionally, including appropriate casts to our types.

I don't have a better idea.

regards, tom lane

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

#11

Andres Freund

andres@anarazel.de

almost 11 years ago

In reply to: Tom Lane (#10)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

On 2015-03-31 12:10:48 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2015-03-30 21:50:09 +0200, Andres Freund wrote:

I'm too fried from the redeye back from pgconf nyc to do anything
complicated, but it seems quite possible to define int64/uint64 based
the stdint.h types if available. And generally a good idea too. I guess
I'll try that tomorrow; unless Andrew beats me to it.

It's possible to do that, but it's not as trivial as I'd hoped. For one
we'd need to include stdint.h in some places we don't today
(postgres_ext.h), for another we'd need some uglyness to determine the
correct printf modifier for int64_t (can't use PRId64 etc afaics).

Yeah, I thought the printf strings would be the sticking point :-(

I hacked things till it worked, but it seems fragile. Using Werror for
the format string test and adding 'l' to the tested combination works,
but... At the point where it basically worked the required changes
already amounted to ~150 lines changed (excluding configure). Making
that robust is more than I'm willing to do right now.

I do think it'd generally not be a bad thing to base our types on the
standard types if available.

I'm tempted to just prefix our limits with PG_ and define them
unconditionally, including appropriate casts to our types.

I don't have a better idea.

Will push that.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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

#12

Robert Haas

robertmhaas@gmail.com

almost 11 years ago

In reply to: Andres Freund (#11)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

On Tue, Mar 31, 2015 at 12:15 PM, Andres Freund <andres@anarazel.de> wrote:

I'm tempted to just prefix our limits with PG_ and define them
unconditionally, including appropriate casts to our types.

I don't have a better idea.

Will push that.

I'd appreciate it if you could do this soon. I like to compile with
-Werror, and this problem means I can't.

--
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

#13

Noname

andres@anarazel.de

almost 11 years ago

In reply to: Robert Haas (#12)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

On 2015-04-02 10:42:58 -0400, Robert Haas wrote:

On Tue, Mar 31, 2015 at 12:15 PM, Andres Freund <andres@anarazel.de> wrote:

I'm tempted to just prefix our limits with PG_ and define them
unconditionally, including appropriate casts to our types.

I don't have a better idea.

Will push that.

I'd appreciate it if you could do this soon. I like to compile with
-Werror, and this problem means I can't.

Done. Sorry for not doing this sooner, I'm more or less having holidays
right now.

Greetings,

Andres Freund

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

#14

Robert Haas

robertmhaas@gmail.com

almost 11 years ago

In reply to: Noname (#13)

Re: [COMMITTERS] pgsql: Centralize definition of integer limits.

On Thu, Apr 2, 2015 at 11:54 AM, Andres Freund <andres@anarazel.de> wrote:

On 2015-04-02 10:42:58 -0400, Robert Haas wrote:

On Tue, Mar 31, 2015 at 12:15 PM, Andres Freund <andres@anarazel.de> wrote:

I'm tempted to just prefix our limits with PG_ and define them
unconditionally, including appropriate casts to our types.

I don't have a better idea.

Will push that.

I'd appreciate it if you could do this soon. I like to compile with
-Werror, and this problem means I can't.

Done. Sorry for not doing this sooner, I'm more or less having holidays
right now.

Thanks. Sorry to interrupt your vacation.

--
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