Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-03-01 Thread Peter Eisentraut 
On 1/12/17 09:36, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 1/11/17 11:25 PM, Tom Lane wrote:
>>> +1 for the concept, but I'm a bit worried about putting atooid() in
>>> postgres_ext.h.  That's going to impose on the namespace of libpq-using
>>> applications, for instance.  A more conservative answer would be to
>>> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
>>> so I do see the consistency of adding this there.  Hard choice.
> 
>> How about two copies: one in postgres_fe.h and one in postgres.h?
> 
> That seems uglier than either of the other choices.
> 
> I don't personally have a huge problem with adding atooid in
> postgres_ext.h, but I thought I'd better flag the potential issue
> to see if anyone else thinks it's a big problem.

committed as is then

-- 
Peter Eisentraut  http://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

Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-01-12 Thread Michael Paquier 
On Thu, Jan 12, 2017 at 11:36 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 1/11/17 11:25 PM, Tom Lane wrote:
>>> +1 for the concept, but I'm a bit worried about putting atooid() in
>>> postgres_ext.h.  That's going to impose on the namespace of libpq-using
>>> applications, for instance.  A more conservative answer would be to
>>> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
>>> so I do see the consistency of adding this there.  Hard choice.
>
>> How about two copies: one in postgres_fe.h and one in postgres.h?
>
> That seems uglier than either of the other choices.
>
> I don't personally have a huge problem with adding atooid in
> postgres_ext.h, but I thought I'd better flag the potential issue
> to see if anyone else thinks it's a big problem.

FWIW, postgres_ext.h would make the most sense to me. Now, there is
one way to not impose that to frontends linked to libpq which would be
to locate it in some new header in src/include/common/, where both
backend and frontend could reference to it.
-- 
Michael


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

Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-01-12 Thread Tom Lane 
Peter Eisentraut  writes:
> On 1/11/17 11:25 PM, Tom Lane wrote:
>> +1 for the concept, but I'm a bit worried about putting atooid() in
>> postgres_ext.h.  That's going to impose on the namespace of libpq-using
>> applications, for instance.  A more conservative answer would be to
>> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
>> so I do see the consistency of adding this there.  Hard choice.

> How about two copies: one in postgres_fe.h and one in postgres.h?

That seems uglier than either of the other choices.

I don't personally have a huge problem with adding atooid in
postgres_ext.h, but I thought I'd better flag the potential issue
to see if anyone else thinks it's a big problem.

>> The oid_cmp() move looks fine if we only need it on the server side.
>> But doesn't pg_dump have one too?

> The pg_dump one isn't a qsort comparator, though.

OK.

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

Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-01-12 Thread Peter Eisentraut 
On 1/12/17 12:25 AM, Kuntal Ghosh wrote:
> On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut
>  wrote:
>> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
>> equivalent, and pending patches are proposing to add more.  I propose
>> these two patches to collect them in central places.
>>
> I've verified that the patch covers all the copies of atooid() and
> oid_cmp() that should be replaced. However, as Tom suggested, I've
> looked into pg_dump and found that there is a oidcmp() as well. It may
> have been missed because it has a different name.
> 
> I was wondering whether it is worth to replace the following as well:
> (TransactionId)strtoul(str, NULL, 16) to something like #define atotranid().

There is a atoxid(), which is actually not used and removed by my patch.
 There are a few other calls of this pattern, but they are not frequent
or consistent enough to worry about (yet), I think.

-- 
Peter Eisentraut  http://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

Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-01-12 Thread Peter Eisentraut 
On 1/11/17 11:25 PM, Tom Lane wrote:
> Peter Eisentraut  writes:
>> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
>> equivalent, and pending patches are proposing to add more.  I propose
>> these two patches to collect them in central places.
> 
> +1 for the concept, but I'm a bit worried about putting atooid() in
> postgres_ext.h.  That's going to impose on the namespace of libpq-using
> applications, for instance.  A more conservative answer would be to
> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
> so I do see the consistency of adding this there.  Hard choice.

How about two copies: one in postgres_fe.h and one in postgres.h?

> The oid_cmp() move looks fine if we only need it on the server side.
> But doesn't pg_dump have one too?

The pg_dump one isn't a qsort comparator, though.

-- 
Peter Eisentraut  http://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

Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-01-11 Thread Kuntal Ghosh 
On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut
 wrote:
> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
> equivalent, and pending patches are proposing to add more.  I propose
> these two patches to collect them in central places.
>
I've verified that the patch covers all the copies of atooid() and
oid_cmp() that should be replaced. However, as Tom suggested, I've
looked into pg_dump and found that there is a oidcmp() as well. It may
have been missed because it has a different name.

I was wondering whether it is worth to replace the following as well:
(TransactionId)strtoul(str, NULL, 16) to something like #define atotranid().

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] many copies of atooid() and oid_cmp()

