pgsql: Make pg_dumpall build with the right object files under MSVC.

Make pg_dumpall build with the right object files under MSVC.

This fixes a longstanding but up to now benign bug in the way pg_dumpall
was built. The bug was exposed by recent code adjustments. The Makefile
does not use $(OBJS) to build pg_dumpall, so this fix removes their source
files from the pg_dumpall object and adds in the one source file it
consequently needs.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/91572ee0a6dfeb62dda6c375f613d1b7fdfc1383

Modified Files
--------------
src/tools/msvc/Mkvcbuild.pm | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

Andrew Dunstan wrote:
> Make pg_dumpall build with the right object files under MSVC.
>
> This fixes a longstanding but up to now benign bug in the way pg_dumpall
> was built. The bug was exposed by recent code adjustments. The Makefile
> does not use $(OBJS) to build pg_dumpall, so this fix removes their source
> files from the pg_dumpall object and adds in the one source file it
> consequently needs.

In summary, for those watching, pg_dump and pg_restore used to share
OBJS, and with my new patch, dumpmem.c is now shared by those and
pg_dumpall. Seems the MSVC code previously could not handle that case,
which is fixed by this patch.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 11/28/2011 11:33 AM, Bruce Momjian wrote:
> Andrew Dunstan wrote:
>> Make pg_dumpall build with the right object files under MSVC.
>>
>> This fixes a longstanding but up to now benign bug in the way pg_dumpall
>> was built. The bug was exposed by recent code adjustments. The Makefile
>> does not use $(OBJS) to build pg_dumpall, so this fix removes their source
>> files from the pg_dumpall object and adds in the one source file it
>> consequently needs.
> In summary, for those watching, pg_dump and pg_restore used to share
> OBJS, and with my new patch, dumpmem.c is now shared by those and
> pg_dumpall. Seems the MSVC code previously could not handle that case,
> which is fixed by this patch.
>

Er, no. Only dumputils.c is shared with pg_dumpall. dumpmem.c is not
(see the Makefile).

The problem that arose is that pg_dumpall has its own (non-static)
versions of pg_malloc and pg_strdup, so we got duplicate symbol errors
from the newly declared dumpmem.c functions when we erroneously tried
linking it in on MSVC.

cheers

andrew

Excerpts from Andrew Dunstan's message of lun nov 28 14:40:24 -0300 2011:
>
> On 11/28/2011 11:33 AM, Bruce Momjian wrote:

> > In summary, for those watching, pg_dump and pg_restore used to share
> > OBJS, and with my new patch, dumpmem.c is now shared by those and
> > pg_dumpall. Seems the MSVC code previously could not handle that case,
> > which is fixed by this patch.
>
> Er, no. Only dumputils.c is shared with pg_dumpall. dumpmem.c is not
> (see the Makefile).
>
> The problem that arose is that pg_dumpall has its own (non-static)
> versions of pg_malloc and pg_strdup, so we got duplicate symbol errors
> from the newly declared dumpmem.c functions when we erroneously tried
> linking it in on MSVC.

I was wondering if it wouldn't make more sense to have pg_dumpall supply
its own version of exit_horribly to avoid separate pg_malloc and
pg_strdup ... but then those routines are so tiny that it hardly makes a
difference.

Another thing I wondered when seeing the original commit is the fact
that the old code passed the AH to exit_horribly in some places, whereas
the new one simply uses NULL.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
>
> Excerpts from Andrew Dunstan's message of lun nov 28 14:40:24 -0300 2011:
> >
> > On 11/28/2011 11:33 AM, Bruce Momjian wrote:
>
> > > In summary, for those watching, pg_dump and pg_restore used to share
> > > OBJS, and with my new patch, dumpmem.c is now shared by those and
> > > pg_dumpall. Seems the MSVC code previously could not handle that case,
> > > which is fixed by this patch.
> >
> > Er, no. Only dumputils.c is shared with pg_dumpall. dumpmem.c is not
> > (see the Makefile).
> >
> > The problem that arose is that pg_dumpall has its own (non-static)
> > versions of pg_malloc and pg_strdup, so we got duplicate symbol errors
> > from the newly declared dumpmem.c functions when we erroneously tried
> > linking it in on MSVC.
>
> I was wondering if it wouldn't make more sense to have pg_dumpall supply
> its own version of exit_horribly to avoid separate pg_malloc and
> pg_strdup ... but then those routines are so tiny that it hardly makes a
> difference.
>
> Another thing I wondered when seeing the original commit is the fact
> that the old code passed the AH to exit_horribly in some places, whereas
> the new one simply uses NULL.

