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

Modify pg_dump to use error-free memory allocation macros. This avoids
ignoring errors and call-site error checking.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/3c0afde11a12bb3ca7c68a30ad0dedaa0d1adef5

Modified Files
--------------
src/bin/pg_dump/Makefile | 8 +-
src/bin/pg_dump/common.c | 977 +--------------------------------
src/bin/pg_dump/common.h | 24 +
src/bin/pg_dump/compress_io.c | 45 +--
src/bin/pg_dump/dumpcatalog.c | 978 +++++++++++++++++++++++++++++++++
src/bin/pg_dump/dumputils.c | 1 +
src/bin/pg_dump/pg_backup_archiver.c | 92 ++--
src/bin/pg_dump/pg_backup_custom.c | 23 +-
src/bin/pg_dump/pg_backup_db.c | 21 +-
src/bin/pg_dump/pg_backup_directory.c | 25 +-
src/bin/pg_dump/pg_backup_files.c | 13 +-
src/bin/pg_dump/pg_backup_null.c | 5 +-
src/bin/pg_dump/pg_backup_tar.c | 27 +-
src/bin/pg_dump/pg_dump.c | 361 ++++++------
src/bin/pg_dump/pg_dump.h | 5 -
src/bin/pg_dump/pg_dump_sort.c | 23 +-
src/bin/pg_dump/pg_dumpall.c | 65 ++-
src/bin/pg_dump/pg_restore.c | 25 +-
18 files changed, 1357 insertions(+), 1361 deletions(-)

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Modify pg_dump to use error-free memory allocation macros. This avoids
> ignoring errors and call-site error checking.

This appears to have broken the MSVC build. More generally, I'd like to
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?

regards, tom lane

On 11/26/2011 02:13 AM, Tom Lane wrote:
> Bruce Momjian<bruce(at)momjian(dot)us> writes:
>> Modify pg_dump to use error-free memory allocation macros. This avoids
>> ignoring errors and call-site error checking.
> This appears to have broken the MSVC build. More generally, I'd like to
> 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?
>
>

Was there any discussion of this change? It seems much too big for
something to be done without discussion, but I don't recall seeing
anything. (And it's probably seriously broken two patches I have in the
commitfest).

cheers

andrew

\Andrew Dunstan wrote:
>
>
> On 11/26/2011 02:13 AM, Tom Lane wrote:
> > Bruce Momjian<bruce(at)momjian(dot)us> writes:
> >> Modify pg_dump to use error-free memory allocation macros. This avoids
> >> ignoring errors and call-site error checking.
> > This appears to have broken the MSVC build. More generally, I'd like to
> > 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?
> >
> >
>
>
> Was there any discussion of this change? It seems much too big for
> something to be done without discussion, but I don't recall seeing
> anything. (And it's probably seriously broken two patches I have in the
> commitfest).

The patch was posted Novembrer 14 and received no replies:

http://archives.postgresql.org/pgsql-hackers/2011-11/msg00860.php

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

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

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Modify pg_dump to use error-free memory allocation macros. This avoids
> > ignoring errors and call-site error checking.
>
> This appears to have broken the MSVC build. More generally, I'd like to

Doesn't the MSVC build scrape the Makefiles? Looking at Mkvcbuild.pm,
it seems it doesn't for pg_dump? When exactly does the MSVC build have
to be adjusted for makefile changes?

I will adjust Mkvcbuild.pm, assuming we want to keep this change.

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

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?

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

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

On 11/26/2011 09:11 AM, Bruce Momjian wrote:
> \Andrew Dunstan wrote:
>>
>> On 11/26/2011 02:13 AM, Tom Lane wrote:
>>> Bruce Momjian<bruce(at)momjian(dot)us> writes:
>>>> Modify pg_dump to use error-free memory allocation macros. This avoids
>>>> ignoring errors and call-site error checking.
>>> This appears to have broken the MSVC build. More generally, I'd like to
>>> 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?
>>>
>>>
>>
>> Was there any discussion of this change? It seems much too big for
>> something to be done without discussion, but I don't recall seeing
>> anything. (And it's probably seriously broken two patches I have in the
>> commitfest).
> The patch was posted Novembrer 14 and received no replies:
>
> http://archives.postgresql.org/pgsql-hackers/2011-11/msg00860.php
>

Oh, I missed it. Sorry.

cheers

andrew

Andrew Dunstan wrote:
>
>
> On 11/26/2011 09:11 AM, Bruce Momjian wrote:
> > \Andrew Dunstan wrote:
> >>
> >> On 11/26/2011 02:13 AM, Tom Lane wrote:
> >>> Bruce Momjian<bruce(at)momjian(dot)us> writes:
> >>>> Modify pg_dump to use error-free memory allocation macros. This avoids
> >>>> ignoring errors and call-site error checking.
> >>> This appears to have broken the MSVC build. More generally, I'd like to
> >>> 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?
> >>>
> >>>
> >>
> >> Was there any discussion of this change? It seems much too big for
> >> something to be done without discussion, but I don't recall seeing
> >> anything. (And it's probably seriously broken two patches I have in the
> >> commitfest).
> > The patch was posted Novembrer 14 and received no replies:
> >
> > http://archives.postgresql.org/pgsql-hackers/2011-11/msg00860.php
> >
>
> Oh, I missed it. Sorry.

