Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Make pg_dumpall build with the right object files under MSVC.

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

    Andrew Dunstan <andrew@dunslane.net> — 2011-11-28T01:20:09Z

    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(-)

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

    Bruce Momjian <bruce@momjian.us> — 2011-11-28T16:33:14Z

    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. +

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

    Andrew Dunstan <andrew@dunslane.net> — 2011-11-28T17:40:24Z

    
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

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

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-11-28T17:48:27Z

    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

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

    Bruce Momjian <bruce@momjian.us> — 2011-11-28T21:26:40Z

    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. +

  6. Allow pg_dumpall to use dumpmem.c functions, simplify exit code

    Bruce Momjian <bruce@momjian.us> — 2011-11-29T02:38:51Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

  7. Re: Allow pg_dumpall to use dumpmem.c functions, simplify exit code

    Bruce Momjian <bruce@momjian.us> — 2011-11-29T21:35:18Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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