Thread: Re: [COMMITTERS] pgsql: Strengthen warnings about using pg_dump's -i option.

momjian@postgresql.org (Bruce Momjian) writes:
> Log Message:
> -----------
> Strengthen warnings about using pg_dump's -i option.

The proposed TODO item was not about doing this, it was about removing
the option altogether.  AFAICS it's a foot-gun and nothing else --- why
do we have it?

BTW, a point I had forgotten is that pg_restore doesn't enforce that it
not be used with a newer server:
    /* XXX Should get this from the archive */    AHX->minRemoteVersion = 070100;    AHX->maxRemoteVersion = 999999;

I think this is probably sane, since after all we couldn't enforce that
the plain script output not be loaded into a newer server.  But it means
that -i is effectively a no-op for pg_restore, which again begs the
question of why we have it.
        regards, tom lane

Tom Lane wrote:
> momjian@postgresql.org (Bruce Momjian) writes:
> > Log Message:
> > -----------
> > Strengthen warnings about using pg_dump's -i option.
> 
> The proposed TODO item was not about doing this, it was about removing
> the option altogether.  AFAICS it's a foot-gun and nothing else --- why
> do we have it?

I thought the simple fix was to just have a better warning and see how
that works in practice.  There was some concern from people about
removing it without more feedback/warning.  I am happy to remove it.

> BTW, a point I had forgotten is that pg_restore doesn't enforce that it
> not be used with a newer server:
> 
>         /* XXX Should get this from the archive */
>         AHX->minRemoteVersion = 070100;
>         AHX->maxRemoteVersion = 999999;
> 
> I think this is probably sane, since after all we couldn't enforce that
> the plain script output not be loaded into a newer server.  But it means
> that -i is effectively a no-op for pg_restore, which again begs the
> question of why we have it.

So pg_restore -i does nothing?  Seems it should be removed.

The plain text file will be a foot-gun too, of course.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> momjian@postgresql.org (Bruce Momjian) writes:
>>> Strengthen warnings about using pg_dump's -i option.
>> 
>> The proposed TODO item was not about doing this, it was about removing
>> the option altogether.  AFAICS it's a foot-gun and nothing else --- why
>> do we have it?

> I thought the simple fix was to just have a better warning and see how
> that works in practice.  There was some concern from people about
> removing it without more feedback/warning.  I am happy to remove it.

My proposal would be to continue to accept the option but just ignore it
(ie, error out on version mismatch whether or not -i is given).  This
way we wouldn't break any scripts that use the option, but things would
still be safe.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> momjian@postgresql.org (Bruce Momjian) writes:
> >>> Strengthen warnings about using pg_dump's -i option.
> >> 
> >> The proposed TODO item was not about doing this, it was about removing
> >> the option altogether.  AFAICS it's a foot-gun and nothing else --- why
> >> do we have it?
> 
> > I thought the simple fix was to just have a better warning and see how
> > that works in practice.  There was some concern from people about
> > removing it without more feedback/warning.  I am happy to remove it.
> 
> My proposal would be to continue to accept the option but just ignore it
> (ie, error out on version mismatch whether or not -i is given).  This
> way we wouldn't break any scripts that use the option, but things would
> still be safe.

A larger question is why the option was added in the first place.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> My proposal would be to continue to accept the option but just ignore it
>> (ie, error out on version mismatch whether or not -i is given).  This
>> way we wouldn't break any scripts that use the option, but things would
>> still be safe.

> A larger question is why the option was added in the first place.

It probably seemed like the conservative choice at the time: allow the
user to be smarter than pg_dump when necessary.  What we couldn't have
foreseen was the way the option has been abused by tools that are not as
bright as they think they are.  With the current situation where -i is
used by default, without the user's knowledge (and without showing him
the warning messages, which is why your patch isn't going to improve
matters), it just seems too dangerous to continue to accept the switch.

(I wonder whether some of the complaints we've seen about broken
dump/restore are courtesy of pgAdmin forcing the dump to be taken with
a too-old copy of pg_dump.)