No problem. Tom is discussing if this is the best approach, and if it
is the MSVC build needs to be fixed. I normally don't hold patches for
two weeks but waited to be sure I was available to fix any breakage.

--
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:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > Modify pg_dump to use error-free memory allocation macros. This avoids
> > > ignoring errors and call-site error checking.
> >
> > This appears to have broken the MSVC build. More generally, I'd like to
>
> Doesn't the MSVC build scrape the Makefiles? Looking at Mkvcbuild.pm,
> it seems it doesn't for pg_dump? When exactly does the MSVC build have
> to be adjusted for makefile changes?
>
> I will adjust Mkvcbuild.pm, assuming we want to keep this change.
>
> > 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.
>
> 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.

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

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

On 11/26/2011 09:18 AM, Bruce Momjian wrote:
> Tom Lane wrote:
>> Bruce Momjian<bruce(at)momjian(dot)us> writes:
>>> Modify pg_dump to use error-free memory allocation macros. This avoids
>>> ignoring errors and call-site error checking.
>> This appears to have broken the MSVC build. More generally, I'd like to
> Doesn't the MSVC build scrape the Makefiles? Looking at Mkvcbuild.pm,
> it seems it doesn't for pg_dump? When exactly does the MSVC build have
> to be adjusted for makefile changes?
>
> I will adjust Mkvcbuild.pm, assuming we want to keep this change.
>

I think the short answer is that the MSVC build system scrapes the
makefile for $OBJS but if you change the files on the target line you
will need to adjust it.

It has this for pg_dump:

my $pgdump = AddSimpleFrontend('pg_dump', 1);
$pgdump->AddIncludeDir('src\backend');
$pgdump->AddFile('src\bin\pg_dump\pg_dump.c');
$pgdump->AddFile('src\bin\pg_dump\common.c');
$pgdump->AddFile('src\bin\pg_dump\pg_dump_sort.c');
$pgdump->AddFile('src\bin\pg_dump\keywords.c');
$pgdump->AddFile('src\backend\parser\kwlookup.c');

But you have made this change in the makefile:

-pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) $(KEYWRDOBJS) |
submake-libpq submake-libpgport
- $(CC) $(CFLAGS) pg_dump.o common.o pg_dump_sort.o $(KEYWRDOBJS)
$(OBJS) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $(at)$(X)
+pg_dump: pg_dump.o dumpcatalog.o pg_dump_sort.o $(OBJS)
$(KEYWRDOBJS) | submake-libpq submake-libpgport
+ $(CC) $(CFLAGS) pg_dump.o dumpcatalog.o pg_dump_sort.o
$(KEYWRDOBJS) $(OBJS) $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX)
$(LIBS) -o $(at)$(X)

so above, I think you would need to replace:

$pgdump->AddFile('src\bin\pg_dump\common.c');

with

$pgdump->AddFile('src\bin\pg_dump\dumpcatalog.c');

cheers

andrew

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

Tom Lane wrote:
> >> 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.

Well, you do a lot more backpatching than I do, so I am willing to do as
you request, but looking at the git history for common.c, I don't see a
lot of backpatching for this file.

Basically, if we keep the existing functions in common.c, we are
deciding that it is never worth moving C functions to new C files for
source code clarity. I was not sure we had made that decision in the
past.

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

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Basically, if we keep the existing functions in common.c, we are
> deciding that it is never worth moving C functions to new C files for
> source code clarity. I was not sure we had made that decision in the
> past.

I once held hope that git would let us be more flexible about this,
but experimentation has shown that it's not significantly smarter than
CVS about back-patches in moved code :-(.

I don't want to say that we should *never* move code once it's released;
but I think the bar for that needs to be set pretty high, and this
particular case isn't clearing the bar for me.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Basically, if we keep the existing functions in common.c, we are
> > deciding that it is never worth moving C functions to new C files for
> > source code clarity. I was not sure we had made that decision in the
> > past.
>
> I once held hope that git would let us be more flexible about this,
> but experimentation has shown that it's not significantly smarter than
> CVS about back-patches in moved code :-(.
>
> I don't want to say that we should *never* move code once it's released;
> but I think the bar for that needs to be set pretty high, and this
> particular case isn't clearing the bar for me.

OK, hearing no other opinions, I have moved the functions back into
common.c, and created a new dumpmem.c/h. Applied patch attached.

There is now no need to modify MSVC.

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

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