pgsql: Throw error for ALTER TABLE RESET of an invalid option

On 2014-08-25 21:06:43 +0000, Bruce Momjian wrote:

Throw error for ALTER TABLE RESET of an invalid option

Also adjust pg_upgrade to not use this method for optional TOAST table
creation.

Patch by Fabr�zio de Royes Mello

I still think this is a bad idea and should rather be reverted.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Mon, Aug 25, 2014 at 11:30:53PM +0200, Andres Freund wrote:

On 2014-08-25 21:06:43 +0000, Bruce Momjian wrote:

Throw error for ALTER TABLE RESET of an invalid option

Also adjust pg_upgrade to not use this method for optional TOAST table
creation.

Patch by Fabr�zio de Royes Mello

I still think this is a bad idea and should rather be reverted.

Uh, which idea don't you like. I thought the discussion had a clear
conclusion.

The only comment I see from you on the thread is:

Why not simply not do anything? This doesn't prevent any bugs and
requiring pg-upgrade specific checks in there seems absurd. Also
somebody might use it for similar purposes.

but it seems most people says it should throw an error, the strongest
argument being the RESET throws an error for an invalid setting.

Is there something you want to re-discuss, or are you just re-stating
your objection, and if so, for what purpose --- you have not added any
additional information for people to think about.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Bruce Momjian <bruce@momjian.us> writes:

On Mon, Aug 25, 2014 at 11:30:53PM +0200, Andres Freund wrote:

I still think this is a bad idea and should rather be reverted.

Uh, which idea don't you like. I thought the discussion had a clear
conclusion.

The discussion seemed to have a consensus that an error would be a good
thing, but the problem is that this implementation is complete junk.

transformRelOptions has *no* business throwing an error, because it has
no idea what the set of valid options is; notice the patch didn't even
touch the comment it falsified:

* Note that this is not responsible for determining whether the options
* are valid, but it does check that namespaces for all the options given are

The kluge that was used instead of actual knowledge (ie look at the
relOptions array) isn't an adequate answer. This coding will allow
options that aren't valid for the particular relkind, and it will probably
reject extension options that should be allowed, and it certainly has no
clue whether the option is valid in the particular option namespace.

A larger point is that we could easily consider RESET as meaning
"remove this option *if it's applied to this relation*", which would
mean that resetting a nonexistent option shouldn't be an error.
If we don't define the action that way, then should RESET foo, where
foo is a valid option that's not been set on the particular table,
be an error? If not, what's the argument for allowing that case
and not this one? Do we need a RESET IF EXISTS to cover that?

Please revert and return the patch for further work/discussion.
We had consensus on a vague idea, not the details of this particular
patch.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Mon, Aug 25, 2014 at 07:44:06PM -0400, Tom Lane wrote:

A larger point is that we could easily consider RESET as meaning
"remove this option *if it's applied to this relation*", which would
mean that resetting a nonexistent option shouldn't be an error.
If we don't define the action that way, then should RESET foo, where
foo is a valid option that's not been set on the particular table,
be an error? If not, what's the argument for allowing that case
and not this one? Do we need a RESET IF EXISTS to cover that?

Please revert and return the patch for further work/discussion.
We had consensus on a vague idea, not the details of this particular
patch.

OK, that makes sense. Reverted.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers