Thread: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
Folks, Please find attached a patch which makes it possible to disallow UPDATEs and DELETEs which lack a WHERE clause. As this changes query behavior, I've made the new GUCs PGC_SUSET. What say? Thanks to Gurjeet Singh for the idea and Andrew Gierth for the tips implementing. 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
Attachment
On Thu, Jul 21, 2016 at 10:27 AM, David Fetter <david@fetter.org> wrote: > Folks, > > Please find attached a patch which makes it possible to disallow > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > behavior, I've made the new GUCs PGC_SUSET. > > What say? > The use case for this functionality that comes to mind is to avoid deleting/updating all the data, if user has accidentally missed the WHERE clause. Do you have other use case for this functionality? With this functionality, if user needs to actually delete or update all the rows, then he has to artificially add where clause which seems slightly inconvenient, but may be such cases are less. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Thu, Jul 21, 2016 at 12:57 AM, David Fetter <david@fetter.org> wrote:
Folks,
Please find attached a patch which makes it possible to disallow
UPDATEs and DELETEs which lack a WHERE clause. As this changes query
behavior, I've made the new GUCs PGC_SUSET.
What say?
Can't you implement this as a extension? The SQL Firewall project is already doing some similar concepts by catching prohibiting SQL and preventing it from executing.
On 21 July 2016 at 15:49, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jul 21, 2016 at 10:27 AM, David Fetter <david@fetter.org> wrote:
> Folks,
>
> Please find attached a patch which makes it possible to disallow
> UPDATEs and DELETEs which lack a WHERE clause. As this changes query
> behavior, I've made the new GUCs PGC_SUSET.
>
> What say?
>
The use case for this functionality that comes to mind is to avoid
deleting/updating all the data, if user has accidentally missed the
WHERE clause. Do you have other use case for this functionality?
With this functionality, if user needs to actually delete or update
all the rows, then he has to artificially add where clause which seems
slightly inconvenient, but may be such cases are less.
It's a commonly requested feature. Personally I think it's kind of silly, but I've had multiple people ask me for it or how to do it too. So whether or not it's really effective/useful, it's in demand.
Personally I'd rather see it as part of an extension that does other filtering, I don't find it compelling for core. But I don't really object either.
David Fetter <david@fetter.org> writes: > Please find attached a patch which makes it possible to disallow > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > behavior, I've made the new GUCs PGC_SUSET. > What say? -1. This is an express violation of the SQL standard, and at least the UPDATE case has reasonable use-cases. Moreover, if your desire is to have training wheels for SQL, there are any number of other well-known gotchas that are just as dangerous, for example ye olde unintentionally-correlated subselect: https://www.postgresql.org/message-id/20160714135233.1410.92538%40wrigleys.postgresql.org I wouldn't have any objection to an extension that enforces rules like these, but I don't think it belongs in core. regards, tom lane
On 07/21/2016 06:49 AM, Tom Lane wrote: > David Fetter <david@fetter.org> writes: >> Please find attached a patch which makes it possible to disallow >> UPDATEs and DELETEs which lack a WHERE clause. As this changes query >> behavior, I've made the new GUCs PGC_SUSET. > >> What say? > -1 > -1. This is an express violation of the SQL standard, and at least the > UPDATE case has reasonable use-cases. Moreover, if your desire is to have > training wheels for SQL, there are any number of other well-known gotchas > that are just as dangerous, for example ye olde unintentionally-correlated > subselect: > https://www.postgresql.org/message-id/20160714135233.1410.92538%40wrigleys.postgresql.org > Yes but I used to teach a weak long class on relational databases using PostgreSQL. The entire week I would iterate over and over and over that you never use an UPDATE or DELETE without a transaction. Toward the end of the class we would being do problem sets that included UPDATE and DELETE. Guess how many would trash their data because they didn't use a WHERE clause AND didn't use a transaction? 50% These weren't kids, these weren't neophytes to technology. These were professionals, many of them programmers (PICK). > I wouldn't have any objection to an extension that enforces rules like > these, but I don't think it belongs in core. I agree it doesn't need to be in core. JD > > regards, tom lane > > -- Command Prompt, Inc. http://the.postgres.company/ +1-503-667-4564 PostgreSQL Centered full stack support, consulting and development. Everyone appreciates your honesty, until you are honest with them.
> Please find attached a patch which makes it possible to disallow > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > behavior, I've made the new GUCs PGC_SUSET. > > What say? DELETE FROM tbl WHERE true; ? -- Teodor Sigaev E-mail: teodor@sigaev.ru WWW: http://www.sigaev.ru/
On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote: > > Please find attached a patch which makes it possible to disallow > > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > > behavior, I've made the new GUCs PGC_SUSET. > > > > What say? > > DELETE FROM tbl WHERE true; ? I specifically left this possible so the feature when turned on allows people to do updates with an always-true qualifier if that's what they actually mean to do. In case it wasn't clear, unqualified updates and deletes are permitted by default. This patch allows people to set it so they're disallowed. 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
On Thu, Jul 21, 2016 at 12:39 PM, David Fetter <david@fetter.org> wrote: > On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote: >> > Please find attached a patch which makes it possible to disallow >> > UPDATEs and DELETEs which lack a WHERE clause. As this changes query >> > behavior, I've made the new GUCs PGC_SUSET. >> > >> > What say? >> >> DELETE FROM tbl WHERE true; ? > > I specifically left this possible so the feature when turned on allows > people to do updates with an always-true qualifier if that's what they > actually mean to do. > > In case it wasn't clear, unqualified updates and deletes are permitted > by default. This patch allows people to set it so they're disallowed. I join with others in thinking it's a reasonable contrib module. In fact, I already wrote it for my 2015 PGCon tutorial. Well, the "delete" part, anyway. https://github.com/robertmhaas/introduction-to-postgresql-hacking/compare/master...robertmhaas:delete_needs_where -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jul 21, 2016 at 09:21:55AM -0400, Jim Mlodgenski wrote: > On Thu, Jul 21, 2016 at 12:57 AM, David Fetter <david@fetter.org> wrote: > > Please find attached a patch which makes it possible to disallow > > UPDATEs and DELETEs which lack a WHERE clause. As this changes > > query behavior, I've made the new GUCs PGC_SUSET. > > > > What say? > > > Can't you implement this as a extension? Yes. In that case, I'd want to make it a contrib extension, as it is at least in theory attached to specific major versions of the backend. Also, if it's not in contrib, we can basically forget about having most people even know about it, let alone get specific separate permission to use it in production. That's reality, much as I would like it not to be. > The SQL Firewall project is already doing some similar concepts by > catching prohibiting SQL and preventing it from executing. > https://github.com/uptimejp/sql_firewall That's very nice, but it illustrates my point perfectly. The extension is from a current respected and prolific contributor to the community, but I had no idea that it was there, and by dint of writing the PostgreSQL Weekly News, I keep closer tabs on external things PostgreSQL than easily 99.9% of people who deploy it. 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
At 2016-07-21 12:46:29 -0400, robertmhaas@gmail.com wrote: > > I join with others in thinking it's a reasonable contrib module. I don't like the use of the term "empty" to describe an UPDATE or DELETE without a WHERE clause. -- Abhijit
On Thu, Jul 21, 2016 at 12:49 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote: > At 2016-07-21 12:46:29 -0400, robertmhaas@gmail.com wrote: >> >> I join with others in thinking it's a reasonable contrib module. > > I don't like the use of the term "empty" to describe an UPDATE or DELETE > without a WHERE clause. /me scratches head. Who used that term? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jul 21, 2016 at 12:46:29PM -0400, Robert Haas wrote: > On Thu, Jul 21, 2016 at 12:39 PM, David Fetter <david@fetter.org> wrote: > > On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote: > >> > Please find attached a patch which makes it possible to disallow > >> > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > >> > behavior, I've made the new GUCs PGC_SUSET. > >> > > >> > What say? > >> > >> DELETE FROM tbl WHERE true; ? > > > > I specifically left this possible so the feature when turned on allows > > people to do updates with an always-true qualifier if that's what they > > actually mean to do. > > > > In case it wasn't clear, unqualified updates and deletes are permitted > > by default. This patch allows people to set it so they're disallowed. > > I join with others in thinking it's a reasonable contrib module. In > fact, I already wrote it for my 2015 PGCon tutorial. Well, the > "delete" part, anyway. > > https://github.com/robertmhaas/introduction-to-postgresql-hacking/compare/master...robertmhaas:delete_needs_where I'm happy to write the rest of this as a contrib module. I hope to get to that this evening. 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
On Thu, Jul 21, 2016 at 12:51:50PM -0400, Robert Haas wrote: > On Thu, Jul 21, 2016 at 12:49 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote: > > At 2016-07-21 12:46:29 -0400, robertmhaas@gmail.com wrote: > >> > >> I join with others in thinking it's a reasonable contrib module. > > > > I don't like the use of the term "empty" to describe an UPDATE or DELETE > > without a WHERE clause. > > /me scratches head. > > Who used that term? I did out of failure to imagine another short way to describe the situation as I was writing it up. I'd be delighted to change it to something else. Best, David. Oh, and the bike shed should definitely be puce with blaze orange polka dots. -- 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
On 7/21/16 11:46 AM, David Fetter wrote: >> > Can't you implement this as a extension? > Yes. In that case, I'd want to make it a contrib extension, as it is > at least in theory attached to specific major versions of the backend. Howso? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On Thu, Jul 21, 2016 at 04:48:37PM -0500, Jim Nasby wrote: > On 7/21/16 11:46 AM, David Fetter wrote: > > > > Can't you implement this as a extension? > > Yes. In that case, I'd want to make it a contrib extension, as it is > > at least in theory attached to specific major versions of the backend. > > Howso? At least one of the structures it references isn't in a public API. 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
On Thu, Jul 21, 2016 at 09:52:26AM -0700, David Fetter wrote: > On Thu, Jul 21, 2016 at 12:46:29PM -0400, Robert Haas wrote: > > On Thu, Jul 21, 2016 at 12:39 PM, David Fetter <david@fetter.org> wrote: > > > On Thu, Jul 21, 2016 at 06:20:37PM +0300, Teodor Sigaev wrote: > > >> > Please find attached a patch which makes it possible to disallow > > >> > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > > >> > behavior, I've made the new GUCs PGC_SUSET. > > >> > > > >> > What say? > > >> > > >> DELETE FROM tbl WHERE true; ? > > > > > > I specifically left this possible so the feature when turned on allows > > > people to do updates with an always-true qualifier if that's what they > > > actually mean to do. > > > > > > In case it wasn't clear, unqualified updates and deletes are permitted > > > by default. This patch allows people to set it so they're disallowed. > > > > I join with others in thinking it's a reasonable contrib module. In > > fact, I already wrote it for my 2015 PGCon tutorial. Well, the > > "delete" part, anyway. > > > > https://github.com/robertmhaas/introduction-to-postgresql-hacking/compare/master...robertmhaas:delete_needs_where > > I'm happy to write the rest of this as a contrib module. I hope to > get to that this evening. I've renamed it to require_where and contrib-ified. 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
Attachment
On Fri, Jul 22, 2016 at 2:38 AM, David Fetter <david@fetter.org> wrote: > I've renamed it to require_where and contrib-ified. I'm not sure that the Authors section is entirely complete. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jul 25, 2016 at 11:12:24PM -0400, Robert Haas wrote: > On Fri, Jul 22, 2016 at 2:38 AM, David Fetter <david@fetter.org> wrote: > > I've renamed it to require_where and contrib-ified. > > I'm not sure that the Authors section is entirely complete. Does this suit? 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
On Mon, Jul 25, 2016 at 11:38 PM, David Fetter <david@fetter.org> wrote: > On Mon, Jul 25, 2016 at 11:12:24PM -0400, Robert Haas wrote: >> On Fri, Jul 22, 2016 at 2:38 AM, David Fetter <david@fetter.org> wrote: >> > I've renamed it to require_where and contrib-ified. >> >> I'm not sure that the Authors section is entirely complete. > > Does this suit? YFTATP. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 21/07/16 06:57, David Fetter wrote: > Folks, > > Please find attached a patch which makes it possible to disallow > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > behavior, I've made the new GUCs PGC_SUSET. > > What say? I say I don't like this at all. As mentioned elsewhere in the thread, you can just do WHERE true to get around it, so why on Earth have it PGC_SUSET? I would rather, if we need nannying at all, have a threshold of number of rows affected. So if your update or delete exceeds that threshold, the query will be canceled. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Jul 26, 2016 at 04:39:14PM -0400, Robert Haas wrote: > On Mon, Jul 25, 2016 at 11:38 PM, David Fetter <david@fetter.org> wrote: > > On Mon, Jul 25, 2016 at 11:12:24PM -0400, Robert Haas wrote: > >> On Fri, Jul 22, 2016 at 2:38 AM, David Fetter <david@fetter.org> wrote: > >> > I've renamed it to require_where and contrib-ified. > >> > >> I'm not sure that the Authors section is entirely complete. > > > > Does this suit? > > YFTATP. Oops. I'd done it on the commitfest app, but not in the patch. I've also made this PGC_USERSET. 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
Attachment
On 7/26/16 6:14 PM, Vik Fearing wrote: > As mentioned elsewhere in the thread, you can just do WHERE true to get > around it, so why on Earth have it PGC_SUSET? I'm not sure whether it's supposed to guard against typos and possibly buggy SQL string concatenation in application code. So it would help against accidental mistakes, whereas putting a WHERE TRUE in there would be an intentional override. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 27/07/16 03:15, Peter Eisentraut wrote: > On 7/26/16 6:14 PM, Vik Fearing wrote: >> As mentioned elsewhere in the thread, you can just do WHERE true to get >> around it, so why on Earth have it PGC_SUSET? > > I'm not sure whether it's supposed to guard against typos and possibly > buggy SQL string concatenation in application code. So it would help > against accidental mistakes, whereas putting a WHERE TRUE in there would > be an intentional override. If buggy SQL string concatenation in application code is your argument, quite a lot of them add "WHERE true" so that they can just append a bunch of "AND ..." clauses without worrying if it's the first (or last, whatever), so I'm not sure this is protecting anything. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 7/26/16 6:14 PM, Vik Fearing wrote:
>> As mentioned elsewhere in the thread, you can just do WHERE true to get
>> around it, so why on Earth have it PGC_SUSET?
> I'm not sure whether it's supposed to guard against typos and possibly
> buggy SQL string concatenation in application code. So it would help
> against accidental mistakes, whereas putting a WHERE TRUE in there would
> be an intentional override.
Maybe I misunderstood Vik's point; I thought he was complaining that
it's silly to make this SUSET rather than USERSET. I tend to agree.
We have a rough consensus that GUCs that change query semantics are
bad, but if it simply throws an error (or not) then it's not likely
to cause any surprising application behaviors.
regards, tom lane
On Wed, Jul 27, 2016 at 03:24:28AM +0200, Vik Fearing wrote: > On 27/07/16 03:15, Peter Eisentraut wrote: > > On 7/26/16 6:14 PM, Vik Fearing wrote: > >> As mentioned elsewhere in the thread, you can just do WHERE true > >> to get around it, so why on Earth have it PGC_SUSET? > > > > I'm not sure whether it's supposed to guard against typos and > > possibly buggy SQL string concatenation in application code. So > > it would help against accidental mistakes, whereas putting a WHERE > > TRUE in there would be an intentional override. > > If buggy SQL string concatenation in application code is your > argument, quite a lot of them add "WHERE true" so that they can just > append a bunch of "AND ..." clauses without worrying if it's the > first (or last, whatever), so I'm not sure this is protecting > anything. I am sure that I'm not the only one who's been asked for this feature because people other than me have piped up on this thread to that very effect. I understand that there may well be lots of really meticulous people on this list, people who would never accidentally do an unqualified DELETE on a table in production, but I can't claim to be one of them because I have, and not just once. It's under once a decade, but even that's too many. I'm not proposing to make this feature default, or even available by default, but I am totally certain that this is the kind of feature people would really appreciate, even if it doesn't prevent every catastrophe. 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
On 27/07/16 06:11, David Fetter wrote: > On Wed, Jul 27, 2016 at 03:24:28AM +0200, Vik Fearing wrote: >> On 27/07/16 03:15, Peter Eisentraut wrote: >>> On 7/26/16 6:14 PM, Vik Fearing wrote: >>>> As mentioned elsewhere in the thread, you can just do WHERE true >>>> to get around it, so why on Earth have it PGC_SUSET? >>> >>> I'm not sure whether it's supposed to guard against typos and >>> possibly buggy SQL string concatenation in application code. So >>> it would help against accidental mistakes, whereas putting a WHERE >>> TRUE in there would be an intentional override. >> >> If buggy SQL string concatenation in application code is your >> argument, quite a lot of them add "WHERE true" so that they can just >> append a bunch of "AND ..." clauses without worrying if it's the >> first (or last, whatever), so I'm not sure this is protecting >> anything. > > I am sure that I'm not the only one who's been asked for this feature > because people other than me have piped up on this thread to that very > effect. Sure. I'm just saying that I think it is poorly designed. I think it would be far better to error out if the command affects x rows, or an estimated y% of the table. Doing that, and also allowing the user to turn it off, would solve the problem as I understand your presentation of it. > I understand that there may well be lots of really meticulous people > on this list, people who would never accidentally do an unqualified > DELETE on a table in production, but I can't claim to be one of them > because I have, and not just once. It's under once a decade, but even > that's too many. That doesn't mean that requiring a WHERE clause -- without even looking at what's in it -- is a good idea. Why not start by turning off autocommit, for example? > I'm not proposing to make this feature default, or even available by > default, but I am totally certain that this is the kind of feature > people would really appreciate, even if it doesn't prevent every > catastrophe. This kind of feature, why not. This feature, no. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Jul 26, 2016 at 6:15 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 7/26/16 6:14 PM, Vik Fearing wrote:
> As mentioned elsewhere in the thread, you can just do WHERE true to get
> around it, so why on Earth have it PGC_SUSET?
I'm not sure whether it's supposed to guard against typos and possibly
buggy SQL string concatenation in application code. So it would help
against accidental mistakes, whereas putting a WHERE TRUE in there would
be an intentional override.
I know I'm late to the thread here, but I just wanted to add my small voice in support
of this feature.
Over the years we've seen this happen at Heroku quite a bit (accidental manual entry
without a where clause) and the only minor gripe I'd have is that contrib modules are
very undiscoverable and users tend not to find out about them.
On the other hand, a session setting in core would probably not be that different.
I expect Heroku will probably wind up enabling this by default on any interactive
psql sessions.
(And I would encourage packagers and distributors to consider doing the same.)
-p
On Wed, Jul 27, 2016 at 02:59:17PM +0200, Vik Fearing wrote: > On 27/07/16 06:11, David Fetter wrote: > > On Wed, Jul 27, 2016 at 03:24:28AM +0200, Vik Fearing wrote: > >> On 27/07/16 03:15, Peter Eisentraut wrote: > >>> On 7/26/16 6:14 PM, Vik Fearing wrote: > >>>> As mentioned elsewhere in the thread, you can just do WHERE true > >>>> to get around it, so why on Earth have it PGC_SUSET? > >>> > >>> I'm not sure whether it's supposed to guard against typos and > >>> possibly buggy SQL string concatenation in application code. So > >>> it would help against accidental mistakes, whereas putting a WHERE > >>> TRUE in there would be an intentional override. > >> > >> If buggy SQL string concatenation in application code is your > >> argument, quite a lot of them add "WHERE true" so that they can just > >> append a bunch of "AND ..." clauses without worrying if it's the > >> first (or last, whatever), so I'm not sure this is protecting > >> anything. > > > > I am sure that I'm not the only one who's been asked for this feature > > because people other than me have piped up on this thread to that very > > effect. > > Sure. I'm just saying that I think it is poorly designed. I think it > would be far better to error out if the command affects x rows, or an > estimated y% of the table. What else would constitute a good design? I am a little wary of relying on estimates, at least those provided by EXPLAIN, because the row counts they produce can be off by several orders of magnitude. Are there more accurate ways to estimate? Would you want x and y to be parameters somewhere? > Doing that, and also allowing the user to turn it off, would solve the > problem as I understand your presentation of it. I made it PGC_USERSET in the third patch. > > I understand that there may well be lots of really meticulous people > > on this list, people who would never accidentally do an unqualified > > DELETE on a table in production, but I can't claim to be one of them > > because I have, and not just once. It's under once a decade, but even > > that's too many. > > That doesn't mean that requiring a WHERE clause -- without even looking > at what's in it -- is a good idea. > > Why not start by turning off autocommit, for example? Because that setting is client side, and even more vulnerable to not being turned on for everyone everywhere. > > I'm not proposing to make this feature default, or even available by > > default, but I am totally certain that this is the kind of feature > > people would really appreciate, even if it doesn't prevent every > > catastrophe. > > This kind of feature, why not. This feature, no. I would very much value your input into the design of the feature. 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
On Thu, Jul 21, 2016 at 09:49:33AM -0400, Tom Lane wrote: > David Fetter <david@fetter.org> writes: > > Please find attached a patch which makes it possible to disallow > > UPDATEs and DELETEs which lack a WHERE clause. As this changes query > > behavior, I've made the new GUCs PGC_SUSET. > > > What say? > > -1. This is an express violation of the SQL standard, and at least the > UPDATE case has reasonable use-cases. Moreover, if your desire is to have > training wheels for SQL, there are any number of other well-known gotchas > that are just as dangerous, for example ye olde unintentionally-correlated > subselect: > https://www.postgresql.org/message-id/20160714135233.1410.92538%40wrigleys.postgresql.org I am hoping for a "novice" mode that issues warnings about possible bugs, e.g. unintentionally-correlated subselect, and this could be part of that. -- 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 +
Review of the patch in the commit fest: - Various naming/spelling inconsistencies: In the source, the module is require_where, the documentation titles it require-where,the GUC parameters are requires_where.*, but incorrectly documented. - Unusual indentation in the Makefile - Needs tests - Not sure about errcode(ERRCODE_CARDINALITY_VIOLATION), which is documented in the code as "this means something returnedthe wrong number of rows". I think ERRCODE_SYNTAX_ERROR or something from nearby there would be better. - errhint() string should end with a period. - The 7th argument of DefineCustomBoolVariable() is of type int, not bool, so passing false is somewhat wrong, even if itworks. - There ought to be a _PG_fini() function that undoes what _PG_init() does. - The documentation should be expanded and clarified. Given that this is a "training wheels" module, we can be extra clearhere. I would like to see some examples at least. - The documentation is a bit incorrect about the ways to load this module. shared_preload_libraries is not necessary. session_and local_ (with prep) should also work. - The claim in the documentation that only superusers can do things with this module is not generally correct. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 8/1/16 11:38 AM, Bruce Momjian wrote: > I am hoping for a "novice" mode that issues warnings about possible > bugs, e.g. unintentionally-correlated subselect, and this could be part > of that. Somewhat related; I've recently been wondering about a mode that disallows Const's in queries coming from specific roles. The idea there is to make it impossible for an application to pass a constant in, which would make it impossible for SQL injection to happen. With how magical modern frameworks/languages are, it's often impossible to enforce that at the application layer. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On Fri, Sep 09, 2016 at 09:57:21AM -0400, Peter Eisentraut wrote: > Review of the patch in the commit fest: > > - Various naming/spelling inconsistencies: In the source, the module > is require_where, the documentation titles it require-where, the GUC > parameters are requires_where.*, but incorrectly documented. Fixed. > - Unusual indentation in the Makefile Fixed. > - Needs tests Still needs some fixing. > - Not sure about errcode(ERRCODE_CARDINALITY_VIOLATION), which is > documented in the code as "this means something returned the wrong > number of rows". I think ERRCODE_SYNTAX_ERROR or something from > nearby there would be better. Changed to ERRCODE_SYNTAX_ERROR. CARDINALITY_VIOLATION was a bit too cute. > - errhint() string should end with a period. Fixed. > - The 7th argument of DefineCustomBoolVariable() is of type int, not > bool, so passing false is somewhat wrong, even if it works. Fixed. > - There ought to be a _PG_fini() function that undoes what _PG_init() > does. Fixed. > - The documentation should be expanded and clarified. Given that this > is a "training wheels" module, we can be extra clear here. I would > like to see some examples at least. Working on this. > - The documentation is a bit incorrect about the ways to load this > module. shared_preload_libraries is not necessary. session_ and > local_ (with prep) should also work. I'm not 100% sure I understand what you want here. I did manage to get the thing loaded without a restart via LOAD, but that's it so far. Will continue to poke at it. > - The claim in the documentation that only superusers can do things > with this module is not generally correct. I think that the claims are fixed. This is SUSET, at least in this patch, because anything short of that that changes query behavior seems incautious. 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
Attachment
On Mon, Sep 19, 2016 at 12:02 AM, David Fetter <david@fetter.org> wrote: >> - The claim in the documentation that only superusers can do things >> with this module is not generally correct. > > I think that the claims are fixed. This is SUSET, at least in this > patch, because anything short of that that changes query behavior > seems incautious. Uggh, I disagree strongly with that, as do lots of existing GUCs. I think it's for the superuser to decide whether this should be enabled by default (e.g. by setting it in postgresql.conf) and for individual users to decide whether they want to override the superuser's decision for particular sessions. Therefore, I think this should be PGC_USERSET. I think PGC_SUSET GUCs are pretty annoying, and we should have a really compelling reason why it's not OK for users to change the value of a setting before resorting to PGC_SUSET. For example, log_duration is PGC_SUSET and that makes sense because the log is "owned" by the administrator, not the individual user. But work_mem, for example, changes query behavior and that is PGC_USERSET. I think that's right. We have talked before about wanting a system that restricts the values to which users can legally set values which they are in principle allowed to change, and someday we might have that. In the meantime, letting regular users change settings that they don't like is, in general, a feature, not a bug. Someone who feels otherwise can, of course, hack up their own version of this module. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 9/19/16 12:02 AM, David Fetter wrote: >> - The claim in the documentation that only superusers can do things >> > with this module is not generally correct. > I think that the claims are fixed. This is SUSET, at least in this > patch, because anything short of that that changes query behavior > seems incautious. Your last patch, which I looked at, had them as USERSET. I think that is the right setting. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Sep 19, 2016 at 03:00:51PM -0400, Peter Eisentraut wrote: > On 9/19/16 12:02 AM, David Fetter wrote: > >> - The claim in the documentation that only superusers can do things > >> > with this module is not generally correct. > > I think that the claims are fixed. This is SUSET, at least in this > > patch, because anything short of that that changes query behavior > > seems incautious. > > Your last patch, which I looked at, had them as USERSET. I think that > is the right setting. Will work one up this evening that has that. 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
On Fri, Sep 09, 2016 at 09:57:21AM -0400, Peter Eisentraut wrote: > Review of the patch in the commit fest: > > - The documentation is a bit incorrect about the ways to load this > module. shared_preload_libraries is not necessary. session_ and > local_ (with prep) should also work. Would you be so kind as to describe how you got local_preload_libraries to work? I'm stuck on getting Makefile to realize that the hook should be installed in $libdir/plugins rather than $libdir itself. 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
On 9/20/16 3:04 PM, David Fetter wrote: > On Fri, Sep 09, 2016 at 09:57:21AM -0400, Peter Eisentraut wrote: >> Review of the patch in the commit fest: >> >> - The documentation is a bit incorrect about the ways to load this >> module. shared_preload_libraries is not necessary. session_ and >> local_ (with prep) should also work. > > Would you be so kind as to describe how you got > local_preload_libraries to work? I'm stuck on getting Makefile to > realize that the hook should be installed in $libdir/plugins rather > than $libdir itself. There is no Makefile support for that, and AFAICT, that particular feature is kind of obsolescent. I wouldn't worry about it too much. The main point was, there are multiple ways to load modules. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote: > > [training_wheels_004.patch] openjade:filelist.sgml:144:16:E: character "_" invalid: only parameter literal, "CDATA", "ENDTAG", "MD", "MS", "PI", "PUBLIC", "SDATA", "STARTTAG", "SYSTEM" and parameter separators allowed openjade:contrib.sgml:138:2:W: cannot generate system identifier for general entity "require" The documentation doesn't build here, I think because require_where is not an acceptable entity name. It works for me if I change the underscore to a minus in various places. That fixes these errors: + <para> + Here is an example showing how to set up a database cluster with + <literal>require_where</literal>. +<screen> +$ psql -U postgres +# SHOW shared_preload_libraries; /* Make sure not to clobber something by accident */ + +If you found something, +# ALTER SYSTEM SET shared_preload_libraries='the,stuff,you,found,require_where'; + +Otherwise, +# ALTER SYSTEM SET shared_preload_libraries='require_where'; + +Then restart <productname>PostgreSQL</productname> +</screen> + </para> Could use a full stop (period) on the end of that sentence. Also it shouldn't be inside the "screen" tags. Maybe "If you found something," and "Otherwise," shouldn't be either, or should somehow be marked up so as not to appear to be text from the session. postgres=# delete from foo; ERROR: DELETE requires a WHERE clause HINT: To delete all rows, use "WHERE true" or similar. Maybe one of those messages could use some indication of where this is coming from, for surprised users encountering this non-standard behaviour for the first time? FWIW I saw something similar enforced globally by the DBA team at a large company with many database users. I think experienced users probably initially felt mollycoddled when they first encountered the error but I'm sure that some were secretly glad of its existence from time to time... I think it's a useful feature for users who want it, and a nice little demonstration of how extensible Postgres is. -- Thomas Munro http://www.enterprisedb.com
On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > underscore to a minus in various places. That fixes these errors: Correction: s/these errors:/the above errors./ -- Thomas Munro http://www.enterprisedb.com
On Mon, Sep 26, 2016 at 5:48 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote:
>
> [training_wheels_004.patch]
openjade:filelist.sgml:144:16:E: character "_" invalid: only parameter
literal, "CDATA", "ENDTAG", "MD", "MS", "PI", "PUBLIC", "SDATA",
"STARTTAG", "SYSTEM" and parameter separators allowed
openjade:contrib.sgml:138:2:W: cannot generate system identifier for
general entity "require"
The documentation doesn't build here, I think because require_where is
not an acceptable entity name. It works for me if I change the
underscore to a minus in various places. That fixes these errors:
+ <para>
+ Here is an example showing how to set up a database cluster with
+ <literal>require_where</literal>.
+<screen>
+$ psql -U postgres
+# SHOW shared_preload_libraries; /* Make sure not to clobber
something by accident */
+
+If you found something,
+# ALTER SYSTEM SET
shared_preload_libraries='the,stuff,you,found,require_where' ;
+
+Otherwise,
+# ALTER SYSTEM SET shared_preload_libraries='require_where';
+
+Then restart <productname>PostgreSQL</productname>
+</screen>
+ </para>
Could use a full stop (period) on the end of that sentence. Also it
shouldn't be inside the "screen" tags. Maybe "If you found
something," and "Otherwise," shouldn't be either, or should somehow be
marked up so as not to appear to be text from the session.
postgres=# delete from foo;
ERROR: DELETE requires a WHERE clause
HINT: To delete all rows, use "WHERE true" or similar.
Maybe one of those messages could use some indication of where this is
coming from, for surprised users encountering this non-standard
behaviour for the first time?
+1.
I think hint message should be more clear about where its coming
from. May be it can specify the GUC name, or suggest that if you
really want to use DELETE without WHERE clause, turn OFF this
GUC? or something in similar line.
FWIW I saw something similar enforced globally by the DBA team at a
large company with many database users. I think experienced users
probably initially felt mollycoddled when they first encountered the
error but I'm sure that some were secretly glad of its existence from
time to time... I think it's a useful feature for users who want it,
and a nice little demonstration of how extensible Postgres is.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Rushabh Lathia
On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
It seems that the version of docbook that you get if you follow the instructions[1] on CentOS is OK with the underscore in entity names, but the version you get if you follow the instructions for macOS + MacPorts doesn't like it. I didn't investigate exactly which component or version was behind that, but it's clear that other entity names use hyphens instead of underscores, so I would recommend making this change to your patch so we don't change the version requirements for building the docs:
On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote:
>
> [training_wheels_004.patch]
openjade:filelist.sgml:144:16:E: character "_" invalid: only parameter
literal, "CDATA", "ENDTAG", "MD", "MS", "PI", "PUBLIC", "SDATA",
"STARTTAG", "SYSTEM" and parameter separators allowed
openjade:contrib.sgml:138:2:W: cannot generate system identifier for
general entity "require"
The documentation doesn't build here, I think because require_where is
not an acceptable entity name. It works for me if I change the
underscore to a minus in various places.
It seems that the version of docbook that you get if you follow the instructions[1] on CentOS is OK with the underscore in entity names, but the version you get if you follow the instructions for macOS + MacPorts doesn't like it. I didn't investigate exactly which component or version was behind that, but it's clear that other entity names use hyphens instead of underscores, so I would recommend making this change to your patch so we don't change the version requirements for building the docs:
diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 7ca62a4..48ca717 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -135,7 +135,7 @@ CREATE EXTENSION <replaceable>module_name</> FROM unpackaged;
&pgtrgm;
&pgvisibility;
&postgres-fdw;
- &require_where;
+ &require-where;
&seg;
&sepgsql;
&contrib-spi;
diff --git a/doc/src/sgml/filelist.sgml b/doc/src/sgml/filelist.sgml
index 8079cbd..4552273 100644
--- a/doc/src/sgml/filelist.sgml
+++ b/doc/src/sgml/filelist.sgml
@@ -141,7 +141,7 @@
<!ENTITY pgtrgm SYSTEM "pgtrgm.sgml">
<!ENTITY pgvisibility SYSTEM "pgvisibility.sgml">
<!ENTITY postgres-fdw SYSTEM "postgres-fdw.sgml">
-<!ENTITY require_where SYSTEM "require_where.sgml">
+<!ENTITY require-where SYSTEM "require_where.sgml">
<!ENTITY seg SYSTEM "seg.sgml">
<!ENTITY contrib-spi SYSTEM "contrib-spi.sgml">
<!ENTITY sepgsql SYSTEM "sepgsql.sgml">
Thomas Munro
http://www.enterprisedb.com
http://www.enterprisedb.com
On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
And I mean these instructions: [1] https://www.postgresql.org/docs/devel/static/docguide-toolsets.html
It seems that the version of docbook that you get if you follow the instructions[1] ...
And I mean these instructions: [1] https://www.postgresql.org/docs/devel/static/docguide-toolsets.html
Thomas Munro
http://www.enterprisedb.com
http://www.enterprisedb.com
On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote: >> > >> > [training_wheels_004.patch] >> >> [review] Ping. -- Thomas Munro http://www.enterprisedb.com
On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote: > On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: > > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro > > <thomas.munro@enterprisedb.com> wrote: > >> > >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote: > >> > > >> > [training_wheels_004.patch] > >> > >> [review] > > Ping. I'll have another revision out as soon as I get some more test cases. 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
On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote: > On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: > > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro > > <thomas.munro@enterprisedb.com> wrote: > >> > >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote: > >> > > >> > [training_wheels_004.patch] > >> > >> [review] > > Ping. Please find attached the next revision. 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
Attachment
On Thu, Sep 29, 2016 at 6:19 PM, David Fetter <david@fetter.org> wrote: > On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote: >> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro >> <thomas.munro@enterprisedb.com> wrote: >> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro >> > <thomas.munro@enterprisedb.com> wrote: >> >> >> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote: >> >> > >> >> > [training_wheels_004.patch] >> >> >> >> [review] >> >> Ping. > > Please find attached the next revision. I'm not sold on ERRCODE_SYNTAX_ERROR. There's nothing wrong with the syntax, since parsing succeeded. It would be cool if we could use ERRCODE_E_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED, though I'm not sure what error class 38 really means. Does require_where's hook function count as an 'external routine' and on that basis is it it allowed to raise this error? Thoughts, anyone? I am still seeing the underscore problem when building the docs. Please find attached fix-docs.patch which applies on top of training_wheels_005.patch. It fixes that problem for me. The regression test fails. The expected error messages show the old wording, so I think you forgot to add a file. Also, should contrib/require_where/Makefile define REGRESS = require_where? That would allow 'make check' from inside that directory, which is convenient and matches other extensions. Please find attached fix-regression-test.patch which also applies on top of training_wheels_005.patch and fixes those things for me, and also adds a couple of extra test cases. Robert Haas mentioned upthread that the authors section was too short, and your follow-up v3 patch addressed that. Somehow two authors got lost on their way to the v5 patch. Please find attached fix-authors.patch which also applies on top of training_wheels_005.patch to restore them. It would be really nice to be able to set this to 'Ready for Committer' in this CF. Do you want to post a v6 patch or are you happy for me to ask a committer to look at v5 + these three corrections? -- Thomas Munro http://www.enterprisedb.com
Attachment
On 30/09/2016 05:23, Thomas Munro wrote: > > It would be really nice to be able to set this to 'Ready for > Committer' in this CF. Do you want to post a v6 patch or are you > happy for me to ask a committer to look at v5 + these three > corrections? I just looked at the patch, and noticed that only plain DELETE and UPDATE commands are handled. Is it intended that writable CTE without WHERE clauses are not detected by this extension? I personally think that wCTE should be handled (everyone can forget a WHERE clause), but if not it should at least be documented. -- Julien Rouhaud http://dalibo.com - http://dalibo.org
Thomas Munro wrote: > On Thu, Sep 29, 2016 at 6:19 PM, David Fetter <david@fetter.org> wrote: > > Please find attached the next revision. > > I'm not sold on ERRCODE_SYNTAX_ERROR. There's nothing wrong with the > syntax, since parsing succeeded. It would be cool if we could use > ERRCODE_E_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED, though I'm not sure > what error class 38 really means. Does require_where's hook function > count as an 'external routine' and on that basis is it it allowed to > raise this error? Thoughts, anyone? I don't think it's appropriate to use class 38. "Part 1: Framework" saith An external routine is an SQL-invoked routine that references some compilation unit of a specified programming languagethat is outside the SQL-environment. The mechanism and time of binding of such a reference is implementation-defined. It seems to me that what matters here is that what the user is doing is an UPDATE, and the restriction is about it's SQL-written WHERE clause; not whether require_where module is written in SQL or not (which seems irrelevant to me). So this is not "external" IMO. But there's class 2F "SQL routine exception" which has 003 for "prohibited SQL-statement attempted" ... oh, and we even have that in errcodes.txt as ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED. Seems apropos. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Sep 30, 2016 at 06:37:17PM +0200, Julien Rouhaud wrote: > On 30/09/2016 05:23, Thomas Munro wrote: > > > > It would be really nice to be able to set this to 'Ready for > > Committer' in this CF. Do you want to post a v6 patch or are you > > happy for me to ask a committer to look at v5 + these three > > corrections? > > I just looked at the patch, and noticed that only plain DELETE and > UPDATE commands are handled. Is it intended that writable CTE without > WHERE clauses are not detected by this extension? I personally think > that wCTE should be handled (everyone can forget a WHERE clause), but if > not it should at least be documented. You are correct in that it should work for every unqualified UPDATE or DELETE, not just some. Would you be so kind as to send along the tests cases you used so I can add them to the patch? 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
On 30/09/2016 21:12, David Fetter wrote: > On Fri, Sep 30, 2016 at 06:37:17PM +0200, Julien Rouhaud wrote: >> On 30/09/2016 05:23, Thomas Munro wrote: >>> >>> It would be really nice to be able to set this to 'Ready for >>> Committer' in this CF. Do you want to post a v6 patch or are you >>> happy for me to ask a committer to look at v5 + these three >>> corrections? >> >> I just looked at the patch, and noticed that only plain DELETE and >> UPDATE commands are handled. Is it intended that writable CTE without >> WHERE clauses are not detected by this extension? I personally think >> that wCTE should be handled (everyone can forget a WHERE clause), but if >> not it should at least be documented. > > You are correct in that it should work for every unqualified UPDATE or > DELETE, not just some. Would you be so kind as to send along the > tests cases you used so I can add them to the patch? > Given your test case, these queries should fail if the related require_where GUCs are set (obviously last one should failed with either of the GUC set): WITH d AS (DELETE FROM test_require_where) SELECT 1; WITH u AS (UPDATE test_require_where SET t = t) SELECT 1; WITH d AS (DELETE FROM test_require_where), u AS (UPDATE test_require_where SET t = t) SELECT 1; -- Julien Rouhaud http://dalibo.com - http://dalibo.org
On 9/29/16 11:23 PM, Thomas Munro wrote: > The regression test fails. The expected error messages show the old > wording, so I think you forgot to add a file. Also, should > contrib/require_where/Makefile define REGRESS = require_where? That > would allow 'make check' from inside that directory, which is > convenient and matches other extensions. Please find attached > fix-regression-test.patch which also applies on top of > training_wheels_005.patch and fixes those things for me, and also adds > a couple of extra test cases. I don't think we need to have a separate data file to load a few test rows. A plain INSERT statement will do. > It would be really nice to be able to set this to 'Ready for > Committer' in this CF. Do you want to post a v6 patch or are you > happy for me to ask a committer to look at v5 + these three > corrections? As a committer, I would prefer a single patch to be posted. Before it gets there, I would still like to see the documentation expanded. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Oct 1, 2016 at 8:32 AM, Julien Rouhaud <julien.rouhaud@dalibo.com> wrote: > On 30/09/2016 21:12, David Fetter wrote: >> On Fri, Sep 30, 2016 at 06:37:17PM +0200, Julien Rouhaud wrote: >>> On 30/09/2016 05:23, Thomas Munro wrote: >>>> >>>> It would be really nice to be able to set this to 'Ready for >>>> Committer' in this CF. Do you want to post a v6 patch or are you >>>> happy for me to ask a committer to look at v5 + these three >>>> corrections? >>> >>> I just looked at the patch, and noticed that only plain DELETE and >>> UPDATE commands are handled. Is it intended that writable CTE without >>> WHERE clauses are not detected by this extension? I personally think >>> that wCTE should be handled (everyone can forget a WHERE clause), but if >>> not it should at least be documented. >> >> You are correct in that it should work for every unqualified UPDATE or >> DELETE, not just some. Would you be so kind as to send along the >> tests cases you used so I can add them to the patch? >> > > Given your test case, these queries should fail if the related > require_where GUCs are set (obviously last one should failed with either > of the GUC set): > > WITH d AS (DELETE FROM test_require_where) SELECT 1; > > WITH u AS (UPDATE test_require_where SET t = t) SELECT 1; > > WITH d AS (DELETE FROM test_require_where), u AS (UPDATE > test_require_where SET t = t) SELECT 1; Right. These cases work because they show up as CMD_DELETE/CMD_UPDATE: postgres=# set require_where.delete = on; SET postgres=# with answer as (select 42) delete from foo; ERROR: DELETE requires a WHERE clause when require_where.delete is set to on HINT: To delete all rows, use "WHERE true" or similar. postgres=# prepare x as delete from foo; ERROR: DELETE requires a WHERE clause when require_where.delete is set to on HINT: To delete all rows, use "WHERE true" or similar. But not this case which shows up as a CMD_SELECT: postgres=# set require_where.delete = on; SET postgres=# with q as (delete from foo) select 42; ┌──────────┐ │ ?column? │ ├──────────┤ │ 42 │ └──────────┘ (1 row) I guess you need something involving query_tree_walker or some other kind of recursive traversal if you want to find DELETE/UPDATE lurking in there. One option would be to document it as working for top level DELETE or UPDATE, and leave the recursive version as an improvement for a later patch. That's the most interesting kind to catch because that's what people are most likely to type directly into a command line. -- Thomas Munro http://www.enterprisedb.com
On Sat, Oct 1, 2016 at 5:08 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > Right. These cases work because they show up as CMD_DELETE/CMD_UPDATE: > > postgres=# set require_where.delete = on; > SET > postgres=# with answer as (select 42) delete from foo; > ERROR: DELETE requires a WHERE clause when require_where.delete is set to on > HINT: To delete all rows, use "WHERE true" or similar. > postgres=# prepare x as delete from foo; > ERROR: DELETE requires a WHERE clause when require_where.delete is set to on > HINT: To delete all rows, use "WHERE true" or similar. Is this patch able to handle the case of DML queries using RETURNING in COPY? Those are authorized since 9.6, and it has not been mentioned yet on this thread. Going through the patch quickly I guess that would not work. -- Michael
On Sat, Oct 1, 2016 at 5:08 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > I guess you need something involving query_tree_walker or some other > kind of recursive traversal if you want to find DELETE/UPDATE lurking > in there. > > One option would be to document it as working for top level DELETE or > UPDATE, and leave the recursive version as an improvement for a later > patch. That's the most interesting kind to catch because that's what > people are most likely to type directly into a command line. That would be a halfy-baked feature then, and the patch would finish by being refactored anyway if we support more cases in the future, because those will need a tree walker (think CTAS, INSERT SELECT, using WITH queries that contain DMLs)... Personally I think that it would be surprising if subqueries are not restrained. So I am -1 for the patch as-is, and let's come up with the most generic approach. Having more regression tests would be a good idea as well. I am marking the patch as returned with feedback. This CF has normally already ended. -- Michael
On Fri, Sep 30, 2016 at 04:23:13PM +1300, Thomas Munro wrote: > On Thu, Sep 29, 2016 at 6:19 PM, David Fetter <david@fetter.org> wrote: > > On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote: > >> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro > >> <thomas.munro@enterprisedb.com> wrote: > >> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro > >> > <thomas.munro@enterprisedb.com> wrote: > >> >> > >> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter <david@fetter.org> wrote: > >> >> > > >> >> > [training_wheels_004.patch] > >> >> > >> >> [review] > >> > >> Ping. > > > > Please find attached the next revision. > > I'm not sold on ERRCODE_SYNTAX_ERROR. There's nothing wrong with the > syntax, since parsing succeeded. It would be cool if we could use > ERRCODE_E_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED, though I'm not sure > what error class 38 really means. Does require_where's hook function > count as an 'external routine' and on that basis is it it allowed to > raise this error? Thoughts, anyone? > > I am still seeing the underscore problem when building the docs. > Please find attached fix-docs.patch which applies on top of > training_wheels_005.patch. It fixes that problem for me. > > The regression test fails. The expected error messages show the old > wording, so I think you forgot to add a file. Also, should > contrib/require_where/Makefile define REGRESS = require_where? That > would allow 'make check' from inside that directory, which is > convenient and matches other extensions. Please find attached > fix-regression-test.patch which also applies on top of > training_wheels_005.patch and fixes those things for me, and also adds > a couple of extra test cases. > > Robert Haas mentioned upthread that the authors section was too short, > and your follow-up v3 patch addressed that. Somehow two authors got > lost on their way to the v5 patch. Please find attached > fix-authors.patch which also applies on top of > training_wheels_005.patch to restore them. > > It would be really nice to be able to set this to 'Ready for > Committer' in this CF. Do you want to post a v6 patch or are you > happy for me to ask a committer to look at v5 + these three > corrections? Thanks! I've rolled your patches into this next one and clarified the commit message, as there appears to have been some confusion about the scope. 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
Attachment
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Michael Paquier
Date:
On Sun, Jan 1, 2017 at 12:34 PM, David Fetter <david@fetter.org> wrote: > I've rolled your patches into this next one and clarified the commit > message, as there appears to have been some confusion about the scope. Not all the comments have been considered! Here are some example.. =# set require_where.delete to on; SET =# copy (delete from aa returning a) to stdout; ERROR: 42601: DELETE requires a WHERE clause when require_where.delete is set to on HINT: To delete all rows, use "WHERE true" or similar. LOCATION: require_where_check, require_where.c:42 COPY with DML returning rows are correctly restricted. Now if I look at WITH clauses... =# with delete_query as (delete from aa returning a) select * from delete_query;a --- (0 rows) =# with update_query as (update aa set a = 3 returning a) select * from update_query;a --- (0 rows) Oops. That's not cool. CTAS also perform no checks: =# create table ab as with delete_query as (delete from aa returning a) select * from delete_query; SELECT 0 Is there actually a meaning to have two options? Couldn't we leave with just one? Actually, I'd just suggest to rip off the option and just to make the checks enabled when the library is loaded, to get a module as simple as passwordcheck. --- /dev/null +++ b/contrib/require_where/data/test_require_where.data @@ -0,0 +1,16 @@ +Four There is no need to load fake data as you need to just check the parsing of the query. So let's simplify the tests and remove that. Except that the shape of the module is not that bad. The copyright notices need to be updated to 2017, and it would be nice to have some comments at the top of require_where.c to describe what it does. -- Michael
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
From
Pavel Stehule
Date:
Hi
2016-07-21 6:57 GMT+02:00 David Fetter <david@fetter.org>:
Folks,
Please find attached a patch which makes it possible to disallow
UPDATEs and DELETEs which lack a WHERE clause. As this changes query
behavior, I've made the new GUCs PGC_SUSET.
What say?
Thanks to Gurjeet Singh for the idea and Andrew Gierth for the tips
implementing.
I am sending review of this patch
1. there are not any problem with patching, compiling, doc
2. the patch is simple, the documentation is good enough
3. all regress tests passed without problems
My questions:
1. is a data file really necessary? INSERT INTO test_require_where VALUES('aaa') has same effect.
2. There is not documented a assert "Assert(query->jointree != NULL)"
It is valid, but should be documented why?
Regards
Pavel
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
On Sun, Jan 01, 2017 at 07:57:33PM +0900, Michael Paquier wrote: > On Sun, Jan 1, 2017 at 12:34 PM, David Fetter <david@fetter.org> wrote: > > I've rolled your patches into this next one and clarified the commit > > message, as there appears to have been some confusion about the scope. > > Not all the comments have been considered! > > Here are some example.. > > =# set require_where.delete to on; > SET > =# copy (delete from aa returning a) to stdout; > ERROR: 42601: DELETE requires a WHERE clause when > require_where.delete is set to on > HINT: To delete all rows, use "WHERE true" or similar. > LOCATION: require_where_check, require_where.c:42 > > COPY with DML returning rows are correctly restricted. > > Now if I look at WITH clauses... as no one this patch was aimed at protecting will do... I get that there's something about this feature that introduces some oddnesses. This stage of it is not intended to be some kind of a general guard against anything. It's intended to put some safety measures in place for an extremely restricted subset of SQL which has caused real damage in real systems. I'd love it if everyone who ever touched a production system was wearing the entire parser, planner, and executor in their heads, but that's not the situation I'm trying to help with here. > =# with delete_query as (delete from aa returning a) select * from delete_query; > a > --- > (0 rows) > =# with update_query as (update aa set a = 3 returning a) select * > from update_query; > a > --- > (0 rows) > Oops. That's not cool. Those are protections for a later feature, if feasible. I'm happy to clarify the documentation and error messages as to the scope. > CTAS also perform no checks: Again, not in scope. > Is there actually a meaning to have two options? Couldn't we leave > with just one? Actually, I'd just suggest to rip off the option and > just to make the checks enabled when the library is loaded, to get a > module as simple as passwordcheck. Excellent suggestion. I'll see about that in the next couple of days. > +++ b/contrib/require_where/data/test_require_where.data > @@ -0,0 +1,16 @@ > +Four > There is no need to load fake data as you need to just check the > parsing of the query. So let's simplify the tests and remove that. OK > Except that the shape of the module is not that bad. The copyright > notices need to be updated to 2017, and it would be nice to have some > comments at the top of require_where.c to describe what it does. Will fix. 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
On Tue, Jan 03, 2017 at 11:59:19AM +0100, Pavel Stehule wrote: > Hi > I am sending review of this patch > > 1. there are not any problem with patching, compiling, doc > 2. the patch is simple, the documentation is good enough > 3. all regress tests passed without problems > > My questions: > > 1. is a data file really necessary? No. I'll remove it. > 2. There is not documented a assert "Assert(query->jointree != NULL)" > > It is valid, but should be documented why? Will do. 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
On Sun, Jan 01, 2017 at 07:57:33PM +0900, Michael Paquier wrote: > On Sun, Jan 1, 2017 at 12:34 PM, David Fetter <david@fetter.org> wrote: > > I've rolled your patches into this next one and clarified the commit > > message, as there appears to have been some confusion about the scope. > > Is there actually a meaning to have two options? Couldn't we leave > with just one? Actually, I'd just suggest to rip off the option and > just to make the checks enabled when the library is loaded, to get a > module as simple as passwordcheck. Done. > --- /dev/null > +++ b/contrib/require_where/data/test_require_where.data > @@ -0,0 +1,16 @@ > +Four > There is no need to load fake data as you need to just check the > parsing of the query. So let's simplify the tests and remove that. Removed. > Except that the shape of the module is not that bad. The copyright > notices need to be updated to 2017, and it would be nice to have some > comments at the top of require_where.c to describe what it does. Please find attached the next version of the patch including Pavel's suggestion that I provide some motivation for including those Asserts. 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
Attachment
On Wed, Jan 04, 2017 at 09:58:26PM -0800, David Fetter wrote: > On Sun, Jan 01, 2017 at 07:57:33PM +0900, Michael Paquier wrote: > > On Sun, Jan 1, 2017 at 12:34 PM, David Fetter <david@fetter.org> wrote: > > > I've rolled your patches into this next one and clarified the commit > > > message, as there appears to have been some confusion about the scope. > > > > Is there actually a meaning to have two options? Couldn't we leave > > with just one? Actually, I'd just suggest to rip off the option and > > just to make the checks enabled when the library is loaded, to get a > > module as simple as passwordcheck. > > Done. > > > --- /dev/null > > +++ b/contrib/require_where/data/test_require_where.data > > @@ -0,0 +1,16 @@ > > +Four > > There is no need to load fake data as you need to just check the > > parsing of the query. So let's simplify the tests and remove that. > > Removed. > > > Except that the shape of the module is not that bad. The copyright > > notices need to be updated to 2017, and it would be nice to have some > > comments at the top of require_where.c to describe what it does. > > Please find attached the next version of the patch including Pavel's > suggestion that I provide some motivation for including those > Asserts. Here's one with the commit message included for easier review. 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
Attachment
On 1/5/17 12:04 AM, David Fetter wrote:
> + errmsg("UPDATE requires a WHERE clause when require_where.delete is set to on"),
ISTM that message is no longer true.
The second if could also be an else if too.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
On Sun, Jan 08, 2017 at 06:50:12PM -0600, Jim Nasby wrote:
> On 1/5/17 12:04 AM, David Fetter wrote:
> > + errmsg("UPDATE requires a WHERE clause when require_where.delete is set to on"),
>
> ISTM that message is no longer true.
>
> The second if could also be an else if too.
I refactored this into one case and removed some fluff, some of which
was never needed, some of which is no longer.
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
Attachment
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATEand DELETE
From
Alvaro Herrera
Date:
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.
I think this should say "the \"require_hook\" extension" rather than
use the term "hook".
(There are two or three translatability rules violations in this
snippet, but since this is an extension and those are not translatable,
I won't say elaborate further.)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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.
>
> I think this should say "the \"require_hook\" extension" rather than
> use the term "hook".
>
> (There are two or three translatability rules violations in this
> snippet, but since this is an extension and those are not translatable,
> I won't say elaborate further.)
I'm happy to fix it. Are the rules all in one spot I can refer to?
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
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.
> 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
Attachment
On Tue, Jan 10, 2017 at 08:31:47AM -0800, David Fetter wrote:
> 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.
>
> > but since this is an extension and those are not translatable, I
> > won't say elaborate further.)
>
> "Not translatable," or "not currently translated?"
Oops^2. Correct patch attached and sent to correct list. :P
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