Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump - Mailing list pgsql-hackers
| From | Tom Lane |
|---|---|
| Subject | Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump |
| Date | |
| Msg-id | 29048.1516812451@sss.pgh.pa.us Whole thread Raw |
| In response to | Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump
|
| List | pgsql-hackers |
I wrote:
> I went looking and realized that actually what we're interested in here
> is the plpgsql extension, not the plpgsql language ... and in fact the
> behavior I was thinking of is already there, except for some reason it's
> only applied during binary upgrade. So I think we should give serious
> consideration to the attached, which just removes the binary_upgrade
> condition (and adjusts nearby comments).
In further testing of that, I noticed that it made the behavior of our
other bugaboo, the public schema, rather inconsistent. With this
builtin-extensions hack, the plpgsql extension doesn't get dumped,
whether or not you say "clean". But the public schema does get
dropped and recreated if you say "clean". That's not helpful for
non-superuser users of pg_dump, so I think we should try to fix it.
Hence, the attached updated patch also makes selection of the public
schema use the DUMP_COMPONENT infrastructure rather than hardwired
code.
I note btw that the removed bit in getNamespaces() is flat out wrong
independently of these other considerations, because it makes the SQL
put into an archive file vary depending on whether --clean was specified
at pg_dump time. That's not supposed to happen.
I've also updated the regression test this time, as I think this
is committable.
regards, tom lane
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index f55aa36..fb03793 100644
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*************** _printTocEntry(ArchiveHandle *AH, TocEnt
*** 3453,3481 ****
{
RestoreOptions *ropt = AH->public.ropt;
- /*
- * Avoid dumping the public schema, as it will already be created ...
- * unless we are using --clean mode (and *not* --create mode), in which
- * case we've previously issued a DROP for it so we'd better recreate it.
- *
- * Likewise for its comment, if any. (We could try issuing the COMMENT
- * command anyway; but it'd fail if the restore is done as non-super-user,
- * so let's not.)
- *
- * XXX it looks pretty ugly to hard-wire the public schema like this, but
- * it sits in a sort of no-mans-land between being a system object and a
- * user object, so it really is special in a way.
- */
- if (!(ropt->dropSchema && !ropt->createDB))
- {
- if (strcmp(te->desc, "SCHEMA") == 0 &&
- strcmp(te->tag, "public") == 0)
- return;
- if (strcmp(te->desc, "COMMENT") == 0 &&
- strcmp(te->tag, "SCHEMA public") == 0)
- return;
- }
-
/* Select owner, schema, and tablespace as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te->namespace);
--- 3453,3458 ----
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d65ea54..50399c9 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** selectDumpableNamespace(NamespaceInfo *n
*** 1407,1412 ****
--- 1407,1425 ----
/* Other system schemas don't get dumped */
nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_NONE;
}
+ else if (strcmp(nsinfo->dobj.name, "public") == 0)
+ {
+ /*
+ * The public schema is a strange beast that sits in a sort of
+ * no-mans-land between being a system object and a user object. We
+ * don't want to dump creation or comment commands for it, because
+ * that complicates matters for non-superuser use of pg_dump. But we
+ * should dump any ACL changes that have occurred for it, and of
+ * course we should dump contained objects.
+ */
+ nsinfo->dobj.dump = DUMP_COMPONENT_ACL;
+ nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
+ }
else
nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
*************** selectDumpableAccessMethod(AccessMethodI
*** 1617,1637 ****
* selectDumpableExtension: policy-setting subroutine
* Mark an extension as to be dumped or not
*
! * Normally, we dump all extensions, or none of them if include_everything
! * is false (i.e., a --schema or --table switch was given). However, in
! * binary-upgrade mode it's necessary to skip built-in extensions, since we
* assume those will already be installed in the target database. We identify
* such extensions by their having OIDs in the range reserved for initdb.
*/
static void
selectDumpableExtension(ExtensionInfo *extinfo, DumpOptions *dopt)
{
/*
! * Use DUMP_COMPONENT_ACL for from-initdb extensions, to allow users to
! * change permissions on those objects, if they wish to, and have those
! * changes preserved.
*/
! if (dopt->binary_upgrade && extinfo->dobj.catId.oid <= (Oid) g_last_builtin_oid)
extinfo->dobj.dump = extinfo->dobj.dump_contains = DUMP_COMPONENT_ACL;
else
extinfo->dobj.dump = extinfo->dobj.dump_contains =
--- 1630,1650 ----
* selectDumpableExtension: policy-setting subroutine
* Mark an extension as to be dumped or not
*
! * Built-in extensions should be skipped except for checking ACLs, since we
* assume those will already be installed in the target database. We identify
* such extensions by their having OIDs in the range reserved for initdb.
+ * We dump all user-added extensions by default, or none of them if
+ * include_everything is false (i.e., a --schema or --table switch was given).
*/
static void
selectDumpableExtension(ExtensionInfo *extinfo, DumpOptions *dopt)
{
/*
! * Use DUMP_COMPONENT_ACL for built-in extensions, to allow users to
! * change permissions on their member objects, if they wish to, and have
! * those changes preserved.
*/
! if (extinfo->dobj.catId.oid <= (Oid) g_last_builtin_oid)
extinfo->dobj.dump = extinfo->dobj.dump_contains = DUMP_COMPONENT_ACL;
else
extinfo->dobj.dump = extinfo->dobj.dump_contains =
*************** getNamespaces(Archive *fout, int *numNam
*** 4435,4463 ****
init_acl_subquery->data,
init_racl_subquery->data);
- /*
- * When we are doing a 'clean' run, we will be dropping and recreating
- * the 'public' schema (the only object which has that kind of
- * treatment in the backend and which has an entry in pg_init_privs)
- * and therefore we should not consider any initial privileges in
- * pg_init_privs in that case.
- *
- * See pg_backup_archiver.c:_printTocEntry() for the details on why
- * the public schema is special in this regard.
- *
- * Note that if the public schema is dropped and re-created, this is
- * essentially a no-op because the new public schema won't have an
- * entry in pg_init_privs anyway, as the entry will be removed when
- * the public schema is dropped.
- *
- * Further, we have to handle the case where the public schema does
- * not exist at all.
- */
- if (dopt->outputClean)
- appendPQExpBuffer(query, " AND pip.objoid <> "
- "coalesce((select oid from pg_namespace "
- "where nspname = 'public'),0)");
-
appendPQExpBuffer(query, ") ");
destroyPQExpBuffer(acl_subquery);
--- 4448,4453 ----
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 74730bf..3e9b4d9 100644
*** a/src/bin/pg_dump/t/002_pg_dump.pl
--- b/src/bin/pg_dump/t/002_pg_dump.pl
*************** qr/^ALTER (?!EVENT TRIGGER|LARGE OBJECT|
*** 1548,1577 ****
all_runs => 1,
catch_all => 'COMMENT commands',
regexp => qr/^COMMENT ON EXTENSION plpgsql IS .*;/m,
! like => {
clean => 1,
clean_if_exists => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
exclude_test_table => 1,
exclude_test_table_data => 1,
no_blobs => 1,
- no_privs => 1,
no_owner => 1,
pg_dumpall_dbprivs => 1,
schema_only => 1,
section_pre_data => 1,
! with_oids => 1, },
! unlike => {
! binary_upgrade => 1,
! column_inserts => 1,
! data_only => 1,
! only_dump_test_schema => 1,
! only_dump_test_table => 1,
! role => 1,
! section_post_data => 1,
! test_schema_plus_blobs => 1, }, },
'COMMENT ON TABLE dump_test.test_table' => {
all_runs => 1,
--- 1548,1578 ----
all_runs => 1,
catch_all => 'COMMENT commands',
regexp => qr/^COMMENT ON EXTENSION plpgsql IS .*;/m,
! # this shouldn't ever get emitted anymore
! like => {},
! unlike => {
! binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
+ column_inserts => 1,
createdb => 1,
+ data_only => 1,
defaults => 1,
exclude_dump_test_schema => 1,
exclude_test_table => 1,
exclude_test_table_data => 1,
no_blobs => 1,
no_owner => 1,
+ no_privs => 1,
+ only_dump_test_schema => 1,
+ only_dump_test_table => 1,
pg_dumpall_dbprivs => 1,
+ role => 1,
schema_only => 1,
+ section_post_data => 1,
section_pre_data => 1,
! test_schema_plus_blobs => 1,
! with_oids => 1, }, },
'COMMENT ON TABLE dump_test.test_table' => {
all_runs => 1,
*************** qr/CREATE CAST \(timestamp with time zon
*** 2751,2783 ****
regexp => qr/^
\QCREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;\E
/xm,
! like => {
clean => 1,
clean_if_exists => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
exclude_test_table => 1,
exclude_test_table_data => 1,
no_blobs => 1,
- no_privs => 1,
no_owner => 1,
! pg_dumpall_dbprivs => 1,
! schema_only => 1,
! section_pre_data => 1,
! with_oids => 1, },
! unlike => {
! binary_upgrade => 1,
! column_inserts => 1,
! data_only => 1,
only_dump_test_schema => 1,
only_dump_test_table => 1,
pg_dumpall_globals => 1,
pg_dumpall_globals_clean => 1,
role => 1,
section_data => 1,
section_post_data => 1,
! test_schema_plus_blobs => 1, }, },
'CREATE AGGREGATE dump_test.newavg' => {
all_runs => 1,
--- 2752,2785 ----
regexp => qr/^
\QCREATE EXTENSION IF NOT EXISTS plpgsql WITH SCHEMA pg_catalog;\E
/xm,
! # this shouldn't ever get emitted anymore
! like => {},
! unlike => {
! binary_upgrade => 1,
clean => 1,
clean_if_exists => 1,
+ column_inserts => 1,
createdb => 1,
+ data_only => 1,
defaults => 1,
exclude_dump_test_schema => 1,
exclude_test_table => 1,
exclude_test_table_data => 1,
no_blobs => 1,
no_owner => 1,
! no_privs => 1,
only_dump_test_schema => 1,
only_dump_test_table => 1,
+ pg_dumpall_dbprivs => 1,
pg_dumpall_globals => 1,
pg_dumpall_globals_clean => 1,
role => 1,
+ schema_only => 1,
section_data => 1,
section_post_data => 1,
! section_pre_data => 1,
! test_schema_plus_blobs => 1,
! with_oids => 1, }, },
'CREATE AGGREGATE dump_test.newavg' => {
all_runs => 1,
*************** qr/CREATE TRANSFORM FOR integer LANGUAGE
*** 4565,4575 ****
all_runs => 1,
catch_all => 'CREATE ... commands',
regexp => qr/^CREATE SCHEMA public;/m,
! like => {
! clean => 1,
! clean_if_exists => 1, },
unlike => {
binary_upgrade => 1,
createdb => 1,
defaults => 1,
exclude_test_table => 1,
--- 4567,4578 ----
all_runs => 1,
catch_all => 'CREATE ... commands',
regexp => qr/^CREATE SCHEMA public;/m,
! # this shouldn't ever get emitted anymore
! like => {},
unlike => {
binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
createdb => 1,
defaults => 1,
exclude_test_table => 1,
*************** qr/CREATE TRANSFORM FOR integer LANGUAGE
*** 5395,5402 ****
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP SCHEMA public;/m,
! like => { clean => 1 },
unlike => {
clean_if_exists => 1,
pg_dumpall_globals_clean => 1, }, },
--- 5398,5407 ----
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP SCHEMA public;/m,
! # this shouldn't ever get emitted anymore
! like => {},
unlike => {
+ clean => 1,
clean_if_exists => 1,
pg_dumpall_globals_clean => 1, }, },
*************** qr/CREATE TRANSFORM FOR integer LANGUAGE
*** 5404,5420 ****
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP SCHEMA IF EXISTS public;/m,
! like => { clean_if_exists => 1 },
unlike => {
clean => 1,
pg_dumpall_globals_clean => 1, }, },
'DROP EXTENSION plpgsql' => {
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP EXTENSION plpgsql;/m,
! like => { clean => 1, },
unlike => {
clean_if_exists => 1,
pg_dumpall_globals_clean => 1, }, },
--- 5409,5429 ----
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP SCHEMA IF EXISTS public;/m,
! # this shouldn't ever get emitted anymore
! like => {},
unlike => {
clean => 1,
+ clean_if_exists => 1,
pg_dumpall_globals_clean => 1, }, },
'DROP EXTENSION plpgsql' => {
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP EXTENSION plpgsql;/m,
! # this shouldn't ever get emitted anymore
! like => {},
unlike => {
+ clean => 1,
clean_if_exists => 1,
pg_dumpall_globals_clean => 1, }, },
*************** qr/CREATE TRANSFORM FOR integer LANGUAGE
*** 5494,5502 ****
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP EXTENSION IF EXISTS plpgsql;/m,
! like => { clean_if_exists => 1, },
unlike => {
clean => 1,
pg_dumpall_globals_clean => 1, }, },
'DROP FUNCTION IF EXISTS dump_test.pltestlang_call_handler()' => {
--- 5503,5513 ----
all_runs => 1,
catch_all => 'DROP ... commands',
regexp => qr/^DROP EXTENSION IF EXISTS plpgsql;/m,
! # this shouldn't ever get emitted anymore
! like => {},
unlike => {
clean => 1,
+ clean_if_exists => 1,
pg_dumpall_globals_clean => 1, }, },
'DROP FUNCTION IF EXISTS dump_test.pltestlang_call_handler()' => {
*************** qr/^GRANT SELECT ON TABLE measurement_y2
*** 6264,6274 ****
\Q--\E\n\n
\QGRANT USAGE ON SCHEMA public TO PUBLIC;\E
/xm,
! like => {
! clean => 1,
! clean_if_exists => 1, },
unlike => {
binary_upgrade => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
--- 6275,6286 ----
\Q--\E\n\n
\QGRANT USAGE ON SCHEMA public TO PUBLIC;\E
/xm,
! # this shouldn't ever get emitted anymore
! like => {},
unlike => {
binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
*************** qr/^GRANT SELECT ON TABLE measurement_y2
*** 6537,6542 ****
--- 6549,6556 ----
/xm,
like => {
binary_upgrade => 1,
+ clean => 1,
+ clean_if_exists => 1,
createdb => 1,
defaults => 1,
exclude_dump_test_schema => 1,
*************** qr/^GRANT SELECT ON TABLE measurement_y2
*** 6549,6556 ****
section_pre_data => 1,
with_oids => 1, },
unlike => {
- clean => 1,
- clean_if_exists => 1,
only_dump_test_schema => 1,
only_dump_test_table => 1,
pg_dumpall_globals_clean => 1,
--- 6563,6568 ----
*************** qr/^GRANT SELECT ON TABLE measurement_y2
*** 6576,6593 ****
exclude_test_table_data => 1,
no_blobs => 1,
no_owner => 1,
pg_dumpall_dbprivs => 1,
schema_only => 1,
section_pre_data => 1,
with_oids => 1, },
unlike => {
- only_dump_test_schema => 1,
- only_dump_test_table => 1,
pg_dumpall_globals_clean => 1,
- role => 1,
section_data => 1,
! section_post_data => 1,
! test_schema_plus_blobs => 1, }, },
'REVOKE commands' => { # catch-all for REVOKE commands
all_runs => 0, # catch-all
--- 6588,6605 ----
exclude_test_table_data => 1,
no_blobs => 1,
no_owner => 1,
+ only_dump_test_schema => 1,
+ only_dump_test_table => 1,
pg_dumpall_dbprivs => 1,
+ role => 1,
schema_only => 1,
section_pre_data => 1,
+ test_schema_plus_blobs => 1,
with_oids => 1, },
unlike => {
pg_dumpall_globals_clean => 1,
section_data => 1,
! section_post_data => 1, }, },
'REVOKE commands' => { # catch-all for REVOKE commands
all_runs => 0, # catch-all
pgsql-hackers by date: