Thread

PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-21T04:57:46Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Amit Kapila <amit.kapila16@gmail.com> — 2016-07-21T07:49:11Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Jim Mlodgenski <jimmy76@gmail.com> — 2016-07-21T13:21:55Z

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.
https://github.com/uptimejp/sql_firewall

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Craig Ringer <craig@2ndquadrant.com> — 2016-07-21T13:29:12Z

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.

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-21T13:49:33Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Joshua D. Drake <jd@commandprompt.com> — 2016-07-21T13:59:06Z

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.

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Teodor Sigaev <teodor@sigaev.ru> — 2016-07-21T15:20:37Z

> 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/

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-21T16:39:08Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Robert Haas <robertmhaas@gmail.com> — 2016-07-21T16:46:29Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-21T16:46:33Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Abhijit Menon-Sen <ams@2ndquadrant.com> — 2016-07-21T16:49:51Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Robert Haas <robertmhaas@gmail.com> — 2016-07-21T16:51:50Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-21T16:52:26Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-21T16:53:56Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Jim Nasby <jim.nasby@bluetreble.com> — 2016-07-21T21:48:37Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-21T21:57:33Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-22T06:38:43Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Robert Haas <robertmhaas@gmail.com> — 2016-07-26T03:12:24Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-26T03:38:27Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Robert Haas <robertmhaas@gmail.com> — 2016-07-26T20:39:14Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Vik Fearing <vik@2ndquadrant.fr> — 2016-07-26T22:14:51Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-27T00:22:01Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-07-27T01:15:33Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Vik Fearing <vik@2ndquadrant.fr> — 2016-07-27T01:24:28Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-27T01:41:28Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-27T04:11:04Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Vik Fearing <vik@2ndquadrant.fr> — 2016-07-27T12:59:17Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Peter van Hardenberg <pvh@pvh.ca> — 2016-07-27T15:11:23Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-07-28T21:53:07Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Bruce Momjian <bruce@momjian.us> — 2016-08-01T16:38:21Z

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 +

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-09-09T13:57:21Z

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 returned the 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 it works.

- 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 clear here.  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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Jim Nasby <jim.nasby@bluetreble.com> — 2016-09-11T03:08:58Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-09-19T04:02:54Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Robert Haas <robertmhaas@gmail.com> — 2016-09-19T18:54:35Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-09-19T19:00:51Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-09-19T19:46:57Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-09-20T19:04:03Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-09-20T19:24:18Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-26T00:18:11Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-26T00:20:46Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Rushabh Lathia <rushabh.lathia@gmail.com> — 2016-09-26T04:10:41Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-26T04:11:56Z

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]
>
> 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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-26T04:12:50Z

On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro <thomas.munro@enterprisedb.com
> wrote:

> 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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-28T22:12:11Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-09-28T22:27:35Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-09-29T05:19:02Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-30T03:23:13Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Julien Rouhaud <julien.rouhaud@dalibo.com> — 2016-09-30T16:37:17Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-09-30T18:19:07Z

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 language that 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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2016-09-30T19:12:32Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Julien Rouhaud <julien.rouhaud@dalibo.com> — 2016-09-30T19:32:29Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-09-30T19:50:17Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Thomas Munro <thomas.munro@enterprisedb.com> — 2016-09-30T20:08:44Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Michael Paquier <michael.paquier@gmail.com> — 2016-10-01T11:19:06Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Michael Paquier <michael.paquier@gmail.com> — 2016-10-01T12:52:25Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-01T03:34:00Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Michael Paquier <michael.paquier@gmail.com> — 2017-01-01T10:57:33Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Pavel Stehule <pavel.stehule@gmail.com> — 2017-01-03T10:59:19Z

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
>

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-03T16:09:18Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-03T16:12:06Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-05T05:58:26Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-05T06:04:12Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-09T00:50:12Z

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)

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-09T22:36:56Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-01-09T22:52:11Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-09T22:54:05Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-10T16:31:47Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-01-10T17:00:01Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Pavel Stehule <pavel.stehule@gmail.com> — 2017-01-11T14:55:43Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Michael Paquier <michael.paquier@gmail.com> — 2017-02-01T05:06:35Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Bruce Momjian <bruce@momjian.us> — 2017-02-02T15:16:29Z

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.

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-02-02T15:18:45Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Bruce Momjian <bruce@momjian.us> — 2017-02-02T15:34:43Z

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 +

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-02-02T16:04:15Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Pavel Stehule <pavel.stehule@gmail.com> — 2017-02-02T16:21:03Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-02-02T16:40:17Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-02T17:14:10Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

David Fetter <david@fetter.org> — 2017-02-02T17:24:51Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Nico Williams <nico@cryptonector.com> — 2017-02-02T17:50:24Z

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
--

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Rod Taylor <rod.taylor@gmail.com> — 2017-02-05T01:08:30Z

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: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Michael Paquier <michael.paquier@gmail.com> — 2017-02-05T01:33:37Z

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

Re: PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

Robert Haas <robertmhaas@gmail.com> — 2017-03-16T16:01:03Z

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