Thread: pgsql: Throw error for ALTER TABLE RESET of an invalid option
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 Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/73d78e11a0f7183c80b93eefbbb6026fe9664015 Modified Files -------------- contrib/pg_upgrade/dump.c | 9 ++++++++- src/backend/access/common/reloptions.c | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-)
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
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. +
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
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. +