2017-01-11 Thread Tom Lane 
Peter Eisentraut  writes:
> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
> equivalent, and pending patches are proposing to add more.  I propose
> these two patches to collect them in central places.

+1 for the concept, but I'm a bit worried about putting atooid() in
postgres_ext.h.  That's going to impose on the namespace of libpq-using
applications, for instance.  A more conservative answer would be to
add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
so I do see the consistency of adding this there.  Hard choice.

The oid_cmp() move looks fine if we only need it on the server side.
But doesn't pg_dump have one too?

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

[HACKERS] many copies of atooid() and oid_cmp()

2017-01-11 Thread Peter Eisentraut 
There are approximately 11 copies of atooid() and 3 of oid_cmp() or
equivalent, and pending patches are proposing to add more.  I propose
these two patches to collect them in central places.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 6f30dc0b653b5b19f9d3ae29788fc922848b52e5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 11 Jan 2017 12:00:00 -0500
Subject: [PATCH 1/2] Move atooid() definition to a central place

---
 contrib/lo/lo.c   | 2 --
 contrib/vacuumlo/vacuumlo.c   | 2 --
 src/backend/bootstrap/bootparse.y | 2 --
 src/backend/libpq/hba.c   | 3 ---
 src/backend/nodes/readfuncs.c | 2 --
 src/backend/utils/adt/misc.c  | 2 --
 src/bin/pg_basebackup/pg_basebackup.c | 2 --
 src/bin/pg_upgrade/pg_upgrade.h   | 2 --
 src/bin/psql/common.h | 2 --
 src/bin/scripts/droplang.c| 2 --
 src/include/fe_utils/string_utils.h   | 2 --
 src/include/postgres_ext.h| 4 
 12 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/contrib/lo/lo.c b/contrib/lo/lo.c
index 953659305f..805514d97a 100644
--- a/contrib/lo/lo.c
+++ b/contrib/lo/lo.c
@@ -14,8 +14,6 @@
 
 PG_MODULE_MAGIC;
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 
 /*
  * This is the trigger that protects us from orphaned large objects
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index d9dee2f7a0..06f469067b 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -24,8 +24,6 @@
 #include "libpq-fe.h"
 #include "pg_getopt.h"
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 #define BUFSIZE			1024
 
 enum trivalue
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index d28af23c94..23bdb5c759 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -51,8 +51,6 @@
 #include "tcop/dest.h"
 #include "utils/rel.h"
 
-#define atooid(x)	((Oid) strtoul((x), NULL, 10))
-
 
 /*
  * Bison doesn't allocate anything that needs to live across parser calls,
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 07f046fd8b..91c5a49de5 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -47,9 +47,6 @@
 #endif
 
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-#define atoxid(x)  ((TransactionId) strtoul((x), NULL, 10))
-
 #define MAX_TOKEN	256
 #define MAX_LINE	8192
 
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index dc40d0181e..52b6cc9ffc 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -164,8 +164,6 @@
  */
 #define atoui(x)  ((unsigned int) strtoul((x), NULL, 10))
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 #define strtobool(x)  ((*(x) == 't') ? true : false)
 
 #define nullable_string(token,length)  \
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 66d09bcb0c..42221da128 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -44,8 +44,6 @@
 #include "utils/builtins.h"
 #include "utils/timestamp.h"
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 
 /*
  * Common subroutine for num_nulls() and num_nonnulls().
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8ebf24e771..f8f1133a1e 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -40,8 +40,6 @@
 #include "streamutil.h"
 
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 typedef struct TablespaceListCell
 {
 	struct TablespaceListCell *next;
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 42e7aebb01..524709a118 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -97,8 +97,6 @@ extern char *output_files[];
 #define CLUSTER_NAME(cluster)	((cluster) == _cluster ? "old" : \
  (cluster) == _cluster ? "new" : "none")
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 /* OID system catalog preservation added during PG 9.0 development */
 #define TABLE_SPACE_SUBDIRS_CAT_VER 20100
 /* postmaster/postgres -b (binary_upgrade) flag added during PG 9.1 development */
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index dad0eb822a..a83bc69ab7 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -13,8 +13,6 @@
 #include "libpq-fe.h"
 #include "fe_utils/print.h"
 
-#define atooid(x)  ((Oid) strtoul((x), NULL, 10))
-
 extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
 extern bool setQFout(const char *fname);
 
diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c
index ab0215d495..97d1de43ae 100644
--- a/src/bin/scripts/droplang.c
+++ b/src/bin/scripts/droplang.c
@@ -14,8 +14,6 @@
 #include "common.h"
 #include "fe_utils/print.h"
 
-#define atooid(x)  ((Oid)