Date: Thu, 23 Feb 2017 10:36:37 -0500
From: Stephen Frost <sfrost@snowman.net>
To: pgsql-hackers@postgresql.org
Subject: Re: pg_upgrade loses security lables and COMMENTs on blobs
Message-ID: <20170223153637.GV9812@tamriel.snowman.net>
References: <20170221162655.GE9812@tamriel.snowman.net>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="QqoAFJW0Wwrf1M3c"
Content-Disposition: inline
In-Reply-To: <20170221162655.GE9812@tamriel.snowman.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--QqoAFJW0Wwrf1M3c
Content-Type: multipart/mixed; boundary="KojRrsbczA2Vuv6e"
Content-Disposition: inline


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

All,

* Stephen Frost (sfrost@snowman.net) wrote:
> Just wanted to get a note out to -hackers about the issue, I'll see
> about getting a fix written up for it soon.

Attached is a patch which addresses this issue.  I'm not terribly
pleased with it, but I also haven't got any great ideas of what else to
do.  Suggestions welcome, of course.

Otherwise, I'll plan to start working on the back-branch changes for
this soon.

Thanks!

Stephen

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

diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
new file mode 100644
index 6480fb8..983a999
*** a/src/bin/pg_dump/pg_backup.h
--- b/src/bin/pg_dump/pg_backup.h
*************** typedef struct _restoreOptions
*** 120,125 ****
--- 120,126 ----
  	int			enable_row_security;
  	int			sequence_data;	/* dump sequence data even in schema-only mode */
  	int			include_subscriptions;
+ 	int			binary_upgrade;
  } RestoreOptions;
 =20
  typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_back=
up_archiver.c
new file mode 100644
index 929f1b5..6c340f9
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*************** _tocEntryRequired(TocEntry *te, teSectio
*** 2876,2882 ****
  	/* Mask it if we only want schema */
  	if (ropt->schemaOnly)
  	{
! 		if (!(ropt->sequence_data && strcmp(te->desc, "SEQUENCE SET") =3D=3D 0))
  			res =3D res & REQ_SCHEMA;
  	}
 =20
--- 2876,2884 ----
  	/* Mask it if we only want schema */
  	if (ropt->schemaOnly)
  	{
! 		if (!(ropt->sequence_data && strcmp(te->desc, "SEQUENCE SET") =3D=3D 0)=
 &&
! 			!(ropt->binary_upgrade && strcmp(te->desc,"BLOB") =3D=3D 0) &&
! 			!(ropt->binary_upgrade && strncmp(te->tag,"LARGE OBJECT ", 13) =3D=3D =
0))
  			res =3D res & REQ_SCHEMA;
  	}
 =20
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
new file mode 100644
index 89db310..44d4f32
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** main(int argc, char **argv)
*** 775,781 ****
  	if (dopt.schemaOnly && dopt.sequence_data)
  		getTableData(&dopt, tblinfo, numTables, dopt.oids, RELKIND_SEQUENCE);
 =20
! 	if (dopt.outputBlobs)
  		getBlobs(fout);
 =20
  	/*
--- 775,789 ----
  	if (dopt.schemaOnly && dopt.sequence_data)
  		getTableData(&dopt, tblinfo, numTables, dopt.oids, RELKIND_SEQUENCE);
 =20
! 	/*
! 	 * In binary-upgrade mode, we do not have to worry about the actual blob
! 	 * data or the associated metadata that resides in the pg_largeobject and
! 	 * pg_largeobject_metadata tables, respectivly.
! 	 *
! 	 * However, we do need to collect blob information as there may be comme=
nts
! 	 * or security labels on blobs and we need to dump those out.
! 	 */
! 	if (dopt.outputBlobs || dopt.binary_upgrade)
  		getBlobs(fout);
 =20
  	/*
*************** main(int argc, char **argv)
*** 855,860 ****
--- 863,869 ----
  	ropt->enable_row_security =3D dopt.enable_row_security;
  	ropt->sequence_data =3D dopt.sequence_data;
  	ropt->include_subscriptions =3D dopt.include_subscriptions;
+ 	ropt->binary_upgrade =3D dopt.binary_upgrade;
 =20
  	if (compressLevel =3D=3D -1)
  		ropt->compression =3D 0;
*************** getBlobs(Archive *fout)
*** 2903,2908 ****
--- 2912,2931 ----
  			PQgetisnull(res, i, i_initlomacl) &&
  			PQgetisnull(res, i, i_initrlomacl))
  			binfo[i].dobj.dump &=3D ~DUMP_COMPONENT_ACL;
+=20
+ 		/*
+ 		 * In binary-upgrade mode for blobs, we do *not* dump out the data or
+ 		 * the ACLs, should any exist.  The data and ACL (if any) will be
+ 		 * copied by pg_upgrade, which simply copies the pg_largeobject and
+ 		 * pg_largeobject_metadata tables.
+ 		 *
+ 		 * We *do* dump out the definition of the blob because we need that
+ 		 * to make the restoration of the comments, and anything else, work
+ 		 * since pg_upgrade copies the files behind pg_largeobject and
+ 		 * pg_largeobject_metadata after the dump is restored.
+ 		 */
+ 		if (dopt->binary_upgrade)
+ 			binfo[i].dobj.dump &=3D ~(DUMP_COMPONENT_DATA | DUMP_COMPONENT_ACL);
  	}
 =20
  	/*
*************** dumpComment(Archive *fout, const char *t
*** 8831,8837 ****
  	}
  	else
  	{
! 		if (dopt->schemaOnly)
  			return;
  	}
 =20
--- 8854,8861 ----
  	}
  	else
  	{
! 		/* We do dump blob comments in binary-upgrade mode */
! 		if (dopt->schemaOnly && !dopt->binary_upgrade)
  			return;
  	}
 =20
