From: Tom Lane <tgl@sss.pgh.pa.us>
To: Vik Fearing <vik.fearing@2ndquadrant.com>
cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Renaming a table to an array's autogenerated name
In-reply-to: <0f4ade49-4f0b-a9a3-c120-7589f01d1eb8@2ndquadrant.com>
References: <0f4ade49-4f0b-a9a3-c120-7589f01d1eb8@2ndquadrant.com>
Comments: In-reply-to Vik Fearing <vik.fearing@2ndquadrant.com>
	message dated "Thu, 25 May 2017 14:19:37 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <29395.1495747387.0@sss.pgh.pa.us>
Date: Thu, 25 May 2017 17:24:57 -0400
Message-ID: <29404.1495747497@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <29395.1495747387.1@sss.pgh.pa.us>

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> In commit 9aa3c782c93, Tom fixed a bug in which creating a table _foo
> when an array type of that name already existed would make the array
> type change its name to get out of the way. But it missed a trick in
> that renaming a table would still cause a conflict.

Good catch.

> One interesting thing to note however, is that if you rename a table to
> its own array's name (which was my case when I found this bug), the new
> array name will be ___foo instead of just __foo.  I don't know if it's
> worth worrying about that.

This seemed weird to me.  Stepping through the logic, I see that what's
happening is that the moveArrayTypeName call you added renames the
array type from "_foo" to "__foo", and then at the bottom of
RenameTypeInternal we call makeArrayTypeName/RenameTypeInternal again
on that array type.  makeArrayTypeName sees that "__foo" exists and
assumes the name is in use, even though it's really this same type and
no more renaming is needed.  So it ends up picking the next choice
"___foo".

After some experimentation, I came up with the attached, which simply
skips the "recursive" step if it would apply to the same array type we
already moved.

I added some queries to the regression test case to show exactly what
happens to the array type names, and in that, you can see that the
behavior for the "normal" case with distinct array types is that neither
array type ends up with a name that is just the unsurprising case of
an underscore followed by its element type's name; they *both* have an
extra underscore compared to that.  Maybe that's okay.  We could possibly
rejigger the order of renaming so that one of them ends with an
unsurprising name, but I failed to make that happen without considerably
more surgery.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-patch; name="rename-_foo-v02.patch";
	charset="us-ascii"
Content-ID: <29395.1495747387.2@sss.pgh.pa.us>
Content-Description: rename-_foo-v02.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index 04c10c6347b81183aa5c5ec3906bc0367672da29..74eeba6e0ab3aa127c1ba01e2c3=
7b770f92629cd 100644
*** a/src/backend/catalog/pg_type.c
--- b/src/backend/catalog/pg_type.c
*************** RenameTypeInternal(Oid typeOid, const ch
*** 695,700 ****
--- 695,701 ----
  	HeapTuple	tuple;
  	Form_pg_type typ;
  	Oid			arrayOid;
+ 	Oid			oldTypeOid;
=20=20
  	pg_type_desc =3D heap_open(TypeRelationId, RowExclusiveLock);
=20=20
*************** RenameTypeInternal(Oid typeOid, const ch
*** 708,720 ****
=20=20
  	arrayOid =3D typ->typarray;
=20=20
! 	/* Just to give a more friendly error than unique-index violation */
! 	if (SearchSysCacheExists2(TYPENAMENSP,
! 							  CStringGetDatum(newTypeName),
! 							  ObjectIdGetDatum(typeNamespace)))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_DUPLICATE_OBJECT),
! 				 errmsg("type \"%s\" already exists", newTypeName)));
=20=20
  	/* OK, do the rename --- tuple is a copy, so OK to scribble on it */
  	namestrcpy(&(typ->typname), newTypeName);
--- 709,736 ----
=20=20
  	arrayOid =3D typ->typarray;
=20=20
! 	/* Check for a conflicting type name. */
! 	oldTypeOid =3D GetSysCacheOid2(TYPENAMENSP,
! 								 CStringGetDatum(newTypeName),
! 								 ObjectIdGetDatum(typeNamespace));
!=20
! 	/*
! 	 * If there is one, see if it's an autogenerated array type, and if so
! 	 * rename it out of the way.  (But we must skip that for a shell type
! 	 * because moveArrayTypeName will do the wrong thing in that case.)
! 	 * Otherwise, we can at least give a more friendly error than unique-ind=
ex
! 	 * violation.
! 	 */
! 	if (OidIsValid(oldTypeOid))
! 	{
! 		if (get_typisdefined(oldTypeOid) &&
! 			moveArrayTypeName(oldTypeOid, newTypeName, typeNamespace))
! 			/* successfully dodged the problem */ ;
! 		else
! 			ereport(ERROR,
! 					(errcode(ERRCODE_DUPLICATE_OBJECT),
! 					 errmsg("type \"%s\" already exists", newTypeName)));
! 	}
=20=20
  	/* OK, do the rename --- tuple is a copy, so OK to scribble on it */
  	namestrcpy(&(typ->typname), newTypeName);
*************** RenameTypeInternal(Oid typeOid, const ch
*** 726,733 ****
  	heap_freetuple(tuple);
  	heap_close(pg_type_desc, RowExclusiveLock);
=20=20
! 	/* If the type has an array type, recurse to handle that */
! 	if (OidIsValid(arrayOid))
  	{
  		char	   *arrname =3D makeArrayTypeName(newTypeName, typeNamespace);
=20=20
--- 742,753 ----
  	heap_freetuple(tuple);
  	heap_close(pg_type_desc, RowExclusiveLock);
=20=20
! 	/*
! 	 * If the type has an array type, recurse to handle that.  But we don't
! 	 * need to do anything more if we already renamed that array type above
! 	 * (which would happen when, eg, renaming "foo" to "_foo").
! 	 */
! 	if (OidIsValid(arrayOid) && arrayOid !=3D oldTypeOid)
  	{
  		char	   *arrname =3D makeArrayTypeName(newTypeName, typeNamespace);
=20=20
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/e=
xpected/alter_table.out
index c88fd768482792bd8376dd21cc5b63be927f7727..8aadbb88a348571dd2fe6e22d1e=
2a2f72380d35b 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** SELECT * FROM tmp_new2;
*** 128,133 ****
--- 128,182 ----
=20=20
  DROP TABLE tmp_new;
  DROP TABLE tmp_new2;
+ --
+ -- check renaming to a table's array type's autogenerated name
+ -- (the array type's name should get out of the way)
+ --
+ CREATE TABLE tmp_array (id int);
+ CREATE TABLE tmp_array2 (id int);
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array[]'::regtype;
+   typname=20=20=20
+ ------------
+  _tmp_array
+ (1 row)
+=20
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array2[]'::regtype;
+    typname=20=20=20
+ -------------
+  _tmp_array2
+ (1 row)
+=20
+ ALTER TABLE tmp_array2 RENAME TO _tmp_array;
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array[]'::regtype;
+    typname=20=20=20
+ -------------
+  __tmp_array
+ (1 row)
+=20
+ SELECT typname FROM pg_type WHERE oid =3D '_tmp_array[]'::regtype;
+    typname=20=20=20=20
+ --------------
+  ___tmp_array
+ (1 row)
+=20
+ DROP TABLE _tmp_array;
+ DROP TABLE tmp_array;
+ -- renaming to table's own array type's name is an interesting corner case
+ CREATE TABLE tmp_array (id int);
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array[]'::regtype;
+   typname=20=20=20
+ ------------
+  _tmp_array
+ (1 row)
+=20
+ ALTER TABLE tmp_array RENAME TO _tmp_array;
+ SELECT typname FROM pg_type WHERE oid =3D '_tmp_array[]'::regtype;
+    typname=20=20=20
+ -------------
+  __tmp_array
+ (1 row)
+=20
+ DROP TABLE _tmp_array;
  -- ALTER TABLE ... RENAME on non-table relations
  -- renaming indexes (FIXME: this should probably test the index's functio=
nality)
  ALTER INDEX IF EXISTS __onek_unique1 RENAME TO tmp_onek_unique1;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/al=
ter_table.sql
index c0e29720dc5dbd6ef0b230ae129a32f5c1f456f0..c41b48785b5cdf2f4ad76ea8ec4=
14059b6de1824 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
*************** SELECT * FROM tmp_new2;
*** 165,170 ****
--- 165,190 ----
  DROP TABLE tmp_new;
  DROP TABLE tmp_new2;
=20=20
+ --
+ -- check renaming to a table's array type's autogenerated name
+ -- (the array type's name should get out of the way)
+ --
+ CREATE TABLE tmp_array (id int);
+ CREATE TABLE tmp_array2 (id int);
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array[]'::regtype;
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array2[]'::regtype;
+ ALTER TABLE tmp_array2 RENAME TO _tmp_array;
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array[]'::regtype;
+ SELECT typname FROM pg_type WHERE oid =3D '_tmp_array[]'::regtype;
+ DROP TABLE _tmp_array;
+ DROP TABLE tmp_array;
+=20
+ -- renaming to table's own array type's name is an interesting corner case
+ CREATE TABLE tmp_array (id int);
+ SELECT typname FROM pg_type WHERE oid =3D 'tmp_array[]'::regtype;
+ ALTER TABLE tmp_array RENAME TO _tmp_array;
+ SELECT typname FROM pg_type WHERE oid =3D '_tmp_array[]'::regtype;
+ DROP TABLE _tmp_array;
=20=20
  -- ALTER TABLE ... RENAME on non-table relations
  -- renaming indexes (FIXME: this should probably test the index's functio=
nality)

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

------- =_aaaaaaaaaa0--