On Wed, Feb 14, 2018 at 9:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
The reasoning behind the INSERT ON CONFLICT DO UPDATE approach when it comes to RLS is that we specifically didn't want to end up "losing" data- an INSERT which doesn't actually INSERT a row (or take the UPDATE action if the row already exists) ends up throwing that data away even though clearly the user expected us to do something with it, which is why we throw an error in that case instead.
For my part, at least, it seems like MERGE would likewise possibly be throwing away data that the user is expecting to be incorporated if no action is taken due to RLS and that then argues that we should be throwing an error in such a case, similar to the INSERT ON CONFLICT DO UPDATE case.
Thanks for confirming the approach. That matches my own thinking too.
Here is an updated v16a patch. This now supports RLS based on what we agreed above. I've added a few test cases to confirm that RLS works correctly with MERGE. We can more later if needed.
This version now also adds support for OVERRIDING clause in the INSERT statement. The whole var referencing issue pointed out by Peter is also fixed by this version. So to summarise the following things are now working:
- Partitioning
- Subqueries in WHEN AND quals and UPDATE targetlists
- Row level security (documentation changes are pending)
- OVERRIDING clause
- Various bugs reported by Peter are fixed (I haven't double checked if all issues are addressed or not, but we should be fairly close)
- Issues so far reported by Andreas Seltenreich as part of sqlsmith testing are fixed
I haven't yet addressed Tomas's review comment on using WAL position for write detection. I'm waiting for Simon to come back from holidays before doing anything there. One idea is to use some variant of contain_mutable_functions() and throw error during query planning, but not do anything during execution.
Other than that, I think we are getting to a point where the patch is mostly feature complete. We still need to decide the concurrent execution behaviour, but hopefully code changes there will be limited (unless we try to do something very invasive, which I hope we don't).
I'm not entirely happy with the way we're going about resolving column references in INSERT/UPDATE targetlist and subsequently what we are doing in setrefs.c. I'll continue to look for improvements there and also hoping to get suggestions from this list.