many copies of atooid() and oid_cmp()
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
Attachments:
0001-Move-atooid-definition-to-a-central-place.patchtext/x-patch; name=0001-Move-atooid-definition-to-a-central-place.patchDownload
From 6f30dc0b653b5b19f9d3ae29788fc922848b52e5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
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) == &old_cluster ? "old" : \
(cluster) == &new_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 201001111
/* 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) strtoul((x), NULL, 10))
-
static void help(const char *progname);
diff --git a/src/include/fe_utils/string_utils.h b/src/include/fe_utils/string_utils.h
index 8f96a0b991..6fb7f5e30e 100644
--- a/src/include/fe_utils/string_utils.h
+++ b/src/include/fe_utils/string_utils.h
@@ -19,8 +19,6 @@
#include "libpq-fe.h"
#include "pqexpbuffer.h"
-#define atooid(x) ((Oid) strtoul((x), NULL, 10))
-
/* Global variables controlling behavior of fmtId() and fmtQualifiedId() */
extern int quote_all_identifiers;
extern PQExpBuffer (*getLocalPQExpBuffer) (void);
diff --git a/src/include/postgres_ext.h b/src/include/postgres_ext.h
index ae2f087798..452eae9935 100644
--- a/src/include/postgres_ext.h
+++ b/src/include/postgres_ext.h
@@ -39,6 +39,10 @@ typedef unsigned int Oid;
#define OID_MAX UINT_MAX
/* you will need to include <limits.h> to use the above #define */
+#define atooid(x) ((Oid) strtoul((x), NULL, 10))
+/* the above needs <stdlib.h> */
+
+
/* Define a signed 64-bit integer type for use in client API declarations. */
typedef PG_INT64_TYPE pg_int64;
--
2.11.0
0002-Collect-duplicate-copies-of-oid_cmp.patchtext/x-patch; name=0002-Collect-duplicate-copies-of-oid_cmp.patchDownload
From 74e07d330aae3c4b68c792f4d82c2a77fb4d80a8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 11 Jan 2017 12:00:00 -0500
Subject: [PATCH 2/2] Collect duplicate copies of oid_cmp()
---
src/backend/catalog/pg_enum.c | 15 ---------------
src/backend/catalog/pg_inherits.c | 18 +-----------------
src/backend/utils/adt/acl.c | 20 +-------------------
src/backend/utils/adt/oid.c | 14 ++++++++++++++
src/include/utils/builtins.h | 1 +
5 files changed, 17 insertions(+), 51 deletions(-)
diff --git a/src/backend/catalog/pg_enum.c b/src/backend/catalog/pg_enum.c
index 77cf608bb1..8175f6a0a7 100644
--- a/src/backend/catalog/pg_enum.c
+++ b/src/backend/catalog/pg_enum.c
@@ -35,7 +35,6 @@
Oid binary_upgrade_next_pg_enum_oid = InvalidOid;
static void RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems);
-static int oid_cmp(const void *p1, const void *p2);
static int sort_order_cmp(const void *p1, const void *p2);
@@ -609,20 +608,6 @@ RenumberEnumType(Relation pg_enum, HeapTuple *existing, int nelems)
}
-/* qsort comparison function for oids */
-static int
-oid_cmp(const void *p1, const void *p2)
-{
- Oid v1 = *((const Oid *) p1);
- Oid v2 = *((const Oid *) p2);
-
- if (v1 < v2)
- return -1;
- if (v1 > v2)
- return 1;
- return 0;
-}
-
/* qsort comparison function for tuples by sort order */
static int
sort_order_cmp(const void *p1, const void *p2)
diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index 82662a1f7e..9bd2cd16f6 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -27,12 +27,11 @@
#include "catalog/pg_inherits_fn.h"
#include "parser/parse_type.h"
#include "storage/lmgr.h"
+#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
-static int oid_cmp(const void *p1, const void *p2);
-
/*
* find_inheritance_children
@@ -357,18 +356,3 @@ typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId)
return result;
}
-
-
-/* qsort comparison function */
-static int
-oid_cmp(const void *p1, const void *p2)
-{
- Oid v1 = *((const Oid *) p1);
- Oid v2 = *((const Oid *) p2);
-
- if (v1 < v2)
- return -1;
- if (v1 > v2)
- return 1;
- return 0;
-}
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 56a69764c4..f9ba429107 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -87,7 +87,6 @@ static void check_circularity(const Acl *old_acl, const AclItem *mod_aip,
Oid ownerId);
static Acl *recursive_revoke(Acl *acl, Oid grantee, AclMode revoke_privs,
Oid ownerId, DropBehavior behavior);
-static int oidComparator(const void *arg1, const void *arg2);
static AclMode convert_priv_string(text *priv_type_text);
static AclMode convert_any_priv_string(text *priv_type_text,
@@ -1489,7 +1488,7 @@ aclmembers(const Acl *acl, Oid **roleids)
}
/* Sort the array */
- qsort(list, j, sizeof(Oid), oidComparator);
+ qsort(list, j, sizeof(Oid), oid_cmp);
/* Remove duplicates from the array */
k = 0;
@@ -1508,23 +1507,6 @@ aclmembers(const Acl *acl, Oid **roleids)
return k + 1;
}
-/*
- * oidComparator
- * qsort comparison function for Oids
- */
-static int
-oidComparator(const void *arg1, const void *arg2)
-{
- Oid oid1 = *(const Oid *) arg1;
- Oid oid2 = *(const Oid *) arg2;
-
- if (oid1 > oid2)
- return 1;
- if (oid1 < oid2)
- return -1;
- return 0;
-}
-
/*
* aclinsert (exported function)
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index fd123827dd..6ec6edd9fb 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -328,6 +328,20 @@ oidparse(Node *node)
return InvalidOid; /* keep compiler quiet */
}
+/* qsort comparison function for oids */
+int
+oid_cmp(const void *p1, const void *p2)
+{
+ Oid v1 = *((const Oid *) p1);
+ Oid v2 = *((const Oid *) p2);
+
+ if (v1 < v2)
+ return -1;
+ if (v1 > v2)
+ return 1;
+ return 0;
+}
+
/*****************************************************************************
* PUBLIC ROUTINES *
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index e1bb344e4f..1541d6fb74 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -548,6 +548,7 @@ extern Datum oidvectorge(PG_FUNCTION_ARGS);
extern Datum oidvectorgt(PG_FUNCTION_ARGS);
extern oidvector *buildoidvector(const Oid *oids, int n);
extern Oid oidparse(Node *node);
+extern int oid_cmp(const void *p1, const void *p2);
/* orderedsetaggs.c */
extern Datum ordered_set_transition(PG_FUNCTION_ARGS);
--
2.11.0
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
On 1/11/17 11:25 PM, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On 1/12/17 12:25 AM, Kuntal Ghosh wrote:
On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On Thu, Jan 12, 2017 at 11:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On 1/12/17 09:36, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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