Thread: pg_dump refactor patch to remove global variables
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. The patch introduces a DumpOptions struct, analogous to the existing RestoreOptions. This struct then holds the globals but also variables like dbname, hostname and so on that are currently declared in pg_dump's main(). Then it passes a pointer to this DumpOptions struct to all functions that need it. Motivation for this patch is to a) clean up the globals just because it's a sane thing to do, b) facilitate future refactoring and c) my own private objective is to introduce a parallel version of what you can currently do with "pg_dump | psql" in a later commitfest, where pg_dump dumps a database in parallel and restores it in parallel without saving any data on disk. For this, pg_dump needs to handle more than one database connection which isn't possible with the current globals and the variables that are declared in main(). One thing I haven't done yet and I'm not sure if it's a good idea to do, is getting rid of dumputils.c's quote_all_identifiers variable. This is used by fmtId() which is called in hundreds of places. I don't need it for my use case but for a proper cleanup of globals one could make the point that it needs refactoring, too. I didn't want to blow up the patch size unnecessarily without previous discussion however so I left it out for now. There's also more potential refactoring with respect to DumpOptions/RestoreOptions, they share common fields now which could be combined and for example a dump in the plain format in pg_dump is implemented as a restore operation (and goes through the restore code). This requires access to both dump and restore options. Here as well, we could do more but for this patch I tried to minimize the impact for now and kept it close to how it is implemented now.
Attachment
On Fri, Aug 15, 2014 at 7:30 PM, Joachim Wieland <joe@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
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
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.
Fixed and rebased patch attached. On Tue, Aug 26, 2014 at 11:40 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > 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. >
Attachment
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?
On Sat, Aug 30, 2014 at 11:12 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > The general idea of this patch is not disputed, I think. Ok, that's good news. > - 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. > - 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... > - What about binary_upgrade? I thought of the binary upgrade more as a different environment that the program is run in (as opposed to a traditional user switch) so I left it aside, but it turns out that it doesn't require that much more code to support it and definitely it enables more code refactoring if it's treated like the other flags, so the new patch also removes the global 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. That's partly true, there are two groups of variables that we could remove from the DumpOptions, one is the group of options that are only used in the huge main() of pg_dump.c and the second group is variables that go one function further down into setup_connection(). dumpencoding for example goes down into setup_connection(). So do use_role, no_synchronized_snapshots, serializable_deferrable and numWorkers. 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... Then there's another group of options that is used only in main() but is then used to populate the RestoreOptions struct. compressLevel is one of them. The thing about creating all those different groups is that it makes refactoring a bit harder. In the end we would group variables not by their meaning and purpose but by their usage pattern in the code. > - 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. Agreed, fixed. > - 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. I would have created the prototype in the respective header of the .c file that holds the function definition, but that's a bit fuzzy around pg_dump. I have now moved the DumpOptions over to pg_backup.h, because pg_backup_archiver.h includes pg_backup.h so that makes pg_backup.h the lower header. > - 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. > Also, looking at dumpOptionsFromRestoreOptions() and related code makes > me think that these should perhaps really be the same structure. Have > you investigated that? I have (as mentioned in my original email about this patch), but that would be a much larger change than what I had envisioned. New patch attached. Joachim
Attachment
Joachim Wieland wrote: > On Sat, Aug 30, 2014 at 11:12 PM, Peter Eisentraut <peter_e@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Sep 11, 2014 at 5:23 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > 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. I've reverted that as proposed, i.e. moved the extern to dumputils.h and removed the static in pg_dumpall.c. > I think I like the idea of ConnectionOpts as separate from DumpOptions, > TBH. Well, in the end it wasn't all that easy and my initial analysis wasn't correct. So what I did was that I kind of restored the original behavior wrt dumpencoding and use_role and pass them as arguments to setup_connection now as they were before. The only difference now is that the DumpOptions pointer is also passed in (to setup_connection), but this makes sense because it needs to check variables in there which used to be global variables before. In the new patch I've also taken out other variables from the DumpOptions struct and made them local in pg_dump.c's main(), partly proposed by Peter before, those are: numWorkers, prompt_password, compressLevel, plainText, archiveFormat, archiveMode. Revised patch attached, thanks for the review Alvaro & Peter.
Attachment
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Joachim Wieland wrote: > On Sat, Aug 30, 2014 at 11:12 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > > - 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. > > Agreed, fixed. > > > - 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. > > I would have created the prototype in the respective header of the .c > file that holds the function definition, but that's a bit fuzzy around > pg_dump. I have now moved the DumpOptions over to pg_backup.h, because > pg_backup_archiver.h includes pg_backup.h so that makes pg_backup.h > the lower header. I gave this a look and my conclusion is that the current situation doesn't make much sense -- supposedly, pg_backup.h is the public header and pg_dump.h is the private header; then how does it make sense that the former includes the latter? I think the reason is that the struct definitions are not well placed. In the attached patch, which applies on top of the rebase of Joachim's patch I submitted yesterday, I fix that by moving some structs (those that make sense for the public interface) to a new file, pg_dump_structs.h (better naming suggestions welcome). This is included everywhere as needed; it's included by pg_backup.h, in particular. With the new header in place I was able to remove most of cross-header forward struct declarations that were liberally used to work around what would otherwise be circularity in header dependencies. I think it makes much more sense this way. With this, headers are cleaner and they compile standalone. pg_backup.h does not include pg_dump.h anymore. DumpId and CatalogId, which were kinda public but defined in the private header (!?), were moved to pg_backup.h. This is necessary because the public functions use them. The one header not real clear to me is pg_backup_db.h. Right now it includes pg_backup_archiver.h, because some of its routines take an ArchiveHandle argument, but that seems pretty strange if looking at it from 10,000 miles. I think we could fix this by having the routines take Archive (the public one, defined in pg_dump_structs.h) and cast to ArchiveHandle internally. However, since pg_backup_db.h is not used much, I guess it doesn't really matter. There are some further (smaller) improvements that could be made if we really cared about it, but I'm satisfied with this. (I just realized after writing all this that I could achieve exactly the same by putting the contents of the new pg_dump_structs.h in pg_backup.h instead. If there are no strong opinions to the contrary, I'm inclined to do it that way instead, but I want to post it this way to gather opinions in case anyone cares.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Here's the complete patch in case anyone is wondering. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera <alvherre@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
$ 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
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
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Fabrízio de Royes Mello wrote: > On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera <alvherre@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sat, Oct 11, 2014 at 10:15 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Fabrízio de Royes Mello wrote:
> > On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera <alvherre@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
>
Thanks and sorry by the noise.
I'll see the changes.
Regards.
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
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
I have pushed this, thanks. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services