Re: Re: [COMMITTERS] pgsql: Don't override arguments set via options with positional argumen

Don't override arguments set via options with positional arguments.

A number of utility programs were rather careless about paremeters
that can be set via both an option argument and a positional
argument. This leads to results which can violate the Principal
Of Least Astonishment. These changes refuse to use positional
arguments to override settings that have been made via positional
arguments. The changes are backpatched to all live branches.

Branch
------
REL8_3_STABLE

Details
-------
http://git.postgresql.org/pg/commitdiff/7bebb851920e74cda57b866ac2f64123b44323c5

Modified Files
--------------
src/bin/initdb/initdb.c | 7 +++++--
src/bin/scripts/clusterdb.c | 26 +++++++++++++++-----------
src/bin/scripts/createlang.c | 14 ++++++++++++--
src/bin/scripts/droplang.c | 14 ++++++++++++--
src/bin/scripts/reindexdb.c | 25 +++++++++++++++----------
src/bin/scripts/vacuumdb.c | 27 ++++++++++++++++-----------
6 files changed, 75 insertions(+), 38 deletions(-)

On Tue, Apr 17, 2012 at 6:39 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Don't override arguments set via options with positional arguments.
>
> A number of utility programs were rather careless about paremeters
> that can be set via both an option argument and a positional
> argument. This leads to results which can violate the Principal
> Of Least Astonishment. These changes refuse to use positional
> arguments to override settings that have been made via positional
> arguments. The changes are backpatched to all live branches.
>
> Branch
> ------
> REL8_3_STABLE

Uh, isn't it kind of a bad idea to back-patch something like this? It
seems like a behavior change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04/17/2012 07:08 PM, Robert Haas wrote:
> On Tue, Apr 17, 2012 at 6:39 PM, Andrew Dunstan<andrew(at)dunslane(dot)net> wrote:
>> Don't override arguments set via options with positional arguments.
>>
>> A number of utility programs were rather careless about paremeters
>> that can be set via both an option argument and a positional
>> argument. This leads to results which can violate the Principal
>> Of Least Astonishment. These changes refuse to use positional
>> arguments to override settings that have been made via positional
>> arguments. The changes are backpatched to all live branches.
>>
>> Branch
>> ------
>> REL8_3_STABLE
> Uh, isn't it kind of a bad idea to back-patch something like this? It
> seems like a behavior change.

It was discussed. I think the previous behaviour is a bug. It can't be
sane to be allowed to do:

initdb -D foo bar

cheers

andrew

On 04/17/2012 07:19 PM, Andrew Dunstan wrote:
>
>
> On 04/17/2012 07:08 PM, Robert Haas wrote:
>> On Tue, Apr 17, 2012 at 6:39 PM, Andrew Dunstan<andrew(at)dunslane(dot)net>
>> wrote:
>>> Don't override arguments set via options with positional arguments.
>>>
>>> A number of utility programs were rather careless about paremeters
>>> that can be set via both an option argument and a positional
>>> argument. This leads to results which can violate the Principal
>>> Of Least Astonishment. These changes refuse to use positional
>>> arguments to override settings that have been made via positional
>>> arguments. The changes are backpatched to all live branches.
>>>
>>> Branch
>>> ------
>>> REL8_3_STABLE
>> Uh, isn't it kind of a bad idea to back-patch something like this? It
>> seems like a behavior change.
>
>
> It was discussed. I think the previous behaviour is a bug. It can't be
> sane to be allowed to do:
>
> initdb -D foo bar
>
>
>

You know, I could have sworn it was discussed, but when I look back I
see it wasn't. I must have been remembering the recent logging protocol bug.

I'll revert it if people want, although I still think it's a bug.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> You know, I could have sworn it was discussed, but when I look back I
> see it wasn't. I must have been remembering the recent logging protocol bug.

> I'll revert it if people want, although I still think it's a bug.

I think we discussed it to the extent of agreeing it was a bug, but
the question of whether to back-patch was not brought up.

