Hi Paul
I'm starting to look at this again. Here's a few proposed changes to
the current code as I read along.
I noticed that 0001 does not compile on its own. It works as soon as I
add 0002. What this is telling me is that the current patch split is
not serving any goals; I think it's okay to merge them all into a single
commit. If you want to split it in smaller pieces, it needs to be stuff
that can be committed separately.
One possible candidate for that is the new makeUniqueTypeName function
you propose. I added this comment to explain what it does:
/*
- * makeUniqueTypeName: Prepend underscores as needed until we make a name that
- * doesn't collide with anything. Tries the original typeName if requested.
+ * makeUniqueTypeName
+ * Generate a unique name for a prospective new type
+ *
+ * Given a typeName of length namelen, produce a new name into dest (an output
+ * buffer allocated by caller, which must of length NAMEDATALEN) by prepending
+ * underscores, until a non-conflicting name results.
+ *
+ * If tryOriginalName, first try with zero underscores.
*
* Returns the number of underscores added.
*/
This seems a little too strange; why not have the function allocate its
output buffer instead, and return it? In order to support the case of
it failing to find an appropriate name, have it return NULL, for caller
to throw the "could not form ... name" error.
The attached 0001 simplifies makeMultirangeConstructors; I think it was
too baroque just because of it trying to imitate makeRangeConstructors;
it seems easier to just repeat the ProcedureCreate call than trying to
be clever. (makeRangeConstructors's comment is lying about the number
of constructors it creates after commit df73584431e7, BTW. But note
that the cruft in it to support doing it twice is not as much as in the
new code).
The other patches should be self-explanatory (I already submitted 0002
previously.)
I'll keep going over the rest of it. Thanks!
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 6a5b230f741fb4272770d09750b57fdad225027e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 28 Nov 2019 19:08:43 -0300
Subject: [PATCH 1/6] Simplify makeMultirangeConstructors
---
src/backend/commands/typecmds.c | 134 +++++++++++++++++++-------------
1 file changed, 80 insertions(+), 54 deletions(-)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 38948a049b..1b012c9cad 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1784,70 +1784,96 @@ static void
makeMultirangeConstructors(const char *name, Oid namespace,
Oid multirangeOid, Oid rangeArrayOid)
{
- static const char *const prosrc[2] = {"multirange_constructor0",
- "multirange_constructor1"};
- static const int pronargs[2] = {0, 1};
-
- Oid constructorArgTypes = rangeArrayOid;
ObjectAddress myself,
referenced;
- int i;
-
- Datum allParamTypes[1] = {ObjectIdGetDatum(rangeArrayOid)};
- ArrayType *allParameterTypes = construct_array(allParamTypes, 1, OIDOID,
- sizeof(Oid), true, 'i');
- Datum constructorAllParamTypes[2] = {PointerGetDatum(NULL), PointerGetDatum(allParameterTypes)};
-
- Datum paramModes[1] = {CharGetDatum(FUNC_PARAM_VARIADIC)};
- ArrayType *parameterModes = construct_array(paramModes, 1, CHAROID,
- 1, true, 'c');
- Datum constructorParamModes[2] = {PointerGetDatum(NULL), PointerGetDatum(parameterModes)};
+ oidvector *argtypes;
+ Datum allParamTypes;
+ ArrayType *allParameterTypes;
+ Datum paramModes;
+ ArrayType *parameterModes;
referenced.classId = TypeRelationId;
referenced.objectId = multirangeOid;
referenced.objectSubId = 0;
- for (i = 0; i < lengthof(prosrc); i++)
- {
- oidvector *constructorArgTypesVector;
+ argtypes = buildoidvector(NULL, 0);
+ myself = ProcedureCreate(name, /* name: same as multirange type */
+ namespace,
+ false, /* replace */
+ false, /* returns set */
+ multirangeOid, /* return type */
+ BOOTSTRAP_SUPERUSERID, /* proowner */
+ INTERNALlanguageId, /* language */
+ F_FMGR_INTERNAL_VALIDATOR,
+ "multirange_constructor0", /* prosrc */
+ NULL, /* probin */
+ PROKIND_FUNCTION,
+ false, /* security_definer */
+ false, /* leakproof */
+ false, /* isStrict */
+ PROVOLATILE_IMMUTABLE, /* volatility */
+ PROPARALLEL_SAFE, /* parallel safety */
+ argtypes, /* parameterTypes */
+ PointerGetDatum(NULL), /* allParameterTypes */
+ PointerGetDatum(NULL), /* parameterModes */
+ PointerGetDatum(NULL), /* parameterNames */
+ NIL, /* parameterDefaults */
+ PointerGetDatum(NULL), /* trftypes */
+ PointerGetDatum(NULL), /* proconfig */
+ InvalidOid, /* prosupport */
+ 1.0, /* procost */
+ 0.0); /* prorows */
- constructorArgTypesVector = buildoidvector(&constructorArgTypes,
- pronargs[i]);
+ /*
+ * Make the constructor internally-dependent on the multirange type so
+ * that they go away silently when the type is dropped. Note that pg_dump
+ * depends on this choice to avoid dumping the constructors.
+ */
+ recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
- myself = ProcedureCreate(name, /* name: same as multirange type */
- namespace, /* namespace */
- false, /* replace */
- false, /* returns set */
- multirangeOid, /* return type */
- BOOTSTRAP_SUPERUSERID, /* proowner */
- INTERNALlanguageId, /* language */
- F_FMGR_INTERNAL_VALIDATOR, /* language validator */
- prosrc[i], /* prosrc */
- NULL, /* probin */
- PROKIND_FUNCTION,
- false, /* security_definer */
- false, /* leakproof */
- false, /* isStrict */
- PROVOLATILE_IMMUTABLE, /* volatility */
- PROPARALLEL_SAFE, /* parallel safety */
- constructorArgTypesVector, /* parameterTypes */
- constructorAllParamTypes[i], /* allParameterTypes */
- constructorParamModes[i], /* parameterModes */
- PointerGetDatum(NULL), /* parameterNames */
- NIL, /* parameterDefaults */
- PointerGetDatum(NULL), /* trftypes */
- PointerGetDatum(NULL), /* proconfig */
- InvalidOid, /* prosupport */
- 1.0, /* procost */
- 0.0); /* prorows */
+ pfree(argtypes);
- /*
- * Make the constructors internally-dependent on the multirange type
- * so that they go away silently when the type is dropped. Note that
- * pg_dump depends on this choice to avoid dumping the constructors.
- */
- recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
- }
+ argtypes = buildoidvector(&rangeArrayOid, 1);
+ allParamTypes = ObjectIdGetDatum(rangeArrayOid);
+ allParameterTypes = construct_array(&allParamTypes,
+ 1, OIDOID,
+ sizeof(Oid), true, 'i');
+ paramModes = CharGetDatum(FUNC_PARAM_VARIADIC);
+ parameterModes = construct_array(¶mModes, 1, CHAROID,
+ 1, true, 'c');
+
+ myself = ProcedureCreate(name, /* name: same as multirange type */
+ namespace,
+ false, /* replace */
+ false, /* returns set */
+ multirangeOid, /* return type */
+ BOOTSTRAP_SUPERUSERID, /* proowner */
+ INTERNALlanguageId, /* language */
+ F_FMGR_INTERNAL_VALIDATOR,
+ "multirange_constructor1", /* prosrc */
+ NULL, /* probin */
+ PROKIND_FUNCTION,
+ false, /* security_definer */
+ false, /* leakproof */
+ false, /* isStrict */
+ PROVOLATILE_IMMUTABLE, /* volatility */
+ PROPARALLEL_SAFE, /* parallel safety */
+ argtypes, /* parameterTypes */
+ PointerGetDatum(allParameterTypes), /* allParameterTypes */
+ PointerGetDatum(parameterModes), /* parameterModes */
+ PointerGetDatum(NULL), /* parameterNames */
+ NIL, /* parameterDefaults */
+ PointerGetDatum(NULL), /* trftypes */
+ PointerGetDatum(NULL), /* proconfig */
+ InvalidOid, /* prosupport */
+ 1.0, /* procost */
+ 0.0); /* prorows */
+ /* ditto */
+ recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
+
+ pfree(argtypes);
+ pfree(allParameterTypes);
+ pfree(parameterModes);
}
/*
--
2.20.1
>From 589ef7f9a7f6debe767e7d3e0427941b349a3d13 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 28 Nov 2019 19:08:46 -0300
Subject: [PATCH 2/6] silence compiler warning
---
src/backend/parser/parse_coerce.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 49c75dc12d..b43b2dc612 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1592,6 +1592,8 @@ check_generic_type_consistency(const Oid *actual_arg_types,
return false;
}
}
+ else
+ range_typelem = InvalidOid; /* keep compiler quiet */
/* Get the element type based on the multirange type, if we have one */
if (OidIsValid(multirange_typeid))
--
2.20.1
>From b116b5c95f875361c01985b56c2f68d271a12e5e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 28 Nov 2019 19:30:03 -0300
Subject: [PATCH 3/6] Be less verbose on variable names
---
src/backend/commands/typecmds.c | 23 +++++++++-------------
src/backend/utils/adt/pg_upgrade_support.c | 4 ++--
src/include/catalog/binary_upgrade.h | 4 ++--
3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 1b012c9cad..4ca2d3364b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -85,8 +85,8 @@ typedef struct
/* Potentially set by pg_upgrade_support functions */
Oid binary_upgrade_next_array_pg_type_oid = InvalidOid;
-Oid binary_upgrade_next_multirange_pg_type_oid = InvalidOid;
-Oid binary_upgrade_next_multirange_array_pg_type_oid = InvalidOid;
+Oid binary_upgrade_next_mrng_pg_type_oid = InvalidOid;
+Oid binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid;
static void makeRangeConstructors(const char *name, Oid namespace,
Oid rangeOid, Oid subtype);
@@ -1529,13 +1529,9 @@ DefineRange(CreateRangeStmt *stmt)
/* alignment must be 'i' or 'd' for ranges */
alignment = (subtypalign == 'd') ? 'd' : 'i';
- /* Allocate OID for array type */
+ /* Allocate OID for array type, its multirange and its multirange array */
rangeArrayOid = AssignTypeArrayOid();
-
- /* Allocate OID for multirange type */
multirangeOid = AssignTypeMultirangeOid();
-
- /* Allocate OID for multirange array type */
multirangeArrayOid = AssignTypeMultirangeArrayOid();
/* Create the pg_type entry */
@@ -1574,7 +1570,6 @@ DefineRange(CreateRangeStmt *stmt)
Assert(typoid == address.objectId);
/* Create the multirange that goes with it */
-
multirangeTypeName = makeMultirangeTypeName(typeName, typeNamespace);
mltrngaddress =
@@ -2313,13 +2308,13 @@ AssignTypeMultirangeOid(void)
/* Use binary-upgrade override for pg_type.oid? */
if (IsBinaryUpgrade)
{
- if (!OidIsValid(binary_upgrade_next_multirange_pg_type_oid))
+ if (!OidIsValid(binary_upgrade_next_mrng_pg_type_oid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("pg_type multirange OID value not set when in binary upgrade mode")));
- type_multirange_oid = binary_upgrade_next_multirange_pg_type_oid;
- binary_upgrade_next_multirange_pg_type_oid = InvalidOid;
+ type_multirange_oid = binary_upgrade_next_mrng_pg_type_oid;
+ binary_upgrade_next_mrng_pg_type_oid = InvalidOid;
}
else
{
@@ -2346,13 +2341,13 @@ AssignTypeMultirangeArrayOid(void)
/* Use binary-upgrade override for pg_type.oid? */
if (IsBinaryUpgrade)
{
- if (!OidIsValid(binary_upgrade_next_multirange_array_pg_type_oid))
+ if (!OidIsValid(binary_upgrade_next_mrng_array_pg_type_oid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("pg_type multirange array OID value not set when in binary upgrade mode")));
- type_multirange_array_oid = binary_upgrade_next_multirange_array_pg_type_oid;
- binary_upgrade_next_multirange_array_pg_type_oid = InvalidOid;
+ type_multirange_array_oid = binary_upgrade_next_mrng_array_pg_type_oid;
+ binary_upgrade_next_mrng_array_pg_type_oid = InvalidOid;
}
else
{
diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c
index d980b96f48..418c26c81b 100644
--- a/src/backend/utils/adt/pg_upgrade_support.c
+++ b/src/backend/utils/adt/pg_upgrade_support.c
@@ -57,7 +57,7 @@ binary_upgrade_set_next_multirange_pg_type_oid(PG_FUNCTION_ARGS)
Oid typoid = PG_GETARG_OID(0);
CHECK_IS_BINARY_UPGRADE;
- binary_upgrade_next_multirange_pg_type_oid = typoid;
+ binary_upgrade_next_mrng_pg_type_oid = typoid;
PG_RETURN_VOID();
}
@@ -68,7 +68,7 @@ binary_upgrade_set_next_multirange_array_pg_type_oid(PG_FUNCTION_ARGS)
Oid typoid = PG_GETARG_OID(0);
CHECK_IS_BINARY_UPGRADE;
- binary_upgrade_next_multirange_array_pg_type_oid = typoid;
+ binary_upgrade_next_mrng_array_pg_type_oid = typoid;
PG_RETURN_VOID();
}
diff --git a/src/include/catalog/binary_upgrade.h b/src/include/catalog/binary_upgrade.h
index 2b6e87bb84..ba132ddf23 100644
--- a/src/include/catalog/binary_upgrade.h
+++ b/src/include/catalog/binary_upgrade.h
@@ -16,8 +16,8 @@
extern PGDLLIMPORT Oid binary_upgrade_next_pg_type_oid;
extern PGDLLIMPORT Oid binary_upgrade_next_array_pg_type_oid;
-extern PGDLLIMPORT Oid binary_upgrade_next_multirange_pg_type_oid;
-extern PGDLLIMPORT Oid binary_upgrade_next_multirange_array_pg_type_oid;
+extern PGDLLIMPORT Oid binary_upgrade_next_mrng_pg_type_oid;
+extern PGDLLIMPORT Oid binary_upgrade_next_mrng_array_pg_type_oid;
extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_type_oid;
extern PGDLLIMPORT Oid binary_upgrade_next_heap_pg_class_oid;
--
2.20.1
>From e78eb8bc8277bb09a98ae6b8c18252df834bce83 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 28 Nov 2019 19:36:53 -0300
Subject: [PATCH 4/6] Protect comment against pgindent
---
src/backend/utils/adt/multirangetypes.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index 7717acb3b1..b8ff39077c 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -1136,15 +1136,16 @@ multirange_intersect_multirange_internal(Oid mltrngtypoid, TypeCacheEntry *range
if (range_count1 == 0 || range_count2 == 0)
return make_multirange(mltrngtypoid, rangetyp, 0, NULL);
- /*
+ /*-----------------------------------------------
* Worst case is a stitching pattern like this:
*
* mr1: --- --- --- ---
* mr2: --- --- ---
* mr3: - - - - - -
*
- * That seems to be range_count1 + range_count2 - 1, but one extra won't
- * hurt.
+ * That seems to be range_count1 + range_count2 - 1,
+ * but one extra won't hurt.
+ *-----------------------------------------------
*/
ranges3 = palloc0((range_count1 + range_count2) * sizeof(RangeType *));
range_count3 = 0;
--
2.20.1
>From 981a6862601e22141938b9cfdea9149b1fdf65f3 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 28 Nov 2019 19:35:43 -0300
Subject: [PATCH 5/6] pgindent
---
src/backend/utils/adt/multirangetypes.c | 14 +++++++-------
src/backend/utils/fmgr/funcapi.c | 12 ++++++------
src/bin/pg_dump/pg_dump.c | 11 ++++++-----
3 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/src/backend/utils/adt/multirangetypes.c b/src/backend/utils/adt/multirangetypes.c
index b8ff39077c..f4dec1bbc9 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -330,13 +330,13 @@ multirange_recv(PG_FUNCTION_ARGS)
Datum
multirange_send(PG_FUNCTION_ARGS)
{
- MultirangeType *multirange = PG_GETARG_MULTIRANGE_P(0);
- Oid mltrngtypoid = MultirangeTypeGetOid(multirange);
- StringInfo buf = makeStringInfo();
- RangeType **ranges;
- int32 range_count;
- int32 i;
- MultirangeIOData *cache;
+ MultirangeType *multirange = PG_GETARG_MULTIRANGE_P(0);
+ Oid mltrngtypoid = MultirangeTypeGetOid(multirange);
+ StringInfo buf = makeStringInfo();
+ RangeType **ranges;
+ int32 range_count;
+ int32 i;
+ MultirangeIOData *cache;
cache = get_multirange_io_data(fcinfo, mltrngtypoid, IOFunc_send);
diff --git a/src/backend/utils/fmgr/funcapi.c b/src/backend/utils/fmgr/funcapi.c
index 25623461bb..808a0e2ca7 100644
--- a/src/backend/utils/fmgr/funcapi.c
+++ b/src/backend/utils/fmgr/funcapi.c
@@ -630,8 +630,8 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
{
if (OidIsValid(anymultirange_type))
{
- Oid rngtype;
- Oid subtype;
+ Oid rngtype;
+ Oid subtype;
rngtype = resolve_generic_type(ANYRANGEOID,
anymultirange_type,
@@ -665,9 +665,9 @@ resolve_polymorphic_tupdesc(TupleDesc tupdesc, oidvector *declared_args,
{
if (OidIsValid(anyrange_type))
{
- Oid subtype;
- Oid mltrngtype;
- Oid rngtype;
+ Oid subtype;
+ Oid mltrngtype;
+ Oid rngtype;
subtype = resolve_generic_type(ANYELEMENTOID,
anyrange_type,
@@ -911,7 +911,7 @@ resolve_polymorphic_argtypes(int numargs, Oid *argtypes, char *argmodes,
{
Oid rngtype;
Oid subtype;
-
+
rngtype = resolve_generic_type(ANYRANGEOID,
anymultirange_type,
ANYMULTIRANGEOID);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 4a0a03d50c..8272a0451b 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4276,15 +4276,16 @@ dumpSubscription(Archive *fout, SubscriptionInfo *subinfo)
free(qsubname);
}
-static Oid get_next_possible_free_pg_type_oid(Archive *fout, PQExpBuffer upgrade_query)
+static Oid
+get_next_possible_free_pg_type_oid(Archive *fout, PQExpBuffer upgrade_query)
{
/*
* If the old version didn't assign an array type, but the new version
- * does, we must select an unused type OID to assign. This currently
- * only happens for domains, when upgrading pre-v11 to v11 and up.
+ * does, we must select an unused type OID to assign. This currently only
+ * happens for domains, when upgrading pre-v11 to v11 and up.
*
- * Note: local state here is kind of ugly, but we must have some,
- * since we mustn't choose the same unused OID more than once.
+ * Note: local state here is kind of ugly, but we must have some, since we
+ * mustn't choose the same unused OID more than once.
*/
static Oid next_possible_free_oid = FirstNormalObjectId;
PGresult *res;
--
2.20.1
>From 04f1d482757debaa7ad44e8b59d76dab33d95503 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Fri, 29 Nov 2019 23:03:13 -0300
Subject: [PATCH 6/6] Add comment and asserts to makeUniqueTypeName
---
src/backend/catalog/pg_type.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index de0bb24649..7b871e7b0d 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -912,8 +912,14 @@ makeMultirangeTypeName(const char *rangeTypeName, Oid typeNamespace)
}
/*
- * makeUniqueTypeName: Prepend underscores as needed until we make a name that
- * doesn't collide with anything. Tries the original typeName if requested.
+ * makeUniqueTypeName
+ * Generate a unique name for a prospective new type
+ *
+ * Given a typeName of length namelen, produce a new name into dest (an output
+ * buffer allocated by caller, which must of length NAMEDATALEN) by prepending
+ * underscores, until a non-conflicting name results.
+ *
+ * If tryOriginalName, first try with zero underscores.
*
* Returns the number of underscores added.
*/
@@ -924,6 +930,9 @@ makeUniqueTypeName(char *dest, const char *typeName, size_t namelen, Oid typeNam
Relation pg_type_desc;
int i;
+ Assert(strlen(typeName) == namelen);
+ Assert(namelen < NAMEDATALEN);
+
pg_type_desc = table_open(TypeRelationId, AccessShareLock);
for (i = 0; i < NAMEDATALEN - 1; i++)
--
2.20.1
