Thread: Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Pavel Stehule
Date:
Hi
2017-01-10 17:31 GMT+01:00 David Fetter <david@fetter.org>:
On Mon, Jan 09, 2017 at 07:52:11PM -0300, Alvaro Herrera wrote:
> David Fetter wrote:
>
> > + if (query->commandType == CMD_UPDATE || query->commandType == CMD_DELETE)
> > + {
> > + /* Make sure there's something to look at. */
> > + Assert(query->jointree != NULL);
> > + if (query->jointree->quals == NULL)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("%s requires a WHERE clause when the require_where hook is enabled.",
> > + query->commandType == CMD_UPDATE ? "UPDATE" : "DELETE"),
> > + errhint("To %s all rows, use \"WHERE true\" or similar.",
> > + query->commandType == CMD_UPDATE ? "update" : "delete")));
> > + }
>
> Per my earlier comment, I think this should use
> ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED instead.
Fixed.
> I think this should say "the \"require_hook\" extension" rather than
> use the term "hook".
Fixed.
> (There are two or three translatability rules violations in this
> snippet,
Based on the hints in the docs docs around translation, I've
refactored this a bit.
Final review:
1. there are not any problem with patching, compilation.
2. all regress tests passed
3. the documentation and regress tests are good enough
4. the code respects postgres formatting rules
I'll mark this patch as ready for commiter
Regards
Pavel
> but since this is an extension and those are not translatable, I
> won't say elaborate further.)
"Not translatable," or "not currently translated?"
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Michael Paquier
Date:
On Wed, Jan 11, 2017 at 11:55 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > I'll mark this patch as ready for commiter Moved to CF 2017-03. -- Michael
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Bruce Momjian
Date:
I just don't see this patch going in. I think it needs are larger approach to the problems it is trying to solve. I thinkit then will be very useful.
On Thu, Feb 02, 2017 at 03:16:29PM +0000, Bruce Momjian wrote: > I just don't see this patch going in. I think it needs are larger > approach to the problems it is trying to solve. I think it then > will be very useful. What problems that it's trying to solve are not addressed? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATEand DELETE
From
Bruce Momjian
Date:
On Thu, Feb 2, 2017 at 07:18:45AM -0800, David Fetter wrote: > On Thu, Feb 02, 2017 at 03:16:29PM +0000, Bruce Momjian wrote: > > I just don't see this patch going in. I think it needs are larger > > approach to the problems it is trying to solve. I think it then > > will be very useful. > > What problems that it's trying to solve are not addressed? Unjoined tables. Inconsistent alias references. I think there are a bunch of things and if we can make a list and get a mode to warn about all of them, it would be very useful. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
On Thu, Feb 02, 2017 at 10:34:43AM -0500, Bruce Momjian wrote: > On Thu, Feb 2, 2017 at 07:18:45AM -0800, David Fetter wrote: > > On Thu, Feb 02, 2017 at 03:16:29PM +0000, Bruce Momjian wrote: > > > I just don't see this patch going in. I think it needs are > > > larger approach to the problems it is trying to solve. I think > > > it then will be very useful. > > > > What problems that it's trying to solve are not addressed? > > Unjoined tables. Inconsistent alias references. I think there are > a bunch of things and if we can make a list and get a mode to warn > about all of them, it would be very useful. There is always a delicate balance between putting in all that's required for a minimum viable feature and actually getting something out there. As I see it, this feature's main benefit is that it prevents some very common and at the same time very damaging kinds of harm. It also, for now, serves a pedagogical purpose. It's relatively straight-forward for someone with little or no PostgreSQL experience to look at it and see what it does. I originally sent the feature to cover unsubtle types of blunders, I'd like to keep "unsubtle blunders" as the guiding principle here. I can see where an unintentional CROSS JOIN would qualify under that standard. I'm not sure I understand what you mean by inconsistent aliasing. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Pavel Stehule
Date:
2017-02-02 16:34 GMT+01:00 Bruce Momjian <bruce@momjian.us>:
On Thu, Feb 2, 2017 at 07:18:45AM -0800, David Fetter wrote:
> On Thu, Feb 02, 2017 at 03:16:29PM +0000, Bruce Momjian wrote:
> > I just don't see this patch going in. I think it needs are larger
> > approach to the problems it is trying to solve. I think it then
> > will be very useful.
>
> What problems that it's trying to solve are not addressed?
Unjoined tables. Inconsistent alias references. I think there are a
bunch of things and if we can make a list and get a mode to warn about
all of them, it would be very useful.
Identification of unjoined tables should be very useful - but it is far to original proposal - so it can be solved separately.
This patch is simple - and usually we prefer more simple patches than one bigger.
Better to enhance this feature step by step.
Regards
Pavel
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATEand DELETE
From
Alvaro Herrera
Date:
Pavel Stehule wrote: > Identification of unjoined tables should be very useful - but it is far to > original proposal - so it can be solved separately. > > This patch is simple - and usually we prefer more simple patches than one > bigger. > > Better to enhance this feature step by step. Agreed -- IMO this is a reasonable first step, except that I would rename the proposed extension so that it doesn't focus solely on the first step. I'd pick a name that suggests that various kinds of checks are applied to queries, so "require_where" would be only one of various options that can be enabled. It will make no sense to enable the require_where module in order to disallow unjoined tables. At the same time, I don't see us providing a dozen of "require_foo" modules. So we'd better start making this one extensible from the get go. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Pavel Stehule wrote:
>> Better to enhance this feature step by step.
> Agreed -- IMO this is a reasonable first step, except that I would
> rename the proposed extension so that it doesn't focus solely on the
> first step.
Quite. The patch fails to make up its mind whether it's a trivial example
meant as a coding demonstration, or something that's going to become
actually useful.
In the category of "actually useful", I would put checks like "are there
unqualified outer references in subqueries". That's something we see
complaints about at least once a month, I think, and it's the type of
error that even seasoned SQL authors can make easily. But the current
patch is not extensible in that direction (checking for this in
post_parse_analyze_hook seems quite impractical).
Also, somebody who wants a check like that isn't necessarily going to want
"no WHERE clause" training wheels. So you're going to need to think about
facilities to enable or disable different checks.
regards, tom lane
On Thu, Feb 02, 2017 at 12:14:10PM -0500, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Pavel Stehule wrote: > >> Better to enhance this feature step by step. > > > Agreed -- IMO this is a reasonable first step, except that I would > > rename the proposed extension so that it doesn't focus solely on > > the first step. > > Quite. The patch fails to make up its mind whether it's a trivial > example meant as a coding demonstration, or something that's going > to become actually useful. > > In the category of "actually useful", I would put checks like "are > there unqualified outer references in subqueries". That's something > we see complaints about at least once a month, I think, and it's the > type of error that even seasoned SQL authors can make easily. But > the current patch is not extensible in that direction (checking for > this in post_parse_analyze_hook seems quite impractical). > > Also, somebody who wants a check like that isn't necessarily going > to want "no WHERE clause" training wheels. So you're going to need > to think about facilities to enable or disable different checks. This is just the discussion I'd hoped for. I'll draft up a patch in the next day or two, reflecting what's gone so far. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATEand DELETE
From
Nico Williams
Date:
On Thu, Feb 02, 2017 at 12:14:10PM -0500, Tom Lane wrote: > Also, somebody who wants a check like that isn't necessarily going to want > "no WHERE clause" training wheels. So you're going to need to think about > facilities to enable or disable different checks. WHERE-less-ness should be something that should be visible to a statement trigger that could then reject the operation if desirable. Nico --
On Thu, Feb 2, 2017 at 11:40 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Pavel Stehule wrote:
> Identification of unjoined tables should be very useful - but it is far to
> original proposal - so it can be solved separately.
>
> This patch is simple - and usually we prefer more simple patches than one
> bigger.
>
> Better to enhance this feature step by step.
Agreed -- IMO this is a reasonable first step, except that I would
rename the proposed extension so that it doesn't focus solely on the
first step. I'd pick a name that suggests that various kinds of checks
are applied to queries, so "require_where" would be only one of various
options that can be enabled.
A general SQL-Critic would be a very welcome extension.
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Michael Paquier
Date:
On Sun, Feb 5, 2017 at 10:08 AM, Rod Taylor <rod.taylor@gmail.com> wrote: > A general SQL-Critic would be a very welcome extension. Please no hyphen for extension names! -- Michael
On Thu, Feb 2, 2017 at 12:24 PM, David Fetter <david@fetter.org> wrote: >> Also, somebody who wants a check like that isn't necessarily going >> to want "no WHERE clause" training wheels. So you're going to need >> to think about facilities to enable or disable different checks. > > This is just the discussion I'd hoped for. I'll draft up a patch in > the next day or two, reflecting what's gone so far. It looks like this was never produced, and it's been over a month. A patch that hasn't been updated in over a month and doesn't have complete consensus doesn't seem like something we should still be thinking about committing in the second half of March, so I'm going to mark this Returned with Feedback. On the substance of the issue, I think there's no problem with having a module like this, and I think it's fine if it only handles the WHERE-less case in the first version. Somebody can add more later if they want. But naming the module in a generic way so that it lends itself to such additions seems like a pretty good plan. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company