Re: pg_dump refactor patch to remove global variables

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