Re: [HACKERS] pg_dump refactor patch to remove global variables
I have pushed this, thanks. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump refactor patch to remove global variables
Alvaro Herrera wrote:
Here's the complete patch in case anyone is wondering.
I found out that with just a little bit of extra header hacking, the
whole thing ends up cleaner -- for instance, with the attached patch,
pg_backup_archiver.h is no longer needed by pg_backup_db.h. I also
tweaked things so that no .h file includes postgres_fe.h, but instead
every .c file includes it before including anything else, as is already
customary for postgres.h in backend code.
The main changes herein are:
* some routines in pg_backup_db.c/h had an argument of type
ArchiveHandle. I made them take Archive instead, and cast internally.
This is already done for some other routines.
* also in pg_backup_db.c/h, EndDBCopyMode() had an argument of type
TocEntry, and then it only uses te-tag for an error message. If I
instead pass the te-tag I can remove the TocEntry, and there is no more
need for pg_backup_archiver.h in pg_backup_db.h.
* I moved exit_horribly() from parallel.h to pg_backup_utils.h, where
prototypes for other exit routines such as exit_nicely() are already
located. (The implementation of exit_horribly() is in parallel.c, but
the prototype looks misplaced in parallel.h.)
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 17e9574..8bfc604 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -13,8 +13,11 @@
*
*-
*/
+#include postgres_fe.h
+
#include pg_backup_archiver.h
#include pg_backup_utils.h
+#include pg_dump.h
#include ctype.h
diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c
index 2e2a447..2c568cd 100644
--- a/src/bin/pg_dump/compress_io.c
+++ b/src/bin/pg_dump/compress_io.c
@@ -51,10 +51,11 @@
*
*-
*/
+#include postgres_fe.h
#include compress_io.h
-#include pg_backup_utils.h
#include parallel.h
+#include pg_backup_utils.h
/*--
* Compressor API
diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index 713c78b..5a21450 100644
--- a/src/bin/pg_dump/compress_io.h
+++ b/src/bin/pg_dump/compress_io.h
@@ -15,7 +15,6 @@
#ifndef __COMPRESS_IO__
#define __COMPRESS_IO__
-#include postgres_fe.h
#include pg_backup_archiver.h
/* Initial buffer sizes used in zlib compression. */
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index 06763ed..688e9ca 100644
--- a/src/bin/pg_dump/dumputils.h
+++ b/src/bin/pg_dump/dumputils.h
@@ -12,7 +12,6 @@
*
*-
*/
-
#ifndef DUMPUTILS_H
#define DUMPUTILS_H
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index ceed115..3b8d584 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -18,8 +18,8 @@
#include postgres_fe.h
-#include pg_backup_utils.h
#include parallel.h
+#include pg_backup_utils.h
#ifndef WIN32
#include sys/types.h
diff --git a/src/bin/pg_dump/parallel.h b/src/bin/pg_dump/parallel.h
index 744c76b..dd3546f 100644
--- a/src/bin/pg_dump/parallel.h
+++ b/src/bin/pg_dump/parallel.h
@@ -86,8 +86,4 @@ extern void ParallelBackupEnd(ArchiveHandle *AH, ParallelState *pstate);
extern void checkAborting(ArchiveHandle *AH);
-extern void
-exit_horribly(const char *modulename, const char *fmt,...)
-__attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3), noreturn));
-
#endif /* PG_DUMP_PARALLEL_H */
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 37fdd8c..c2ebcd4 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -23,10 +23,7 @@
#ifndef PG_BACKUP_H
#define PG_BACKUP_H
-#include postgres_fe.h
-
#include dumputils.h
-
#include libpq-fe.h
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 71ac6e5..95cb6e8 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -19,10 +19,12 @@
*
*-
*/
+#include postgres_fe.h
+#include parallel.h
+#include pg_backup_archiver.h
#include pg_backup_db.h
#include pg_backup_utils.h
-#include parallel.h
#include ctype.h
#include fcntl.h
@@ -424,7 +426,7 @@ RestoreArchive(Archive *AHX)
if (ropt-single_txn)
{
if (AH-connection)
- StartTransaction(AH);
+ StartTransaction(AHX);
else
ahprintf(AH, BEGIN;\n\n);
}
@@ -642,7 +644,7 @@ RestoreArchive(Archive *AHX)
if (ropt-single_txn)
{
if (AH-connection)
- CommitTransaction(AH);
+ CommitTransaction(AHX);
else
ahprintf(AH, COMMIT;\n\n);
}
@@ -823,7 +825,7 @@ restore_toc_entry(ArchiveHandle *AH, TocEntry *te,
* Parallel restore is always talking directly to a
* server, so no need
Re: [HACKERS] pg_dump refactor patch to remove global variables
Here's the complete patch in case anyone is wondering.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 2f855cf..17e9574 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -64,7 +64,7 @@ static DumpableObject **nspinfoindex;
static void flagInhTables(TableInfo *tbinfo, int numTables,
InhInfo *inhinfo, int numInherits);
-static void flagInhAttrs(TableInfo *tblinfo, int numTables);
+static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
static DumpableObject **buildIndexArray(void *objArray, int numObjs,
Size objSize);
static int DOCatalogIdCompare(const void *p1, const void *p2);
@@ -78,7 +78,7 @@ static int strInArray(const char *pattern, char **arr, int arr_size);
* Collect information about all potentially dumpable objects
*/
TableInfo *
-getSchemaData(Archive *fout, int *numTablesPtr)
+getSchemaData(Archive *fout, DumpOptions *dopt, int *numTablesPtr)
{
ExtensionInfo *extinfo;
InhInfo*inhinfo;
@@ -114,7 +114,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
*/
if (g_verbose)
write_msg(NULL, reading user-defined tables\n);
- tblinfo = getTables(fout, numTables);
+ tblinfo = getTables(fout, dopt, numTables);
tblinfoindex = buildIndexArray(tblinfo, numTables, sizeof(TableInfo));
/* Do this after we've built tblinfoindex */
@@ -122,11 +122,11 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading extensions\n);
- extinfo = getExtensions(fout, numExtensions);
+ extinfo = getExtensions(fout, dopt, numExtensions);
if (g_verbose)
write_msg(NULL, reading user-defined functions\n);
- funinfo = getFuncs(fout, numFuncs);
+ funinfo = getFuncs(fout, dopt, numFuncs);
funinfoindex = buildIndexArray(funinfo, numFuncs, sizeof(FuncInfo));
/* this must be after getTables and getFuncs */
@@ -142,7 +142,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading user-defined aggregate functions\n);
- getAggregates(fout, numAggregates);
+ getAggregates(fout, dopt, numAggregates);
if (g_verbose)
write_msg(NULL, reading user-defined operators\n);
@@ -183,7 +183,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading default privileges\n);
- getDefaultACLs(fout, numDefaultACLs);
+ getDefaultACLs(fout, dopt, numDefaultACLs);
if (g_verbose)
write_msg(NULL, reading user-defined collations\n);
@@ -213,7 +213,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
*/
if (g_verbose)
write_msg(NULL, finding extension members\n);
- getExtensionMembership(fout, extinfo, numExtensions);
+ getExtensionMembership(fout, dopt, extinfo, numExtensions);
/* Link tables to parents, mark parents of target tables interesting */
if (g_verbose)
@@ -222,11 +222,11 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading column info for interesting tables\n);
- getTableAttrs(fout, tblinfo, numTables);
+ getTableAttrs(fout, dopt, tblinfo, numTables);
if (g_verbose)
write_msg(NULL, flagging inherited columns in subtables\n);
- flagInhAttrs(tblinfo, numTables);
+ flagInhAttrs(dopt, tblinfo, numTables);
if (g_verbose)
write_msg(NULL, reading indexes\n);
@@ -307,7 +307,7 @@ flagInhTables(TableInfo *tblinfo, int numTables,
* modifies tblinfo
*/
static void
-flagInhAttrs(TableInfo *tblinfo, int numTables)
+flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
{
int i,
j,
@@ -384,7 +384,7 @@ flagInhAttrs(TableInfo *tblinfo, int numTables)
attrDef-adef_expr = pg_strdup(NULL);
/* Will column be dumped explicitly? */
-if (shouldPrintColumn(tbinfo, j))
+if (shouldPrintColumn(dopt, tbinfo, j))
{
attrDef-separate = false;
/* No dependency needed: NULL cannot have dependencies */
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index b387aa1..06763ed 100644
--- a/src/bin/pg_dump/dumputils.h
+++ b/src/bin/pg_dump/dumputils.h
@@ -19,6 +19,23 @@
#include libpq-fe.h
#include pqexpbuffer.h
+/*
+ * Data structures for simple lists of OIDs and strings. The support for
+ * these is very primitive compared to the backend's List facilities, but
+ * it's all we need in pg_dump.
+ */
+typedef struct SimpleOidListCell
+{
+ struct SimpleOidListCell *next;
+ Oid val;
+} SimpleOidListCell;
+
+typedef struct SimpleOidList
+{
+ SimpleOidListCell *head;
+ SimpleOidListCell *tail;
+} SimpleOidList;
+
typedef struct SimpleStringListCell
{
struct SimpleStringListCell *next;
@@ -31,6 +48,7 @@ typedef struct SimpleStringList
SimpleStringListCell *tail;
} SimpleStringList;
+#define atooid(x) ((Oid) strtoul((x), NULL, 10))
extern int quote_all_identifiers;
extern PQExpBuffer (*getLocalPQExpBuffer) (void);
diff --git
Re: [HACKERS] pg_dump refactor patch to remove global variables
On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's the complete patch in case anyone is wondering. Hi, Your patch doesn't apply to master: $ git apply ~/Downloads/pg_dump_refactor_globals.6.diff /home/fabrizio/Downloads/pg_dump_refactor_globals.6.diff:10: trailing whitespace. static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables); /home/fabrizio/Downloads/pg_dump_refactor_globals.6.diff:19: trailing whitespace. getSchemaData(Archive *fout, DumpOptions *dopt, int *numTablesPtr) /home/fabrizio/Downloads/pg_dump_refactor_globals.6.diff:28: trailing whitespace. tblinfo = getTables(fout, dopt, numTables); /home/fabrizio/Downloads/pg_dump_refactor_globals.6.diff:37: trailing whitespace. extinfo = getExtensions(fout, dopt, numExtensions); /home/fabrizio/Downloads/pg_dump_refactor_globals.6.diff:42: trailing whitespace. funinfo = getFuncs(fout, dopt, numFuncs); error: patch failed: src/bin/pg_dump/common.c:64 error: src/bin/pg_dump/common.c: patch does not apply error: patch failed: src/bin/pg_dump/dumputils.h:19 error: src/bin/pg_dump/dumputils.h: patch does not apply error: patch failed: src/bin/pg_dump/parallel.c:89 error: src/bin/pg_dump/parallel.c: patch does not apply error: patch failed: src/bin/pg_dump/parallel.h:19 error: src/bin/pg_dump/parallel.h: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup.h:25 error: src/bin/pg_dump/pg_backup.h: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_archiver.c:107 error: src/bin/pg_dump/pg_backup_archiver.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_archiver.h:29 error: src/bin/pg_dump/pg_backup_archiver.h: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_custom.c:41 error: src/bin/pg_dump/pg_backup_custom.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_db.c:217 error: src/bin/pg_dump/pg_backup_db.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_db.h:16 error: src/bin/pg_dump/pg_backup_db.h: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_directory.c:71 error: src/bin/pg_dump/pg_backup_directory.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_null.c:35 error: src/bin/pg_dump/pg_backup_null.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_backup_tar.c:48 error: src/bin/pg_dump/pg_backup_tar.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_dump.c:81 error: src/bin/pg_dump/pg_dump.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_dump.h:16 error: src/bin/pg_dump/pg_dump.h: patch does not apply error: patch failed: src/bin/pg_dump/pg_dumpall.c:57 error: src/bin/pg_dump/pg_dumpall.c: patch does not apply error: patch failed: src/bin/pg_dump/pg_restore.c:423 error: src/bin/pg_dump/pg_restore.c: patch does not apply Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] pg_dump refactor patch to remove global variables
Fabrízio de Royes Mello wrote: On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's the complete patch in case anyone is wondering. Hi, Your patch doesn't apply to master: I think your email system mangled it. I can download it from archives and apply it just fine: $ wget http://www.postgresql.org/message-id/attachment/35216/pg_dump_refactor_globals.6.diff -O - | patch -p1 --2014-10-11 20:44:52-- http://www.postgresql.org/message-id/attachment/35216/pg_dump_refactor_globals.6.diff Resolviendo www.postgresql.org (www.postgresql.org)... 87.238.57.232, 174.143.35.230, 217.196.149.50, ... Conectando con www.postgresql.org (www.postgresql.org)[87.238.57.232]:80... conectado. Petición HTTP enviada, esperando respuesta... 200 OK Longitud: no especificado [text/x-diff] Grabando a: “STDOUT” [ = ] 140.127 105K/s en 1,3s 2014-10-11 20:44:55 (105 KB/s) - escritos a stdout [140127] patching file src/bin/pg_dump/common.c patching file src/bin/pg_dump/dumputils.h patching file src/bin/pg_dump/parallel.c patching file src/bin/pg_dump/parallel.h patching file src/bin/pg_dump/pg_backup.h patching file src/bin/pg_dump/pg_backup_archiver.c patching file src/bin/pg_dump/pg_backup_archiver.h patching file src/bin/pg_dump/pg_backup_custom.c patching file src/bin/pg_dump/pg_backup_db.c patching file src/bin/pg_dump/pg_backup_db.h patching file src/bin/pg_dump/pg_backup_directory.c patching file src/bin/pg_dump/pg_backup_null.c patching file src/bin/pg_dump/pg_backup_tar.c patching file src/bin/pg_dump/pg_dump.c patching file src/bin/pg_dump/pg_dump.h patching file src/bin/pg_dump/pg_dumpall.c patching file src/bin/pg_dump/pg_restore.c -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump refactor patch to remove global variables
On Sat, Oct 11, 2014 at 10:15 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fabrízio de Royes Mello wrote: On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's the complete patch in case anyone is wondering. Hi, Your patch doesn't apply to master: I think your email system mangled it. I can download it from archives and apply it just fine: $ wget http://www.postgresql.org/message-id/attachment/35216/pg_dump_refactor_globals.6.diff -O - | patch -p1 --2014-10-11 20:44:52-- http://www.postgresql.org/message-id/attachment/35216/pg_dump_refactor_globals.6.diff Resolviendo www.postgresql.org (www.postgresql.org)... 87.238.57.232, 174.143.35.230, 217.196.149.50, ... Conectando con www.postgresql.org (www.postgresql.org)[87.238.57.232]:80... conectado. Petición HTTP enviada, esperando respuesta... 200 OK Longitud: no especificado [text/x-diff] Grabando a: “STDOUT” [ = ] 140.127 105K/s en 1,3s 2014-10-11 20:44:55 (105 KB/s) - escritos a stdout [140127] patching file src/bin/pg_dump/common.c patching file src/bin/pg_dump/dumputils.h patching file src/bin/pg_dump/parallel.c patching file src/bin/pg_dump/parallel.h patching file src/bin/pg_dump/pg_backup.h patching file src/bin/pg_dump/pg_backup_archiver.c patching file src/bin/pg_dump/pg_backup_archiver.h patching file src/bin/pg_dump/pg_backup_custom.c patching file src/bin/pg_dump/pg_backup_db.c patching file src/bin/pg_dump/pg_backup_db.h patching file src/bin/pg_dump/pg_backup_directory.c patching file src/bin/pg_dump/pg_backup_null.c patching file src/bin/pg_dump/pg_backup_tar.c patching file src/bin/pg_dump/pg_dump.c patching file src/bin/pg_dump/pg_dump.h patching file src/bin/pg_dump/pg_dumpall.c patching file src/bin/pg_dump/pg_restore.c Yeah... my gmail mangled the attached file. Thanks and sorry by the noise. I'll see the changes. Regards. -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] pg_dump refactor patch to remove global variables
Here's a rebased patch for this (I also pgindented it).
One thing I find a bit odd is the fact that we copy
RestoreOptions-superuser into DumpOptions-outputSuperuser (a char *
pointer) without pstrdup or similar. We're already doing the inverse
elsewhere, and the uses of the routine where this is done are pretty
limited, so it seems harmless. Still, it's pretty weird. (Really,
the whole dumpOptionsFromRestoreOptions() business is odd.)
I'm not real happy with the forward struct declare in pg_backup.h, but I
think this is just general messiness in this code structure and not this
patch' fault.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 2f855cf..17e9574 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -64,7 +64,7 @@ static DumpableObject **nspinfoindex;
static void flagInhTables(TableInfo *tbinfo, int numTables,
InhInfo *inhinfo, int numInherits);
-static void flagInhAttrs(TableInfo *tblinfo, int numTables);
+static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
static DumpableObject **buildIndexArray(void *objArray, int numObjs,
Size objSize);
static int DOCatalogIdCompare(const void *p1, const void *p2);
@@ -78,7 +78,7 @@ static int strInArray(const char *pattern, char **arr, int arr_size);
* Collect information about all potentially dumpable objects
*/
TableInfo *
-getSchemaData(Archive *fout, int *numTablesPtr)
+getSchemaData(Archive *fout, DumpOptions *dopt, int *numTablesPtr)
{
ExtensionInfo *extinfo;
InhInfo*inhinfo;
@@ -114,7 +114,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
*/
if (g_verbose)
write_msg(NULL, reading user-defined tables\n);
- tblinfo = getTables(fout, numTables);
+ tblinfo = getTables(fout, dopt, numTables);
tblinfoindex = buildIndexArray(tblinfo, numTables, sizeof(TableInfo));
/* Do this after we've built tblinfoindex */
@@ -122,11 +122,11 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading extensions\n);
- extinfo = getExtensions(fout, numExtensions);
+ extinfo = getExtensions(fout, dopt, numExtensions);
if (g_verbose)
write_msg(NULL, reading user-defined functions\n);
- funinfo = getFuncs(fout, numFuncs);
+ funinfo = getFuncs(fout, dopt, numFuncs);
funinfoindex = buildIndexArray(funinfo, numFuncs, sizeof(FuncInfo));
/* this must be after getTables and getFuncs */
@@ -142,7 +142,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading user-defined aggregate functions\n);
- getAggregates(fout, numAggregates);
+ getAggregates(fout, dopt, numAggregates);
if (g_verbose)
write_msg(NULL, reading user-defined operators\n);
@@ -183,7 +183,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading default privileges\n);
- getDefaultACLs(fout, numDefaultACLs);
+ getDefaultACLs(fout, dopt, numDefaultACLs);
if (g_verbose)
write_msg(NULL, reading user-defined collations\n);
@@ -213,7 +213,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
*/
if (g_verbose)
write_msg(NULL, finding extension members\n);
- getExtensionMembership(fout, extinfo, numExtensions);
+ getExtensionMembership(fout, dopt, extinfo, numExtensions);
/* Link tables to parents, mark parents of target tables interesting */
if (g_verbose)
@@ -222,11 +222,11 @@ getSchemaData(Archive *fout, int *numTablesPtr)
if (g_verbose)
write_msg(NULL, reading column info for interesting tables\n);
- getTableAttrs(fout, tblinfo, numTables);
+ getTableAttrs(fout, dopt, tblinfo, numTables);
if (g_verbose)
write_msg(NULL, flagging inherited columns in subtables\n);
- flagInhAttrs(tblinfo, numTables);
+ flagInhAttrs(dopt, tblinfo, numTables);
if (g_verbose)
write_msg(NULL, reading indexes\n);
@@ -307,7 +307,7 @@ flagInhTables(TableInfo *tblinfo, int numTables,
* modifies tblinfo
*/
static void
-flagInhAttrs(TableInfo *tblinfo, int numTables)
+flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
{
int i,
j,
@@ -384,7 +384,7 @@ flagInhAttrs(TableInfo *tblinfo, int numTables)
attrDef-adef_expr = pg_strdup(NULL);
/* Will column be dumped explicitly? */
-if (shouldPrintColumn(tbinfo, j))
+if (shouldPrintColumn(dopt, tbinfo, j))
{
attrDef-separate = false;
/* No dependency needed: NULL cannot have dependencies */
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index e50dd8b..ceed115 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -89,11 +89,12 @@ static void WaitForTerminatingWorkers(ParallelState *pstate);
static void sigTermHandler(int signum);
#endif
static void SetupWorker(ArchiveHandle *AH, int pipefd[2], int worker,
+ DumpOptions *dopt,
RestoreOptions *ropt);
static bool
Re: [HACKERS] pg_dump refactor patch to remove global variables
Joachim Wieland wrote: On Sat, Aug 30, 2014 at 11:12 PM, Peter Eisentraut pete...@gmx.net wrote: - Why is quote_all_identifiers left behind as a global variable? As I said, it's really used everywhere. I'm not opposed to passing it around (which would be straightforward) but it would really blow up the patch size and it would be a rather mechanical patch. I'd rather do that as a separate patch, otherwise all other changes would get lost in the noise. I don't understand this bit. You have struct _dumpOptions containing a quote_all_identifiers, which seems to be unused. There's also a static int quote_all_identifiers in pg_dumpall.c, which I assume hides the non-static one in dumputils. And we also have an extern in pg_dump.c, which seems misplaced. There was an extern in dumputils.h but your patch removes it. Oh, after reading some code, I realize that those two variables are actually separate and do different things: pg_dumpall has one that can be set by passing --quote-all-identifiers; if set, it activates passing --quote-all-identifiers to pg_dump. pg_dump.c misleadingly has an extern which is enabled by its --quote-all-identifiers switch, and the effect is to execute SET quote_all_identifiers=true to the server. And finally we have a quote_all_identifiers in dumputils and has a quoting effect on the fmtId() function. So the way this works is that pg_dumpall doesn't use the one in dumputils; and pg_dump.c sets the one in dumputils.c and affects both the SET command and fmtId(). In other words, unless I miss something, pg_dumpall would never change the fmtId() behavior, which would be pretty broken IMHO. I find this rather odd. Can we find a more sensible arrangement? At least let's move the extern to dumputils.h, remove it from pg_dump.c. Not totally sure about the static in pg_dumpall.c -- note that it does call fmtId(), so we might be doing the wrong thing there; maybe we need to remove the static from there as well, and let --quote-all-identifiers set the one in dumputils. Is there a bug here that should be backpatched? I checked 9.3 and there is no static in pg_dumpall so it should behave sensibly AFAICT. - Shouldn't pg_dumpall also use DumpOptions? It could, it would mostly be a cosmetic change, since pg_dumpall is only a wrapper that invokes the pg_dump command. pg_dumpall's argument handling is isolated from the rest, so it could use DumpOptions or not... Probably no point, yeah. However if dumputils.c starts using DumpOptions, it might need to go in that direction as well. Not sure. Previously dumpencoding and use_role were passed as additional arguments to setup_connection(), being NULL when there was no change to the encoding (which I found quite ugly). I would say we should treat all variables of that group the same, so either all of them become local variables in main() and are passed as parameters to setup_connection() or we just pass the DumpOptions struct and put them in there (as is done in my patch now)... Or we create a new struct ConnectionOpts or so... I think I like the idea of ConnectionOpts as separate from DumpOptions, TBH. - In dumpOptionsFromRestoreOptions() you are building the return value in a local variable that is not zeroed. You should probably use NewDumpOptions() there. The idea was to avoid malloc()ing and free()ing and to instead just create those dumpOptions on the stack, because they're only used for a short time and a small chunk of data, but I assume it's more consistent to do it as you propose with NewDumpOptions(). So this is fixed in the new patch. I think putting stuff in the stack is fine, as long as you don't depend on it being initialized to all zeroes, because that's not guaranteed. You could memset() the struct in the stack also, but really I think it's simpler to just allocate it. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump refactor patch to remove global variables
The general idea of this patch is not disputed, I think. Some concerns about the details: - Why is quote_all_identifiers left behind as a global variable? - Shouldn't pg_dumpall also use DumpOptions? - What about binary_upgrade? - I think some of the variables in pg_dump's main() don't need to be moved into DumpOptions. For example, compressLevel is only looked at once in main() and then forgotten. We don't need to pass that around everywhere. The same goes for dumpencoding and possibly others. - The forward declaration of struct _dumpOptions in pg_backup.h seems kind of useless. You could move some things around so that that's not necessary. - NewDumpOptions() and NewRestoreOptions() are both in pg_backup_archiver.c, but NewRestoreOptions() is in pg_backup.h whereas NewDumpOptions() is being put into pg_backup_archiver.h. None of that makes too much sense, but it could be made more consistent. - In dumpOptionsFromRestoreOptions() you are building the return value in a local variable that is not zeroed. You should probably use NewDumpOptions() there. Also, looking at dumpOptionsFromRestoreOptions() and related code makes me think that these should perhaps really be the same structure. Have you investigated that? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump refactor patch to remove global variables
On 8/15/14 7:30 PM, Joachim Wieland wrote: Attached is a patch that doesn't add any new functionality or features, all it does is get rid of the global variables that pg_dump.c is full of. I'm getting a compiler error: In file included from pg_dump.c:60: In file included from ./pg_backup_archiver.h:32: ./pg_backup.h:212:3: error: redefinition of typedef 'DumpOptions' is a C11 feature [-Werror,-Wtypedef-redefinition] } DumpOptions; ^ ./pg_dump.h:507:29: note: previous definition is here typedef struct _dumpOptions DumpOptions; ^ 1 error generated. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump refactor patch to remove global variables
On 08/19/2014 01:40 AM, Robert Haas wrote: Attached is a patch that doesn't add any new functionality or features, all it does is get rid of the global variables that pg_dump.c is full of. I think this is an excellent idea. It's also one small step toward library-ifying pg_dump. Huge +1. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump refactor patch to remove global variables
On Fri, Aug 15, 2014 at 7:30 PM, Joachim Wieland j...@mcknight.de wrote: Attached is a patch that doesn't add any new functionality or features, all it does is get rid of the global variables that pg_dump.c is full of. I think this is an excellent idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
