On Fri, Jul 25, 2014 at 4:45 AM, Fabrízio de Royes Mello
<fabriziome...@gmail.com> wrote:
>
> Given this is a very small and simple patch I thought it's not necessary...
>
> Added to the next CommitFest.
I had a look at this patch, and here are a couple of comments:
1) Depending on how ArchiveEntry is called to register an object to
dump, namespace may be NULL, but it is not the case
namespace->dobj.name, so you could get the namespace name at the top
of the function that have their verbose output improved with something
like that:
const char *namespace = tbinfo->dobj.namespace ?
tbinfo->dobj.namespace->dobj.name : NULL;
And then simplify the message output as follows:
if (namespace)
write_msg("blah \"%s\".\"%s\" blah", namespace, classname);
else
write_msg("blah \"%s\" blah", classname);
You can as well safely remove the checks on namespace->dobj.name.
2) I don't think that this is correct:
- ahlog(AH, 1, "processing data
for table \"%s\"\n",
- te->tag);
+ ahlog(AH, 1, "processing data
for table \"%s\".\"%s\"\n",
+ AH->currSchema, te->tag);
There are some code paths where AH->currSchema is set to NULL, and I
think that you should use te->namespace instead.
3) Changing only this message is not enough. The following verbose
messages need to be changed too for consistency:
- pg_dump: creating $tag $object
- pg_dump: setting owner and privileges for [blah]
I have been pondering as well about doing similar modifications to the
error message paths, but it did not seem worth it as this patch is
aimed only for the verbose output. Btw, I have basically fixed those
issues while doing the review, and finished with the attached patch.
Fabrizio, is this new version fine for you?
Regards,
--
Michael
From e0809869655c9e22cce11955c7286cef8a42bf1d Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Wed, 20 Aug 2014 14:40:40 +0900
Subject: [PATCH] Improve verbose messages of pg_dump with namespace
Namespace is added to the verbose output when it is available, relation
and namespace names are put within quotes for clarity and consistency
with the other tools as well.
---
src/bin/pg_dump/pg_backup_archiver.c | 26 ++++++++---
src/bin/pg_dump/pg_dump.c | 85 ++++++++++++++++++++++++++++++------
2 files changed, 93 insertions(+), 18 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 3aebac8..07cc10e 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -546,8 +546,13 @@ RestoreArchive(Archive *AHX)
/* Both schema and data objects might now have ownership/ACLs */
if ((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0)
{
- ahlog(AH, 1, "setting owner and privileges for %s %s\n",
- te->desc, te->tag);
+ /* Show namespace if available */
+ if (te->namespace)
+ ahlog(AH, 1, "setting owner and privileges for %s \"%s\".\"%s\"\n",
+ te->desc, te->namespace, te->tag);
+ else
+ ahlog(AH, 1, "setting owner and privileges for %s \"%s\"\n",
+ te->desc, te->tag);
_printTocEntry(AH, te, ropt, false, true);
}
}
@@ -621,7 +626,13 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te,
if ((reqs & REQ_SCHEMA) != 0) /* We want the schema */
{
- ahlog(AH, 1, "creating %s %s\n", te->desc, te->tag);
+ /* Show namespace if available */
+ if (te->namespace)
+ ahlog(AH, 1, "creating %s \"%s\".\"%s\"\n",
+ te->desc, te->namespace, te->tag);
+ else
+ ahlog(AH, 1, "creating %s \"%s\"\n", te->desc, te->tag);
+
_printTocEntry(AH, te, ropt, false, false);
defnDumped = true;
@@ -713,8 +724,13 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te,
_becomeOwner(AH, te);
_selectOutputSchema(AH, te->namespace);
- ahlog(AH, 1, "processing data for table \"%s\"\n",
- te->tag);
+ /* Show namespace if available */
+ if (te->namespace)
+ ahlog(AH, 1, "processing data for table \"%s\".\"%s\"\n",
+ te->namespace, te->tag);
+ else
+ ahlog(AH, 1, "processing data for table \"%s\"\n",
+ te->tag);
/*
* In parallel restore, if we created the table earlier in
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5c0f95f..dd7eef9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1383,6 +1383,8 @@ dumpTableData_copy(Archive *fout, void *dcontext)
{
TableDataInfo *tdinfo = (TableDataInfo *) dcontext;
TableInfo *tbinfo = tdinfo->tdtable;
+ const char *namespace = tbinfo->dobj.namespace ?
+ tbinfo->dobj.namespace->dobj.name : NULL;
const char *classname = tbinfo->dobj.name;
const bool hasoids = tbinfo->hasoids;
const bool oids = tdinfo->oids;
@@ -1400,7 +1402,16 @@ dumpTableData_copy(Archive *fout, void *dcontext)
const char *column_list;
if (g_verbose)
- write_msg(NULL, "dumping contents of table %s\n", classname);
+ {
+ /* Print namespace information if available */
+ if (namespace)
+ write_msg(NULL, "dumping contents of table \"%s\".\"%s\"\n",
+ namespace,
+ classname);
+ else
+ write_msg(NULL, "dumping contents of table \"%s\"\n",
+ classname);
+ }
/*
* Make sure we are in proper schema. We will qualify the table name
@@ -5019,8 +5030,16 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
continue;
if (g_verbose)
- write_msg(NULL, "reading indexes for table \"%s\"\n",
- tbinfo->dobj.name);
+ {
+ /* Print namespace information if available */
+ if (tbinfo->dobj.namespace != NULL)
+ write_msg(NULL, "reading indexes for table \"%s\".\"%s\"\n",
+ tbinfo->dobj.namespace->dobj.name,
+ tbinfo->dobj.name);
+ else
+ write_msg(NULL, "reading indexes for table \"%s\"\n",
+ tbinfo->dobj.name);
+ }
/* Make sure we are in proper schema so indexdef is right */
selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
@@ -5385,8 +5404,16 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
continue;
if (g_verbose)
- write_msg(NULL, "reading foreign key constraints for table \"%s\"\n",
- tbinfo->dobj.name);
+ {
+ /* Print namespace information if available */
+ if (tbinfo->dobj.namespace != NULL)
+ write_msg(NULL, "reading foreign key constraints for table \"%s\".\"%s\"\n",
+ tbinfo->dobj.namespace->dobj.name,
+ tbinfo->dobj.name);
+ else
+ write_msg(NULL, "reading foreign key constraints for table \"%s\"\n",
+ tbinfo->dobj.name);
+ }
/*
* select table schema to ensure constraint expr is qualified if
@@ -5723,8 +5750,16 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables)
continue;
if (g_verbose)
- write_msg(NULL, "reading triggers for table \"%s\"\n",
- tbinfo->dobj.name);
+ {
+ /* Print namespace information if available */
+ if (tbinfo->dobj.namespace != NULL)
+ write_msg(NULL, "reading triggers for table \"%s\".\"%s\"\n",
+ tbinfo->dobj.namespace->dobj.name,
+ tbinfo->dobj.name);
+ else
+ write_msg(NULL, "reading triggers for table \"%s\"\n",
+ tbinfo->dobj.name);
+ }
/*
* select table schema to ensure regproc name is qualified if needed
@@ -6336,8 +6371,16 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* the output of an indexscan on pg_attribute_relid_attnum_index.
*/
if (g_verbose)
- write_msg(NULL, "finding the columns and types of table \"%s\"\n",
- tbinfo->dobj.name);
+ {
+ /* Print namespace information if available */
+ if (tbinfo->dobj.namespace != NULL)
+ write_msg(NULL, "finding the columns and types of table \"%s\".\"%s\"\n",
+ tbinfo->dobj.namespace->dobj.name,
+ tbinfo->dobj.name);
+ else
+ write_msg(NULL, "finding the columns and types of table \"%s\"\n",
+ tbinfo->dobj.name);
+ }
resetPQExpBuffer(q);
@@ -6548,8 +6591,16 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
int numDefaults;
if (g_verbose)
- write_msg(NULL, "finding default expressions of table \"%s\"\n",
- tbinfo->dobj.name);
+ {
+ /* Print namespace information if available */
+ if (tbinfo->dobj.namespace != NULL)
+ write_msg(NULL, "finding default expressions of table \"%s\".\"%s\"\n",
+ tbinfo->dobj.namespace->dobj.name,
+ tbinfo->dobj.name);
+ else
+ write_msg(NULL, "finding default expressions of table \"%s\"\n",
+ tbinfo->dobj.name);
+ }
resetPQExpBuffer(q);
if (fout->remoteVersion >= 70300)
@@ -6672,8 +6723,16 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
int numConstrs;
if (g_verbose)
- write_msg(NULL, "finding check constraints for table \"%s\"\n",
- tbinfo->dobj.name);
+ {
+ /* Print namespace information if available */
+ if (tbinfo->dobj.namespace != NULL)
+ write_msg(NULL, "finding check constraints for table \"%s\".\"%s\"\n",
+ tbinfo->dobj.namespace->dobj.name,
+ tbinfo->dobj.name);
+ else
+ write_msg(NULL, "finding check constraints for table \"%s\"\n",
+ tbinfo->dobj.name);
+ }
resetPQExpBuffer(q);
if (fout->remoteVersion >= 90200)
--
2.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
