Date: Wed, 7 Dec 2016 15:09:15 -0500
From: Stephen Frost <sfrost@snowman.net>
To: pgsql-hackers@postgresql.org
Subject: Re: pg_dump vs. TRANSFORMs
Message-ID: <20161207200915.GZ23417@tamriel.snowman.net>
References: <20160504183952.GE10850@tamriel.snowman.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="xnH/NIbSm9ew9GxF"
Content-Disposition: inline
In-Reply-To: <20160504183952.GE10850@tamriel.snowman.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--xnH/NIbSm9ew9GxF
Content-Type: multipart/mixed; boundary="YgGp9RfepglW4nsE"
Content-Disposition: inline


--YgGp9RfepglW4nsE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

All,

* Stephen Frost (sfrost@snowman.net) wrote:
> While testing pg_dump, I discovered that there seems to be an issue when
> it comes to TRANSFORMs.

[...]

As pointed out by Peter E, this also impacts CASTs.  Attached is a patch
which addresses both by simply also pulling any functions which are
referenced from pg_cast or pg_transform when they have OIDs at or after
FirstNormalObjectId.  I also modified dumpCast() and dumpTransform() to
complain loudly if they're unable to dump out the cast or transform due
to not finding the function definition(s) necessary.

This'll need to be back-patched to 9.5 for the pg_transform look up and
all the way for pg_cast, though I don't expect that to be too difficult.

We don't do anything else with FirstNormalObjectId in SQL code in
pg_dump, though we obviously use it all over the place in the actual
code based on the OIDs returned from the database.  Still, does anyone
see an issue with using it in a query?  Without it, we end grabbing the
info for 100+ or so functions in a default install that we don't need,
which isn't horrible, but there were concerns raised before about
pg_dump performance for very small databases.

This also adds in regression tests to pg_dump for casts and transforms
and the pg_upgrade testing with the regression database will now
actually test the dump/restore of transforms (which it didn't previously
because the transforms weren't dumped).

Thoughts?

Thanks!

Stephen

--YgGp9RfepglW4nsE
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix_pg_dump_cast_transform_v1.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 3eaa8d32170b69e58b5780c0aa92b05d10869389 Mon Sep 17 00:00:00 2001
=46rom: Stephen Frost <sfrost@snowman.net>
Date: Wed, 7 Dec 2016 14:59:44 -0500
Subject: [PATCH] Fix dumping of casts and transforms using built-in functio=
ns

In pg_dump.c dumpCast() and dumpTransform(), we would happily ignore the
cast or transform if it happened to use a built-in function because we
weren't including the information about built-in functions when querying
pg_proc from getFuncs().

Modify the query in getFuncs() to also gather information about
functions which are used by user-defined casts and transforms (where
"user-defined" means "has an OID >=3D FirstNormalObjectId").  This also
adds to the TAP regression tests for 9.6 and master to cover these
types of objects.

Back-patch all the way for casts, back to 9.5 for transforms.

Discussion: https://www.postgresql.org/message-id/flat/20160504183952.GE108=
50%40tamriel.snowman.net
---
 src/bin/pg_dump/pg_dump.c        | 33 +++++++++++++---
 src/bin/pg_dump/t/002_pg_dump.pl | 85 +++++++++++++++++++++++++-----------=
----
 2 files changed, 80 insertions(+), 38 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 42873bb..530500c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4721,12 +4721,21 @@ getFuncs(Archive *fout, int *numFuncs)
 						  "\n  AND ("
 						  "\n  pronamespace !=3D "
 						  "(SELECT oid FROM pg_namespace "
-						  "WHERE nspname =3D 'pg_catalog')",
+						  "WHERE nspname =3D 'pg_catalog')"
+						  "\n  OR EXISTS (SELECT 1 FROM pg_cast"
+						  "\n  WHERE pg_cast.oid >=3D %u "
+						  "\n  AND p.oid =3D pg_cast.castfunc)"
+						  "\n  OR EXISTS (SELECT 1 FROM pg_transform"
+						  "\n  WHERE pg_transform.oid >=3D %u AND "
+						  "\n  (p.oid =3D pg_transform.trffromsql"
+						  "\n  OR p.oid =3D pg_transform.trftosql))",
 						  acl_subquery->data,
 						  racl_subquery->data,
 						  initacl_subquery->data,
 						  initracl_subquery->data,
-						  username_subquery);
+						  username_subquery,
+						  FirstNormalObjectId,
+						  FirstNormalObjectId);
 		if (dopt->binary_upgrade)
 			appendPQExpBufferStr(query,
 							   "\n  OR EXISTS(SELECT 1 FROM pg_depend WHERE "
@@ -4764,7 +4773,16 @@ getFuncs(Archive *fout, int *numFuncs)
 							 "\n  AND ("
 							 "\n  pronamespace !=3D "
 							 "(SELECT oid FROM pg_namespace "
-							 "WHERE nspname =3D 'pg_catalog')");
+							 "WHERE nspname =3D 'pg_catalog')"
+							 "\n  OR EXISTS (SELECT 1 FROM pg_cast"
+							 "\n  WHERE p.oid =3D pg_cast.castfunc)");
+
+		if (fout->remoteVersion >=3D 90500)
+			appendPQExpBufferStr(query,
+								 "\n  OR EXISTS (SELECT 1 FROM pg_transform"
+								 "\n  WHERE p.oid =3D pg_transform.trffromsql"
+								 "\n  OR p.oid =3D pg_transform.trftosql)");
+
 		if (dopt->binary_upgrade && fout->remoteVersion >=3D 90100)
 			appendPQExpBufferStr(query,
 							   "\n  OR EXISTS(SELECT 1 FROM pg_depend WHERE "
@@ -10828,7 +10846,8 @@ dumpCast(Archive *fout, CastInfo *cast)
 	{
 		funcInfo =3D findFuncByOid(cast->castfunc);
 		if (funcInfo =3D=3D NULL)
-			return;
+			exit_horribly(NULL, "unable to find function definition for OID %u",
+						  cast->castfunc);
 	}
=20
 	/*
@@ -10937,13 +10956,15 @@ dumpTransform(Archive *fout, TransformInfo *trans=
form)
 	{
 		fromsqlFuncInfo =3D findFuncByOid(transform->trffromsql);
 		if (fromsqlFuncInfo =3D=3D NULL)
-			return;
+			exit_horribly(NULL, "unable to find function definition for OID %u",
+						  transform->trffromsql);
 	}
 	if (OidIsValid(transform->trftosql))
 	{
 		tosqlFuncInfo =3D findFuncByOid(transform->trftosql);
 		if (tosqlFuncInfo =3D=3D NULL)
-			return;
+			exit_horribly(NULL, "unable to find function definition for OID %u",
+						  transform->trftosql);
 	}
=20
 	/* Make sure we are in proper schema (needed for getFormattedTypeName) */
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_du=
mp.pl
index f895522..59191cc 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1108,6 +1108,33 @@ my %tests =3D (
 			section_post_data        =3D> 1,
 			test_schema_plus_blobs   =3D> 1, }, },
=20
+	'CREATE CAST FOR timestamptz' =3D> {
+		create_order =3D> 51,
+		create_sql =3D> 'CREATE CAST (timestamptz AS interval) WITH FUNCTION age=
(timestamptz) AS ASSIGNMENT;',
+		regexp =3D> qr/CREATE CAST \(timestamp with time zone AS interval\) WITH=
 FUNCTION pg_catalog\.age\(timestamp with time zone\) AS ASSIGNMENT;/m,
+		like =3D> {
+			binary_upgrade =3D> 1,
+			clean =3D> 1,
+			clean_if_exists =3D> 1,
+			createdb =3D> 1,
+			defaults =3D> 1,
+			exclude_dump_test_schema =3D> 1,
+			exclude_test_table =3D> 1,
+			exclude_test_table_data =3D> 1,
+			no_blobs                =3D> 1,
+			no_privs =3D> 1,
+			no_owner =3D> 1,
+			pg_dumpall_dbprivs       =3D> 1,
+			schema_only =3D> 1,
+			section_pre_data =3D> 1,
+		},
+		unlike =3D> {
+			only_dump_test_schema =3D> 1,
+			only_dump_test_table =3D> 1,
+			pg_dumpall_globals =3D> 1,
+			section_post_data =3D> 1,
+			test_schema_plus_blobs =3D> 1, }, },
+
 	'CREATE DATABASE postgres' =3D> {
 		all_runs =3D> 1,
 		regexp =3D> qr/^
@@ -1856,38 +1883,32 @@ my %tests =3D (
 			section_post_data        =3D> 1,
 			test_schema_plus_blobs   =3D> 1, }, },
