Re: [COMMITTERS] pgsql: Modify pg_dump to use error-free memory allocation macros. This

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [COMMITTERS] pgsql: Modify pg_dump to use error-free memory allocation macros. This
Date: 2011-11-26 15:58:39
Message-ID: 12541.1322323119@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Bruce Momjian wrote:
>> Tom Lane wrote:
>>> object to arbitrarily moving a bunch of longstanding code from one file
>>> to another. What that is mainly going to accomplish is creating a big
>>> headache whenever we have to back-patch fixes that touch that code
>>> ... and what exactly did it buy in return?

>> Yes, I didn't like that either. The problem was that common.c was setup
>> to share code between pg_dump and a long-forgotten tool for Postgres 4.X
>> called pg4_dump (yes, pre-1996). That code that was moved was really
>> not "common" in any current sense because it was used only by pg_dump
>> (not by pg_restore or pg_dumpall), so I moved it into dumpcatalog.c, and
>> put the really "common" code into common.c. (We could call it dumpmem.c
>> or something.)
>>
>> Now, one approach would have been to rename common.c to dumpcatalog.c in
>> git, then create a new common.c, but that seems quite confusing to
>> people trying to reconstruct the history.

That will not help. git is not smart enough to deal with back-patching
changes in code that has moved from one file to another. If you force
this through it will mean manual adjustment of every back-patchable fix
that has to touch this code, for the next five or six years. Yes, the
name "common.c" is a bit of a misnomer now, but that doesn't mean we
have to change it, especially since the file's header comment already
explained what it was common for (and could be modified some more if
you don't find that adequate).

>> It is not possible to just link the old common.c into pg_restore and
>> pg_dumpall because it contains calls to pg_dump-only functions.
>>
>> Ideas?

> The only other idea I have is to keep most functions in the mis-named
> common.c and move the memory functions into dumpmem.c and dumpmem.h and
> include that in the other C files, but that doesn't help with long-term
> code clarity.

I don't think there is any "long-term code clarity" gain here that is
worth the maintenance headache. Please put the existing functions back
where they were. Inventing new files for the new code seems fine.

regards, tom lane

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Bruce Momjian 2011-11-26 17:15:39 Re: [COMMITTERS] pgsql: Modify pg_dump to use error-free memory allocation macros. This
Previous Message Andrew Dunstan 2011-11-26 14:51:30 Re: pgsql: Modify pg_dump to use error-free memory allocation macros. This

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2011-11-26 16:02:10 Re: psql setenv command
Previous Message Pavan Deolasee 2011-11-26 15:52:50 Avoiding repeated snapshot computation