I can see both sides of this. I agree that the old behavior is buggy,
but what I imagine Robert is worried about is scripts that accidentally
work okay today and would stop working once the PG programs are fixed
to complain about bogus usage. People tend not to like it if you make
that kind of change in a minor release. Against that you have to set
the probability of mistaken interactive usage being caught, or not,
by a repaired program. Stopping a disastrous command-line typo seems
potentially worth any pain from having to fix scripts that would have
to be fixed eventually anyway.

If you had patched only HEAD I would have been fine with that, but
seeing that you've done the work to back-patch I'm kind of inclined
to let it stand.

regards, tom lane

On Tue, Apr 17, 2012 at 10:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I can see both sides of this.  I agree that the old behavior is buggy,
> but what I imagine Robert is worried about is scripts that accidentally
> work okay today and would stop working once the PG programs are fixed
> to complain about bogus usage.  People tend not to like it if you make
> that kind of change in a minor release.

Exactly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On tis, 2012-04-17 at 19:19 -0400, Andrew Dunstan wrote:
> It was discussed. I think the previous behaviour is a bug. It can't be
> sane to be allowed to do:
>
> initdb -D foo bar

It's debatable whether it should be allowed. I don't see anything wrong
with it. After all, later arguments usually override earlier arguments,
and non-option arguments notionally come after option arguments. Also,
if this should be disallowed, would you also disallow initdb -D foo -D
bar?

But what I think is worse is that the "bar" is silently ignored now. If
you think that this is an error, it should produce an error.

My vote is to revert this altogether and leave it be. In the
alternative, make it an error.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> My vote is to revert this altogether and leave it be. In the
> alternative, make it an error.

You mean in HEAD too? I don't agree with that, for sure. What this
patch is accomplishing is to make sure that the less-commonly-used
programs have similar command-line-parsing behavior to psql and pg_dump,
where we long ago realized that failure to check this carefully could
result in very confusing behavior. (Especially on machines where
getopt is willing to rearrange the command line.)

I agree with Andrew that this is a bug fix. I can see the argument
for not applying it to back branches, but not for declaring that it's
not a bug.

regards, tom lane

On ons, 2012-04-18 at 09:53 -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > My vote is to revert this altogether and leave it be. In the
> > alternative, make it an error.
>
> You mean in HEAD too? I don't agree with that, for sure. What this
> patch is accomplishing is to make sure that the less-commonly-used
> programs have similar command-line-parsing behavior to psql and pg_dump,
> where we long ago realized that failure to check this carefully could
> result in very confusing behavior. (Especially on machines where
> getopt is willing to rearrange the command line.)

OK, if you care strongly about that, make it an error. But don't just
ignore things.

> I agree with Andrew that this is a bug fix. I can see the argument
> for not applying it to back branches, but not for declaring that it's
> not a bug.

We shouldn't be backpatching things that are merely confusing. It works
as designed at the time, after all. Improvements belong in master.

On 04/18/2012 10:03 AM, Peter Eisentraut wrote:
> On ons, 2012-04-18 at 09:53 -0400, Tom Lane wrote:
>> Peter Eisentraut<peter_e(at)gmx(dot)net> writes:
>>> My vote is to revert this altogether and leave it be. In the
>>> alternative, make it an error.
>> You mean in HEAD too? I don't agree with that, for sure. What this
>> patch is accomplishing is to make sure that the less-commonly-used
>> programs have similar command-line-parsing behavior to psql and pg_dump,
>> where we long ago realized that failure to check this carefully could
>> result in very confusing behavior. (Especially on machines where
>> getopt is willing to rearrange the command line.)
> OK, if you care strongly about that, make it an error. But don't just
> ignore things.

It won't be ignored. It will be caught by the "too many arguments" logic.

The case where repeated arguments should be disallowed is a similar but
different case that probably demands a much larger patch. I don't think
its existence militates against this fix, however.

>
>> I agree with Andrew that this is a bug fix. I can see the argument
>> for not applying it to back branches, but not for declaring that it's
>> not a bug.
> We shouldn't be backpatching things that are merely confusing. It works
> as designed at the time, after all. Improvements belong in master.
>

If it was really intended to work this way then that's a piece of very
poor design, IMNSHO. It looks to me much more like it was just an
oversight.

I don't have terribly strong feelings about this, since we've not had
lots of complaints over the years, so I'll revert it in the back branches.

cheers

andrew