Good point. Our old 9.1 code had:

common.c: exit_horribly(NULL, NULL, "cannot duplicate null pointer\n");
common.c: exit_horribly(NULL, NULL, "out of memory\n");
common.c: exit_horribly(NULL, NULL, "out of memory\n");
common.c: exit_horribly(NULL, NULL, "out of memory\n");
common.c: exit_horribly(NULL, NULL, "out of memory\n");
--> pg_backup_archiver.c: exit_horribly(AH, modulename, "out of memory\n");
pg_backup_archiver.c:exit_horribly(Archive *AH, const char *modulename, const char *fmt,...)
pg_dump_sort.c: exit_horribly(NULL, modulename, "out of memory\n");
pg_dump_sort.c: exit_horribly(NULL, modulename, "out of memory\n");
pg_dump_sort.c: exit_horribly(NULL, modulename, "out of memory\n");
pg_dump_sort.c: exit_horribly(NULL, modulename, "out of memory\n");
pg_dump_sort.c: exit_horribly(NULL, modulename, "invalid dumpId %d\n", j);
pg_dump_sort.c: exit_horribly(NULL, modulename, "invalid dependency %d\n", k);
pg_dump_sort.c: exit_horribly(NULL, modulename, "out of memory\n");
pg_dump_sort.c: exit_horribly(NULL, modulename, "could not identify dependency loop\n");

while our new code has:

dumpmem.c: exit_horribly(NULL, NULL, "cannot duplicate null pointer\n");
dumpmem.c: exit_horribly(NULL, NULL, "out of memory\n");
dumpmem.c: exit_horribly(NULL, NULL, "out of memory\n");
dumpmem.c: exit_horribly(NULL, NULL, _("out of memory\n"));
dumpmem.c: exit_horribly(NULL, NULL, _("out of memory\n"));
pg_backup_archiver.c:exit_horribly(Archive *AH, const char *modulename, const char *fmt,...)
pg_dump_sort.c: exit_horribly(NULL, modulename, "invalid dumpId %d\n", j);
pg_dump_sort.c: exit_horribly(NULL, modulename, "invalid dependency %d\n", k);
pg_dump_sort.c: exit_horribly(NULL, modulename, "could not identify dependency loop\n");

There is actually one case in the old code where we passed AH, and that
AH is only used for:

if (AH)
{
if (AH->public.verbose)
write_msg(NULL, "*** aborted because of error\n");
if (AH->connection)
PQfinish(AH->connection);
}

I am thinking we should just get rid of the whole AH passing.

I have always felt the pg_dump code is overly complex, and this is
confirming my suspicion.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> > I was wondering if it wouldn't make more sense to have pg_dumpall supply
> > its own version of exit_horribly to avoid separate pg_malloc and
> > pg_strdup ... but then those routines are so tiny that it hardly makes a
> > difference.
> >
> > Another thing I wondered when seeing the original commit is the fact
> > that the old code passed the AH to exit_horribly in some places, whereas
> > the new one simply uses NULL.
...
>
> I am thinking we should just get rid of the whole AH passing.
>
> I have always felt the pg_dump code is overly complex, and this is
> confirming my suspicion.

I have developed the attached patch which accomplishes this. I was also
able to move the write_msg function into dumputils.c (which is linked to
pg_dumpall), which allows pg_dumpall to use the new dumpmem.c functions,
and I removed its private ones.

FYI, I found write_msg() was a useless valist trampoline so I removed
the trampoline code and renamed _write_msg() to write_msg(). I also
modified the MSVC code.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:
> Bruce Momjian wrote:
> > > I was wondering if it wouldn't make more sense to have pg_dumpall supply
> > > its own version of exit_horribly to avoid separate pg_malloc and
> > > pg_strdup ... but then those routines are so tiny that it hardly makes a
> > > difference.
> > >
> > > Another thing I wondered when seeing the original commit is the fact
> > > that the old code passed the AH to exit_horribly in some places, whereas
> > > the new one simply uses NULL.
> ...
> >
> > I am thinking we should just get rid of the whole AH passing.
> >
> > I have always felt the pg_dump code is overly complex, and this is
> > confirming my suspicion.
>
> I have developed the attached patch which accomplishes this. I was also
> able to move the write_msg function into dumputils.c (which is linked to
> pg_dumpall), which allows pg_dumpall to use the new dumpmem.c functions,
> and I removed its private ones.
>
> FYI, I found write_msg() was a useless valist trampoline so I removed
> the trampoline code and renamed _write_msg() to write_msg(). I also
> modified the MSVC code.

Applied.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +