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

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