One point after looking back at the previous discussion is that the
current version test is too strict: it will complain if your server is
8.2.7 and pg_dump is 8.2.6.  We probably should not make a newer minor
number a hard error, since 99.99% of the time it would be fine.  So
while I think newer major should be a hard error regardless of -i,
we could consider several responses to newer minor:* silently allow it always* print warning and proceed always* allow
-ito control error vs warning for this case only.
 
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> My proposal would be to continue to accept the option but just ignore it
> >> (ie, error out on version mismatch whether or not -i is given).  This
> >> way we wouldn't break any scripts that use the option, but things would
> >> still be safe.
> 
> > A larger question is why the option was added in the first place.
> 
> It probably seemed like the conservative choice at the time: allow the
> user to be smarter than pg_dump when necessary.  What we couldn't have
> foreseen was the way the option has been abused by tools that are not as
> bright as they think they are.  With the current situation where -i is
> used by default, without the user's knowledge (and without showing him
> the warning messages, which is why your patch isn't going to improve
> matters), it just seems too dangerous to continue to accept the switch.
> 
> (I wonder whether some of the complaints we've seen about broken
> dump/restore are courtesy of pgAdmin forcing the dump to be taken with
> a too-old copy of pg_dump.)

Agreed, but I thought the tools have been fixed so is this still a
problem?

> One point after looking back at the previous discussion is that the
> current version test is too strict: it will complain if your server is
> 8.2.7 and pg_dump is 8.2.6.  We probably should not make a newer minor
> number a hard error, since 99.99% of the time it would be fine.  So
> while I think newer major should be a hard error regardless of -i,
> we could consider several responses to newer minor:
>     * silently allow it always
>     * print warning and proceed always
>     * allow -i to control error vs warning for this case only.

I think it should be silent.  Do we ever change the server behavior that
is visible to pg_dump in a minor release?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> One point after looking back at the previous discussion is that the
>> current version test is too strict: it will complain if your server is
>> 8.2.7 and pg_dump is 8.2.6.  We probably should not make a newer minor
>> number a hard error, since 99.99% of the time it would be fine.  So
>> while I think newer major should be a hard error regardless of -i,
>> we could consider several responses to newer minor:
>> * silently allow it always
>> * print warning and proceed always
>> * allow -i to control error vs warning for this case only.

> I think it should be silent.  Do we ever change the server behavior that
> is visible to pg_dump in a minor release?

It's hardly out of the question --- consider the backslash-escaping
security fixes we applied in 8.1.4, 8.0.8, etc.  Parts of the server
changes were intended to intentionally break unpatched clients, and
I think that'd apply to unpatched pg_dump as well.

Of course, that precedent suggests that any such change would be made in
such a way as to be enforced on the server side, so it wouldn't matter
if pg_dump didn't know it wouldn't work.

Silent allow is fine with me, I was just wondering if anyone liked
the other options better.
        regards, tom lane

I wrote:
> Silent allow is fine with me, I was just wondering if anyone liked
> the other options better.

Okay, I'm back on the warpath about this after noting yet another user
who thinks he should use -i mindlessly:
http://archives.postgresql.org/pgsql-admin/2008-04/msg00111.php

I believe the consensus was

* pg_dump and pg_restore should continue to accept -i/--ignore-version
options, to avoid needlessly breaking existing scripts, but these
switches will become no-ops; the version check will occur anyway.

* pg_dump should be fixed to allow server minor version greater than its
own, but not server major version.

Last chance for objections...
        regards, tom lane

Tom Lane wrote:

> I believe the consensus was
> 
> * pg_dump and pg_restore should continue to accept -i/--ignore-version
> options, to avoid needlessly breaking existing scripts, but these
> switches will become no-ops; the version check will occur anyway.
> 
> * pg_dump should be fixed to allow server minor version greater than its
> own, but not server major version.
> 
> Last chance for objections...

None here.

Joshua D. Drake