Date: Mon, 21 Jul 2008 00:21:31 -0700
From: daveg <daveg@sonic.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-patches@postgresql.org, hari@efrontier.com
Subject: Re: pg_dump lock timeout
Message-ID: <20080721072131.GE30869@sonic.net>
References: <20080511113047.GV5673@sonic.net>
 <20080511130035.GW5673@sonic.net>
	<20080702155626.GC30328@fetter.org>
	<20080717024407.GL11471@sonic.net> <13580.1216579850@sss.pgh.pa.us>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <13580.1216579850@sss.pgh.pa.us>
User-Agent: Mutt/1.4.2.1i

On Sun, Jul 20, 2008 at 02:50:50PM -0400, Tom Lane wrote:
> daveg <daveg@sonic.net> writes:
> > Here is an updated version of this patch against head. It builds, runs and
> > functions as expected. I did not build the sgml.
> 
> Applied with mostly minor cosmetic improvements --- the only actual
> error I noticed was failing to check whether the server version supports
> statement_timeout.

I chose not to test backend version on the grounds that getting an explicit
failure for an explicitly requested option would be preferable to it being
silently ignored. However if the user is trying to use the same scripts for
many versions then ignoring unsupported but unessential features may be
preferred.

One of the cosmetic changes made in response to other reviewers was to
not reuse lockquery, instead to have a separate query buffer. You have
reversed that and eliminated lockquery too. Which seems better.

> In most cases our policy has been that pg_dumpall should accept and pass
> through any pg_dump option for which it's sensible to do so. I did not
> make that happen but it seems it'd be a reasonable follow-on patch.

I'll remember that next time.
 
> A minor point is that the syntax "-X lock-wait-timeout=n" or
> "-X lock-wait-timeout n" won't work, although perhaps people used to
> -X might expect it to.  Since we deprecate -X (and don't even document
> it anymore), I thought that making this work would be much more trouble
> than it's worth, but perhaps that's open to argument.

All the other -X options are flags and supported directly by the getopt
machinery. Adding a -X option with an argument would require parsing
the argument by hand including dealing with '=' or ' ' as the separator and
telling getopt you had eaten an extra argument. This seemed a bit too much
code for the value of supporting a deprecated format for a brand new option.

Finally, you changed the option value and case from 1 to case 2. getopt_long()
only returns the value argument when there is no flag to set, so 1 was unique
and could have been read as "the first no-short option with an argument".

Thanks for checking this in.

-dg

-- 
David Gould       daveg@sonic.net      510 536 1443    510 282 0869
If simplicity worked, the world would be overrun with insects.