Обсуждение: 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@momjian.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@momjian.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@momjian.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Tom Lane wrote: > Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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@momjian.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 $@$(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 $@$(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