*************** dumpSecLabel(Archive *fout, const char *
*** 14226,14232 ****
  	}
  	else
  	{
! 		if (dopt->schemaOnly)
  			return;
  	}
 =20
--- 14250,14257 ----
  	}
  	else
  	{
! 		/* We do dump blob security labels in binary-upgrade mode */
! 		if (dopt->schemaOnly && !dopt->binary_upgrade)
  			return;
  	}
 =20
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_du=
mp.pl
new file mode 100644
index f73bf89..c51088a
*** a/src/bin/pg_dump/t/002_pg_dump.pl
--- b/src/bin/pg_dump/t/002_pg_dump.pl
*************** my %pgdump_runs =3D (
*** 39,49 ****
  	binary_upgrade =3D> {
  		dump_cmd =3D> [
  			'pg_dump',
! 			"--file=3D$tempdir/binary_upgrade.sql",
  			'--schema-only',
  			'--binary-upgrade',
  			'-d', 'postgres',    # alternative way to specify database
! 		], },
  	clean =3D> {
  		dump_cmd =3D> [
  			'pg_dump',
--- 39,55 ----
  	binary_upgrade =3D> {
  		dump_cmd =3D> [
  			'pg_dump',
! 			'--format=3Dcustom',
! 			"--file=3D$tempdir/binary_upgrade.dump",
  			'--schema-only',
  			'--binary-upgrade',
  			'-d', 'postgres',    # alternative way to specify database
! 		],
! 		restore_cmd =3D> [
! 			'pg_restore', '-Fc',
! 			'--verbose',
! 			"--file=3D$tempdir/binary_upgrade.sql",
! 			"$tempdir/binary_upgrade.dump", ], },
  	clean =3D> {
  		dump_cmd =3D> [
  			'pg_dump',
*************** my %tests =3D (
*** 334,339 ****
--- 340,346 ----
  		all_runs =3D> 1,
  		regexp   =3D> qr/^ALTER LARGE OBJECT \d+ OWNER TO .*;/m,
  		like     =3D> {
+ 			binary_upgrade           =3D> 1,
  			clean                    =3D> 1,
  			clean_if_exists          =3D> 1,
  			column_inserts           =3D> 1,
*************** my %tests =3D (
*** 348,354 ****
  			section_pre_data         =3D> 1,
  			test_schema_plus_blobs   =3D> 1, },
  		unlike =3D> {
- 			binary_upgrade           =3D> 1,
  			no_blobs                 =3D> 1,
  			no_owner                 =3D> 1,
  			only_dump_test_schema    =3D> 1,
--- 355,360 ----
*************** my %tests =3D (
*** 666,671 ****
--- 672,678 ----
  'SELECT pg_catalog.lo_from_bytea(0, \'\\x310a320a330a340a350a360a370a380a=
390a\');',
  		regexp =3D> qr/^SELECT pg_catalog\.lo_create\('\d+'\);/m,
  		like   =3D> {
+ 			binary_upgrade           =3D> 1,
  			clean                    =3D> 1,
  			clean_if_exists          =3D> 1,
  			column_inserts           =3D> 1,
*************** my %tests =3D (
*** 681,687 ****
  			section_pre_data         =3D> 1,
  			test_schema_plus_blobs   =3D> 1, },
  		unlike =3D> {
- 			binary_upgrade           =3D> 1,
  			no_blobs                 =3D> 1,
  			only_dump_test_schema    =3D> 1,
  			only_dump_test_table     =3D> 1,
--- 688,693 ----
diff --git a/src/test/regress/expected/large_object.out b/src/test/regress/=
expected/large_object.out
new file mode 100644
index ...b00d47c
*** a/src/test/regress/expected/large_object.out
--- b/src/test/regress/expected/large_object.out
***************
*** 0 ****
--- 1,15 ----
+ -- This is more-or-less DROP IF EXISTS LARGE OBJECT 3001;
+ WITH unlink AS (SELECT lo_unlink(loid) FROM pg_largeobject WHERE loid =3D=
 3001) SELECT 1;
+  ?column?=20
+ ----------
+         1
+ (1 row)
+=20
+ -- Test creation of a large object and leave it for testing pg_upgrade
+ SELECT lo_create(3001);
+  lo_create=20
+ -----------
+       3001
+ (1 row)
+=20
+ COMMENT ON LARGE OBJECT 3001 IS 'testing comments';
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/ex=
pected/privileges.out
new file mode 100644
index f66b443..1af66ff
*** a/src/test/regress/expected/privileges.out
--- b/src/test/regress/expected/privileges.out
*************** DROP ROLE IF EXISTS regress_user3;
*** 12,18 ****
  DROP ROLE IF EXISTS regress_user4;
  DROP ROLE IF EXISTS regress_user5;
  DROP ROLE IF EXISTS regress_user6;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata;
   lo_unlink=20
  -----------
  (0 rows)
--- 12,18 ----
  DROP ROLE IF EXISTS regress_user4;
  DROP ROLE IF EXISTS regress_user5;
  DROP ROLE IF EXISTS regress_user6;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >=3D 1000 AN=
D oid < 3000;
   lo_unlink=20
  -----------
  (0 rows)
*************** SELECT lo_unlink(2002);
*** 1173,1183 ****
 =20
  \c -
  -- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobje=
ct_metadata;
   oid  |   ownername   |                                             lomac=
l                                            =20
  ------+---------------+--------------------------------------------------=
----------------------------------------------
-  1002 | regress_user1 |=20
   1001 | regress_user1 | {regress_user1=3Drw/regress_user1,=3Drw/regress_u=
ser1}
   1003 | regress_user1 | {regress_user1=3Drw/regress_user1,regress_user2=
=3Dr/regress_user1}
   1004 | regress_user1 | {regress_user1=3Drw/regress_user1,regress_user2=
=3Drw/regress_user1}
   1005 | regress_user1 | {regress_user1=3Drw/regress_user1,regress_user2=
=3Dr*w/regress_user1,regress_user3=3Dr/regress_user2}
--- 1173,1183 ----
 =20
  \c -
  -- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobje=
ct_metadata WHERE oid >=3D 1000 AND oid < 3000 ORDER BY oid;
   oid  |   ownername   |                                             lomac=
l                                            =20
  ------+---------------+--------------------------------------------------=
----------------------------------------------
   1001 | regress_user1 | {regress_user1=3Drw/regress_user1,=3Drw/regress_u=
ser1}
+  1002 | regress_user1 |=20
   1003 | regress_user1 | {regress_user1=3Drw/regress_user1,regress_user2=
=3Dr/regress_user1}
   1004 | regress_user1 | {regress_user1=3Drw/regress_user1,regress_user2=
=3Drw/regress_user1}
   1005 | regress_user1 | {regress_user1=3Drw/regress_user1,regress_user2=
=3Dr*w/regress_user1,regress_user3=3Dr/regress_user2}
*************** DROP TABLE atest6;
*** 1546,1552 ****
  DROP TABLE atestc;
  DROP TABLE atestp1;
  DROP TABLE atestp2;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata;
   lo_unlink=20
  -----------
           1
--- 1546,1552 ----
  DROP TABLE atestc;
  DROP TABLE atestp1;
  DROP TABLE atestp2;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >=3D 1000 AN=
D oid < 3000;
   lo_unlink=20
  -----------
           1
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel=
_schedule
new file mode 100644
index edeb2d6..1f2fb59
*** a/src/test/regress/parallel_schedule
--- b/src/test/regress/parallel_schedule
*************** test: select_into select_distinct select
*** 84,90 ****
  # ----------
  # Another group of parallel tests
  # ----------
! test: brin gin gist spgist privileges init_privs security_label collate m=
atview lock replica_identity rowsecurity object_address tablesample groupin=
gsets drop_operator
 =20
  # ----------
  # Another group of parallel tests
--- 84,90 ----
  # ----------
  # Another group of parallel tests
  # ----------
! test: brin gin gist spgist privileges init_privs security_label collate m=
atview lock replica_identity rowsecurity object_address tablesample groupin=
gsets drop_operator large_object
 =20
  # ----------
  # Another group of parallel tests
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_sch=
edule
new file mode 100644
index 27a46d7..9ffceff
*** a/src/test/regress/serial_schedule
--- b/src/test/regress/serial_schedule
*************** test: object_address
*** 116,121 ****
--- 116,122 ----
  test: tablesample
  test: groupingsets
  test: drop_operator
+ test: large_object
  test: alter_generic
  test: alter_operator
  test: misc
diff --git a/src/test/regress/sql/large_object.sql b/src/test/regress/sql/l=
arge_object.sql
new file mode 100644
index ...a9e18b7
*** a/src/test/regress/sql/large_object.sql
--- b/src/test/regress/sql/large_object.sql
***************
*** 0 ****
--- 1,8 ----
+=20
+ -- This is more-or-less DROP IF EXISTS LARGE OBJECT 3001;
+ WITH unlink AS (SELECT lo_unlink(loid) FROM pg_largeobject WHERE loid =3D=
 3001) SELECT 1;
+=20
+ -- Test creation of a large object and leave it for testing pg_upgrade
+ SELECT lo_create(3001);
+=20
+ COMMENT ON LARGE OBJECT 3001 IS 'testing comments';
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/pri=
vileges.sql
new file mode 100644
index 00dc7bd..82d6751
*** a/src/test/regress/sql/privileges.sql
--- b/src/test/regress/sql/privileges.sql
*************** DROP ROLE IF EXISTS regress_user4;
*** 17,23 ****
  DROP ROLE IF EXISTS regress_user5;
  DROP ROLE IF EXISTS regress_user6;
 =20
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata;
 =20
  RESET client_min_messages;
 =20
--- 17,23 ----
  DROP ROLE IF EXISTS regress_user5;
  DROP ROLE IF EXISTS regress_user6;
 =20
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >=3D 1000 AN=
D oid < 3000;
 =20
  RESET client_min_messages;
 =20
*************** SELECT lo_unlink(2002);
*** 729,735 ****
 =20
  \c -
  -- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobje=
ct_metadata;
 =20
  SET SESSION AUTHORIZATION regress_user3;
 =20
--- 729,735 ----
 =20
  \c -
  -- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobje=
ct_metadata WHERE oid >=3D 1000 AND oid < 3000 ORDER BY oid;
 =20
  SET SESSION AUTHORIZATION regress_user3;
 =20
*************** DROP TABLE atestc;
*** 960,966 ****
  DROP TABLE atestp1;
  DROP TABLE atestp2;
 =20
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata;
 =20
  DROP GROUP regress_group1;
  DROP GROUP regress_group2;
--- 960,966 ----
  DROP TABLE atestp1;
  DROP TABLE atestp2;
 =20
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >=3D 1000 AN=
D oid < 3000;
 =20
  DROP GROUP regress_group1;
  DROP GROUP regress_group2;

--KojRrsbczA2Vuv6e--

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

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

iQIcBAEBCgAGBQJYrwGEAAoJEO1sijiDR2RVgJAQAKtQUgIRwNqQkvgZCVZS8rw7
Eajp2eLdYdPkI0nuNDXumpoqhIr1XrwxPtvBoGQptDB7eOS5++uhREFhflD0rmti
r7W/PWkCUsseEcQ4oXF6TZU+aOyIx0FuI72xksKvt9G3BsGkeoYwOUq9AVrIgxwh
bWpkOwNDZFAru0YJ+vUue4Tc264ZJRm9Jp2yN+3NQsddjQcI46XSO44nzi8KEkPS
Vgy8przetceVDTgjibwwBA3iDdrpPUDpEdwUtFDG2ItcFMizNBpDD1DOFc9df1X5
wKRH2gH1YBtQOIFwFTcEWQu4zPKdaIwwI4FVeuUbVXtvUFGMQ6kVs9XGB4awcmnQ
u3cJijA9h98krFSMWk6n+N75ibJWUQTx7D4oPnUrtSHzQuE2Ur8F2iycWrO9TKlI
765jXsMxx2DtsEI8knJvGA9JRZf5SnqT47T3PEPvICi4TAnZ1qgfQH6pM3c2a7PN
ngQjBFDwoXWMc5HLNFDMcivVFKOd4o8Ph9iB+cfSfvcnlLxXe0QgmAa/Pvlwsqky
NcFdYIoeeJp1ZQAbTF4wcHDSN2iDagKNPK4eM9CHlDtIxqC6c5D4DqVBX/6kx9Zm
NluQ2lwZgLtz3oC0xU9gaKH/UQEIGhzYPvd5ryHqGVkLwCJ2U9KE535M77YMpJTx
VmY5tTIQ5Sem3z9n6Awl
=K9kU
-----END PGP SIGNATURE-----

--QqoAFJW0Wwrf1M3c--