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.

On Fri, Aug 15, 2014 at 7:30 PM, Joachim Wieland <joe(at)mcknight(dot)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(at)gmx(dot)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.
>

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(at)gmx(dot)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

Joachim Wieland wrote:
> On Sat, Aug 30, 2014 at 11:12 PM, Peter Eisentraut <peter_e(at)gmx(dot)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(at)2ndquadrant(dot)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.

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

Joachim Wieland wrote:
> On Sat, Aug 30, 2014 at 11:12 PM, Peter Eisentraut <peter_e(at)gmx(dot)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

Here's the complete patch in case anyone is wondering.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

Fabrízio de Royes Mello wrote:
> On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com>
wrote:
>
> Fabrízio de Royes Mello wrote:
> > On Sat, Oct 11, 2014 at 2:56 PM, Alvaro Herrera <
alvherre(at)2ndquadrant(dot)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

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

I have pushed this, thanks.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services