Re: pg_dump refactor patch to remove global variables

From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Joachim Wieland <joe(at)mcknight(dot)de>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_dump refactor patch to remove global variables
Date: 2014-08-31 03:12:18
Message-ID: 1409454738.14080.13.camel@vanquo.pezone.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

The general idea of this patch is not disputed, I think.

Some concerns about the details:

- Why is quote_all_identifiers left behind as a global variable?

- Shouldn't pg_dumpall also use DumpOptions?

- What about binary_upgrade?

- I think some of the variables in pg_dump's main() don't need to be
moved into DumpOptions. For example, compressLevel is only looked at
once in main() and then forgotten. We don't need to pass that around
everywhere. The same goes for dumpencoding and possibly others.

- The forward declaration of struct _dumpOptions in pg_backup.h seems
kind of useless. You could move some things around so that that's not
necessary.

- NewDumpOptions() and NewRestoreOptions() are both in
pg_backup_archiver.c, but NewRestoreOptions() is in pg_backup.h whereas
NewDumpOptions() is being put into pg_backup_archiver.h. None of that
makes too much sense, but it could be made more consistent.

- In dumpOptionsFromRestoreOptions() you are building the return value
in a local variable that is not zeroed. You should probably use
NewDumpOptions() there.

Also, looking at dumpOptionsFromRestoreOptions() and related code makes
me think that these should perhaps really be the same structure. Have
you investigated that?

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2014-08-31 03:52:39 Re: improving speed of make check-world
Previous Message Sawada Masahiko 2014-08-31 03:06:31 Re: add line number as prompt option to psql