From: Stephen Frost <sfrost@snowman.net>
To: pgsql-hackers@postgresql.org
Subject: Re: pg_upgrade loses security lables and COMMENTs on blobs
Date: Thu, 23 Feb 2017 10:36:37 -0500
Message-ID: <20170223153637.GV9812@tamriel.snowman.net> (raw)
In-Reply-To: <20170221162655.GE9812@tamriel.snowman.net>
References: <20170221162655.GE9812@tamriel.snowman.net>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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
Attachments:fix_pg_upgrade_blob_comments-master_v5.patch (text/x-diff, 13.0K) downloaddiff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.hnew file mode 100644index 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;
typedef struct _dumpOptions
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.cnew file mode 100644index 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") == 0))
res = res & REQ_SCHEMA;
}
--- 2876,2884 ----
/* Mask it if we only want schema */
if (ropt->schemaOnly)
{
! if (!(ropt->sequence_data && strcmp(te->desc, "SEQUENCE SET") == 0) &&
! !(ropt->binary_upgrade && strcmp(te->desc,"BLOB") == 0) &&
! !(ropt->binary_upgrade && strncmp(te->tag,"LARGE OBJECT ", 13) == 0))
res = res & REQ_SCHEMA;
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cnew file mode 100644index 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);
! if (dopt.outputBlobs)
getBlobs(fout);
/*
--- 775,789 ----
if (dopt.schemaOnly && dopt.sequence_data)
getTableData(&dopt, tblinfo, numTables, dopt.oids, RELKIND_SEQUENCE);
! /*
! * 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 comments
! * or security labels on blobs and we need to dump those out.
! */
! if (dopt.outputBlobs || dopt.binary_upgrade)
getBlobs(fout);
/*
*************** main(int argc, char **argv)
*** 855,860 ****
--- 863,869 ----
ropt->enable_row_security = dopt.enable_row_security;
ropt->sequence_data = dopt.sequence_data;
ropt->include_subscriptions = dopt.include_subscriptions;
+ ropt->binary_upgrade = dopt.binary_upgrade;
if (compressLevel == -1)
ropt->compression = 0;
*************** getBlobs(Archive *fout)
*** 2903,2908 ****
--- 2912,2931 ----
PQgetisnull(res, i, i_initlomacl) &&
PQgetisnull(res, i, i_initrlomacl))
binfo[i].dobj.dump &= ~DUMP_COMPONENT_ACL;
+ + /*+ * 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 &= ~(DUMP_COMPONENT_DATA | DUMP_COMPONENT_ACL);
}
/*
*************** dumpComment(Archive *fout, const char *t
*** 8831,8837 ****
}
else
{
! if (dopt->schemaOnly)
return;
}
--- 8854,8861 ----
}
else
{
! /* We do dump blob comments in binary-upgrade mode */
! if (dopt->schemaOnly && !dopt->binary_upgrade)
return;
}
*************** dumpSecLabel(Archive *fout, const char *
*** 14226,14232 ****
}
else
{
! if (dopt->schemaOnly)
return;
}
--- 14250,14257 ----
}
else
{
! /* We do dump blob security labels in binary-upgrade mode */
! if (dopt->schemaOnly && !dopt->binary_upgrade)
return;
}
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.plnew file mode 100644index 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 = (
*** 39,49 ****
binary_upgrade => {
dump_cmd => [
'pg_dump',
! "--file=$tempdir/binary_upgrade.sql",
'--schema-only',
'--binary-upgrade',
'-d', 'postgres', # alternative way to specify database
! ], },
clean => {
dump_cmd => [
'pg_dump',
--- 39,55 ----
binary_upgrade => {
dump_cmd => [
'pg_dump',
! '--format=custom',
! "--file=$tempdir/binary_upgrade.dump",
'--schema-only',
'--binary-upgrade',
'-d', 'postgres', # alternative way to specify database
! ],
! restore_cmd => [
! 'pg_restore', '-Fc',
! '--verbose',
! "--file=$tempdir/binary_upgrade.sql",
! "$tempdir/binary_upgrade.dump", ], },
clean => {
dump_cmd => [
'pg_dump',
*************** my %tests = (
*** 334,339 ****
--- 340,346 ----
all_runs => 1,
regexp => qr/^ALTER LARGE OBJECT \d+ OWNER TO .*;/m,
like => {
+ binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
column_inserts => 1,
*************** my %tests = (
*** 348,354 ****
section_pre_data => 1,
test_schema_plus_blobs => 1, },
unlike => {
- binary_upgrade => 1,
no_blobs => 1,
no_owner => 1,
only_dump_test_schema => 1,
--- 355,360 ----
*************** my %tests = (
*** 666,671 ****
--- 672,678 ----
'SELECT pg_catalog.lo_from_bytea(0, \'\\x310a320a330a340a350a360a370a380a390a\');',
regexp => qr/^SELECT pg_catalog\.lo_create\('\d+'\);/m,
like => {
+ binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
column_inserts => 1,
*************** my %tests = (
*** 681,687 ****
section_pre_data => 1,
test_schema_plus_blobs => 1, },
unlike => {
- binary_upgrade => 1,
no_blobs => 1,
only_dump_test_schema => 1,
only_dump_test_table => 1,
--- 688,693 ----diff --git a/src/test/regress/expected/large_object.out b/src/test/regress/expected/large_object.outnew file mode 100644index ...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 = 3001) SELECT 1;+ ?column? + ----------+ 1+ (1 row)+ + -- Test creation of a large object and leave it for testing pg_upgrade+ SELECT lo_create(3001);+ lo_create + -----------+ 3001+ (1 row)+ + COMMENT ON LARGE OBJECT 3001 IS 'testing comments';diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.outnew file mode 100644index 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
-----------
(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 >= 1000 AND oid < 3000;
lo_unlink
-----------
(0 rows)
*************** SELECT lo_unlink(2002);
*** 1173,1183 ****
\c -
-- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobject_metadata;
oid | ownername | lomacl
------+---------------+------------------------------------------------------------------------------------------------
- 1002 | regress_user1 |
1001 | regress_user1 | {regress_user1=rw/regress_user1,=rw/regress_user1}
1003 | regress_user1 | {regress_user1=rw/regress_user1,regress_user2=r/regress_user1}
1004 | regress_user1 | {regress_user1=rw/regress_user1,regress_user2=rw/regress_user1}
1005 | regress_user1 | {regress_user1=rw/regress_user1,regress_user2=r*w/regress_user1,regress_user3=r/regress_user2}
--- 1173,1183 ----
\c -
-- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000 ORDER BY oid;
oid | ownername | lomacl
------+---------------+------------------------------------------------------------------------------------------------
1001 | regress_user1 | {regress_user1=rw/regress_user1,=rw/regress_user1}
+ 1002 | regress_user1 |
1003 | regress_user1 | {regress_user1=rw/regress_user1,regress_user2=r/regress_user1}
1004 | regress_user1 | {regress_user1=rw/regress_user1,regress_user2=rw/regress_user1}
1005 | regress_user1 | {regress_user1=rw/regress_user1,regress_user2=r*w/regress_user1,regress_user3=r/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
-----------
1
--- 1546,1552 ----
DROP TABLE atestc;
DROP TABLE atestp1;
DROP TABLE atestp2;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000;
lo_unlink
-----------
1
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedulenew file mode 100644index 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 matview lock replica_identity rowsecurity object_address tablesample groupingsets drop_operator
# ----------
# Another group of parallel tests
--- 84,90 ----
# ----------
# Another group of parallel tests
# ----------
! test: brin gin gist spgist privileges init_privs security_label collate matview lock replica_identity rowsecurity object_address tablesample groupingsets drop_operator large_object
# ----------
# Another group of parallel tests
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedulenew file mode 100644index 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/large_object.sqlnew file mode 100644index ...a9e18b7
*** a/src/test/regress/sql/large_object.sql
--- b/src/test/regress/sql/large_object.sql
***************
*** 0 ****
--- 1,8 ----+ + -- This is more-or-less DROP IF EXISTS LARGE OBJECT 3001;+ WITH unlink AS (SELECT lo_unlink(loid) FROM pg_largeobject WHERE loid = 3001) SELECT 1;+ + -- Test creation of a large object and leave it for testing pg_upgrade+ SELECT lo_create(3001);+ + COMMENT ON LARGE OBJECT 3001 IS 'testing comments';diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sqlnew file mode 100644index 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;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata;
RESET client_min_messages;
--- 17,23 ----
DROP ROLE IF EXISTS regress_user5;
DROP ROLE IF EXISTS regress_user6;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000;
RESET client_min_messages;
*************** SELECT lo_unlink(2002);
*** 729,735 ****
\c -
-- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobject_metadata;
SET SESSION AUTHORIZATION regress_user3;
--- 729,735 ----
\c -
-- confirm ACL setting
! SELECT oid, pg_get_userbyid(lomowner) ownername, lomacl FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000 ORDER BY oid;
SET SESSION AUTHORIZATION regress_user3;
*************** DROP TABLE atestc;
*** 960,966 ****
DROP TABLE atestp1;
DROP TABLE atestp2;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata;
DROP GROUP regress_group1;
DROP GROUP regress_group2;
--- 960,966 ----
DROP TABLE atestp1;
DROP TABLE atestp2;
! SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000;
DROP GROUP regress_group1;
DROP GROUP regress_group2;
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net
Subject: Re: pg_upgrade loses security lables and COMMENTs on blobs
In-Reply-To: <20170223153637.GV9812@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox