Thread: 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@momjian.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@commandprompt.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +