Re: Postgres do not allow to create many tables with more than 63-symbols prefix

[Tom Lane]

Andrey Lepikhov  writes:
> On 6/27/22 06:38, Masahiko Sawada wrote:
 Regarding the patch, I think we can merge makeUniqueTypeName() to
 makeArrayTypeName() as there is no caller of makeUniqueTypeName() who
 pass tryOriginal = true.

>>> I partially agree with you. But I have one reason to leave
>>> makeUniqueTypeName() separated:
>>> It may be used in other codes with auto generated types. For example, I
>>> think, the DefineRelation routine should choose composite type instead
>>> of using the same name as the table.

No, this is an absolutely horrid idea.  The rule that "_foo" means "array
of foo" is quite well known to a lot of client code, and as long as they
don't use any type names approaching NAMEDATALEN, it's solid.  So we must
not build backend code that uses "_foo"-like type names for any other
purpose than arrays.

I suspect in fact that the reason we ended up with this orphaned logic
is that somebody pointed out this problem somewhere along the development
of multiranges, whereupon makeMultirangeTypeName was changed to not
use the shared code --- but the breakup of makeArrayTypeName wasn't
undone altogether.  It should have been, because it just tempts other
people to make the same wrong choice.

Pushed with re-merging of the code into makeArrayTypeName and some
work on the comments.

regards, tom lane

Re: Postgres do not allow to create many tables with more than 63-symbols prefix

[Andrey Lepikhov]

On 6/27/22 06:38, Masahiko Sawada wrote:

On Fri, Jun 24, 2022 at 2:12 PM Andrey Lepikhov
 wrote:

On 6/23/22 07:03, Masahiko Sawada wrote:
  > On Sat, Jun 4, 2022 at 4:03 AM Andrey Lepikhov
  >  wrote:
  >> It is very corner case, of course. But solution is easy and short. So,
  >> why not to fix? - See the patch in attachment.
  >
  > While this seems to be a good improvement, I think it's not a bug.
  > Probably we cannot backpatch it as it will end up having type names
  > defined by different naming rules. I'd suggest discussing it on
  > -hackers.
Done.


Thank for updating the patch. Please register this item to the next CF
if not yet.

Done [1].


  > Regarding the patch, I think we can merge makeUniqueTypeName() to
  > makeArrayTypeName() as there is no caller of makeUniqueTypeName() who
  > pass tryOriginal = true.
I partially agree with you. But I have one reason to leave
makeUniqueTypeName() separated:
It may be used in other codes with auto generated types. For example, I
think, the DefineRelation routine should choose composite type instead
of using the same name as the table.


Okay.

I have one comment on v2 patch:

  +   for(;;)
  {
  -   dest[i - 1] = '_';
  -   strlcpy(dest + i, typeName, NAMEDATALEN - i);
  -   if (namelen + i >= NAMEDATALEN)
  -   truncate_identifier(dest, NAMEDATALEN, false);
  -
  if (!SearchSysCacheExists2(TYPENAMENSP,
  -  CStringGetDatum(dest),
  +  CStringGetDatum(type_name),
 ObjectIdGetDatum(typeNamespace)))
  -   return pstrdup(dest);
  +   return type_name;
  +
  +   /* Previous attempt was failed. Prepare a new one. */
  +   pfree(type_name);
  +   snprintf(suffix, sizeof(suffix), "%d", ++pass);
  +   type_name = makeObjectName("", typeName, suffix);
  }

  return NULL;

I think it's better to break from the loop instead of returning from
there. That way, we won't need "return NULL".

Agree. Done.

[1] https://commitfest.postgresql.org/38/3712/

--
Regards
Andrey Lepikhov
Postgres ProfessionalFrom 73b80676fff98e9edadab2576cf22778c6b5168b Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" 
Date: Fri, 3 Jun 2022 21:40:01 +0300
Subject: [PATCH] Allow postgresql to generate more relations with the same 63
 symbols long prefix. Rewrite the makeUniqueTypeName routine - generator of
 unique name is based on the same idea as the ChooseRelationName() routine.

Authors: Dmitry Koval, Andrey Lepikhov
---
 src/backend/catalog/pg_type.c | 37 ---
 src/test/regress/expected/alter_table.out |  6 ++--
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index 03788cb975..405553013e 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -26,6 +26,7 @@
 #include "catalog/pg_namespace.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
+#include "commands/defrem.h"
 #include "commands/typecmds.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -936,17 +937,17 @@ makeMultirangeTypeName(const char *rangeTypeName, Oid typeNamespace)
  * makeUniqueTypeName
  *		Generate a unique name for a prospective new type
  *
- * Given a typeName, return a new palloc'ed name by prepending underscores
- * until a non-conflicting name results.
+ * Given a typeName, return a new palloc'ed name by prepending underscore
+ * and (if needed) adding a suffix to the end of the type name.
  *
  * If tryOriginal, first try with zero underscores.
  */
 static char *
 makeUniqueTypeName(const char *typeName, Oid typeNamespace, bool tryOriginal)
 {
-	int			i;
-	int			namelen;
-	char		dest[NAMEDATALEN];
+	int		pass = 0;
+	char	suffix[NAMEDATALEN];
+	char   *type_name;
 
 	Assert(strlen(typeName) <= NAMEDATALEN - 1);
 
@@ -956,23 +957,25 @@ makeUniqueTypeName(const char *typeName, Oid typeNamespace, bool tryOriginal)
 			   ObjectIdGetDatum(typeNamespace)))
 		return pstrdup(typeName);
 
