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