=20
-#######################################
-	# Currently broken.
-#######################################
-#
-#	'CREATE TRANSFORM FOR int' =3D> {
-#		create_order =3D> 34,
-#		create_sql =3D> 'CREATE TRANSFORM FOR int LANGUAGE SQL (FROM SQL WITH F=
UNCTION varchar_transform(internal), TO SQL WITH FUNCTION int4recv(internal=
));',
-#		regexp =3D> qr/CREATE TRANSFORM FOR int LANGUAGE SQL \(FROM SQL WITH FU=
NCTION varchar_transform\(internal\), TO SQL WITH FUNCTION int4recv\(intern=
al\)\);/m,
-#		like =3D> {
-#			binary_upgrade =3D> 1,
-#			clean =3D> 1,
-#			clean_if_exists =3D> 1,
-#			createdb =3D> 1,
-#			defaults =3D> 1,
-#			exclude_dump_test_schema =3D> 1,
-#			exclude_test_table =3D> 1,
-#			exclude_test_table_data =3D> 1,
-#			no_blobs                =3D> 1,
-#			no_privs =3D> 1,
-#			no_owner =3D> 1,
-#			pg_dumpall_dbprivs       =3D> 1,
-#			schema_only =3D> 1,
-#			section_post_data =3D> 1,
-#		},
-#		unlike =3D> {
-#			section_pre_data =3D> 1,
-#			only_dump_test_schema =3D> 1,
-#			only_dump_test_table =3D> 1,
-#			pg_dumpall_globals =3D> 1,
-#			test_schema_plus_blobs =3D> 1,
-#		},
-#	},
+	'CREATE TRANSFORM FOR int' =3D> {
+		create_order =3D> 34,
+		create_sql =3D> 'CREATE TRANSFORM FOR int LANGUAGE SQL (FROM SQL WITH FU=
NCTION varchar_transform(internal), TO SQL WITH FUNCTION int4recv(internal)=
);',
+		regexp =3D> qr/CREATE TRANSFORM FOR integer LANGUAGE sql \(FROM SQL WITH=
 FUNCTION pg_catalog\.varchar_transform\(internal\), TO SQL WITH FUNCTION p=
g_catalog\.int4recv\(internal\)\);/m,
+		like =3D> {
+			binary_upgrade =3D> 1,
+			clean =3D> 1,
+			clean_if_exists =3D> 1,
+			createdb =3D> 1,
+			defaults =3D> 1,
+			exclude_dump_test_schema =3D> 1,
+			exclude_test_table =3D> 1,
+			exclude_test_table_data =3D> 1,
+			no_blobs                =3D> 1,
+			no_privs =3D> 1,
+			no_owner =3D> 1,
+			pg_dumpall_dbprivs       =3D> 1,
+			schema_only =3D> 1,
+			section_pre_data =3D> 1,
+		},
+		unlike =3D> {
+			only_dump_test_schema =3D> 1,
+			only_dump_test_table =3D> 1,
+			pg_dumpall_globals =3D> 1,
+			section_post_data =3D> 1,
+			test_schema_plus_blobs =3D> 1, }, },
=20
 	'CREATE LANGUAGE pltestlang' =3D> {
 		all_runs =3D> 1,
--=20
2.7.4


--YgGp9RfepglW4nsE--

--xnH/NIbSm9ew9GxF
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJYSGxrAAoJEO1sijiDR2RVqIIQAMj3VRj+yJrWaj/2/wng+hwW
Mp8t0HsNDEPXe2yV/45D3wmVDDRf+FiF8Tlz2OTrapVzCg7NplyY++vWTJmhWwkx
G53UV0DLXfrreb4xqIqNfU6Py1fX81O+cBtfJOepTWVEWemrGUrsVGQGnf34emxO
DjQWKB/YRtHKGiNhyPvlR5xgKnSgZXdB1veD6jTTbfvPfotySXwqeWi2ppz8Iaov
sXQcyphQoWXlmnzBKOQxN1f4RyRzCXcPpzeymWPsVb2JbsaEk+jeFevj4hy9+xhe
7G9wb1x5yqXfBQGU519gap6h20vbcQan12Ydplg6vH4900BgqrOdMd2oPu8DjdmQ
5TIKac2gaEW++EaDppinfhgNEtI+8TYFQFNLjw7yqT6HQaBvn7QxqJg59jIVowvC
S8/jlA3pm3QmyAbTuWJMg3u5Sez7FXdsGrhe4b7FeVRZSY1SyUHKkScBDrokH4RA
UE0vTL2BzDPX0bsFeHM0/Ow0v9KtZ4YkbR07Aj3gXR9HfdyyO7CRqNE71/PIXpkG
tGjRjWsAU8Sbn29HZUFj7gDTgvrYK1cEqw/egIaIt6/+fiq4cdvYzEkggGEpKGGa
BhmjQ1IK90qyOnwW++VDHlatY9TIzJoM0KRzC5QfZDklWLffI73C8x6VUyPvENBC
1hnBJB9WwpmW8FwWnzOs
=yTPF
-----END PGP SIGNATURE-----

--xnH/NIbSm9ew9GxF--