+	/* Prepare initial object name. Just for compatibility. */
+	type_name = makeObjectName("", typeName, NULL);
+
 	/*
-	 * The idea is to prepend underscores as needed until we make a name that
-	 * doesn't collide with anything ...
+	 * The idea of unique name generation is similar to ChooseRelationName()
+	 * implementation logic.
 	 */
-	namelen = strlen(typeName);
-	for (i = 1; i < NAMEDATALEN - 1; i++)
+	for(;;)
 	{
-		dest[i - 1] = '_';
-		strlcpy(dest + i, typeName, NAMEDATALEN - i);
-		if (namelen + i >= NAMEDATALEN)
-			truncate_identifier(dest, NAMEDATALEN, false);
-
 		if (!SearchSysCacheExists2(TYPENAMENSP,
-   CStringGetDatum(dest),
+   CStringGetDatum(type_name),
    ObjectIdGetDatum(typeNamespace)))
-			return pstrdup(dest);
+			break;
+
+		/* Previous attempt was failed. Prepare a new one. */
+		pfree(type_name);
+		snprintf(suffix, sizeof(suffix), "%d", ++pass);

Re: Postgres do not allow to create many tables with more than 63-symbols prefix

[Masahiko Sawada]

On Fri, Jun 24, 2022 at 2:12 PM Andrey Lepikhov
 wrote:
>
> Moved from the pgsql-bugs mailing list [1].
>
> On 6/23/22 07:03, Masahiko Sawada wrote:
>  > Hi,
>  >
>  > On Sat, Jun 4, 2022 at 4:03 AM Andrey Lepikhov
>  >  wrote:
>  >>
>  >> According to subj you can try to create many tables (induced by the case
>  >> of partitioned table) with long prefix - see 6727v.sql for reproduction.
>  >> But now it's impossible because of logic of the makeUniqueTypeName()
>  >> routine.
>  >> You get the error:
>  >> ERROR:  could not form array type name for type ...
>  >>
>  >> It is very corner case, of course. But solution is easy and short. So,
>  >> why not to fix? - See the patch in attachment.
>  >
>  > While this seems to be a good improvement, I think it's not a bug.
>  > Probably we cannot backpatch it as it will end up having type names
>  > defined by different naming rules. I'd suggest discussing it on
>  > -hackers.
> Done.

Thank for updating the patch. Please register this item to the next CF
if not yet.

>
>  > Regarding the patch, I think we can merge makeUniqueTypeName() to
>  > makeArrayTypeName() as there is no caller of makeUniqueTypeName() who
>  > pass tryOriginal = true.
> I partially agree with you. But I have one reason to leave
> makeUniqueTypeName() separated:
> It may be used in other codes with auto generated types. For example, I
> think, the DefineRelation routine should choose composite type instead
> of using the same name as the table.

Okay.

I have one comment on v2 patch:

 +   for(;;)
 {
 -   dest[i - 1] = '_';
 -   strlcpy(dest + i, typeName, NAMEDATALEN - i);
 -   if (namelen + i >= NAMEDATALEN)
 -   truncate_identifier(dest, NAMEDATALEN, false);
 -
 if (!SearchSysCacheExists2(TYPENAMENSP,
 -  CStringGetDatum(dest),
 +  CStringGetDatum(type_name),
ObjectIdGetDatum(typeNamespace)))
 -   return pstrdup(dest);
 +   return type_name;
 +
 +   /* Previous attempt was failed. Prepare a new one. */
 +   pfree(type_name);
 +   snprintf(suffix, sizeof(suffix), "%d", ++pass);
 +   type_name = makeObjectName("", typeName, suffix);
 }

 return NULL;

I think it's better to break from the loop instead of returning from
there. That way, we won't need "return NULL".

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Postgres do not allow to create many tables with more than 63-symbols prefix

[Önder Kalacı]

Hi,

Thanks for working on this.

 >> According to subj you can try to create many tables (induced by the case
>  >> of partitioned table) with long prefix - see 6727v.sql for
> reproduction.
>  >> But now it's impossible because of logic of the makeUniqueTypeName()
>  >> routine.
>  >> You get the error:
>  >> ERROR:  could not form array type name for type ...
>  >>
>  >> It is very corner case, of course. But solution is easy and short. So,
>  >> why not to fix? - See the patch in attachment.
>  >
>  > While this seems to be a good improvement, I think it's not a bug.
>  > Probably we cannot backpatch it as it will end up having type names
>  > defined by different naming rules. I'd suggest discussing it on
>  > -hackers.
> Done.
>

On Citus extension, we hit a similar issue while creating partitions (over
multiple transactions in parallel). You can see some more discussions on
the related Github issue #5334
. We basically discuss this
behavior on the issue.

I tested this patch with the mentioned issue, and as expected the issue is
resolved.

Also, in general, the patch looks reasonable, following the approach
that ChooseRelationName()
implements makes sense to me as well.

Onder KALACI
Developing the Citus extension @Microsoft