Sorry for the delay. Spent some time over the last couple weeks on this. Status while I finish splitting v3 up.
I'll just say, I was humbled by how much there was to think about security-wise that I didn't have in mind at all when I started this project. The CF entry was sitting in PG20-1, which has closed, so I've moved it to PG20-2. Both of the things Zsolt raised are implemented in the next version. A caller holding only MAINTAIN gets 42501 for a predicate whose functions aren't all leakproof, and an error partway through no longer leaves the MV modification restrictions cleared. Working on that turned up one more in the same area. The predicate is evaluated inside the maintenance window, so a predicate function isn't only running as the owner, it's running with MV write protection switched off and can write to any MV in the database, not just the one being refreshed. That exemption is scoped to the target MV now. Also fixed: a correlated subquery in the predicate never worked at all. The predicate was deparsed against the MV's real name, but the statements the refresh runs alias it, so anything referring to the outer row got "missing FROM-clause entry". It deparses against the alias now. There are tests for the variants I could think of: EXISTS, IN, scalar subqueries, LATERAL, CTEs, set operations. One more came out of writing those tests. MAINTAIN doesn't imply SELECT, and the row count a refresh reports is an answer about which rows the predicate matched. So a caller with MAINTAIN and no SELECT could write WHERE some_column = 'x' and binary search a column they can't read. DELETE and UPDATE already require SELECT on the columns their WHERE clause reads, so this does the same now. Two scope reductions in v3. The first breaks statements that worked in v2, so anyone who tested it should read this bit. WHERE requires CONCURRENTLY now. In v2 the two forms picked two different implementations: bare REFRESH ... WHERE did the new direct modification, and REFRESH CONCURRENTLY ... WHERE went through match/merge. v3 keeps the direct modification, drops match/merge, and maps direct modification to CONCURRENTLY. So calling REFRESH MATERIALIZED VIEW mv WHERE ... now errors asking for the keyword, and adding it gets you what the bare form used to do. I had tested ways to improve performance under a coarser lock level and found no easy wins, so removing that path seemed like the best way forward, considering there is parallel work going on [1]. A partial refresh is now refused on an MV with more than one unique index. The upsert can't delete before it inserts, so any value moving between rows on a second unique index fails, and widening the predicate doesn't help. I tried letting the duplicate key error happen when there is a second index...but ended up with deadlocks across sessions on disjoint id ranges. The pre-lock orders on the arbiter key, and a second index adds an ordering nothing controls. While looking at [1], which is partly about the duplicate precheck in refresh_by_match_merge(), I ran its cases against v3. After this patch CONCURRENTLY selects between two implementations: match/merge without a WHERE clause, and direct modification with one. Mostly the implementations agreed, and where they didn't it was mine that was wrong. The upsert applies source rows one at a time against the arbiter index, so two source rows sharing a key can't both be represented, and the second was silently overwriting the first. A full refresh has rejected that for duplicates with no NULLs all along, and a partial one took them without a word. Fixed, following the index for which rows conflict, so two NULL keys are still not duplicates unless the index says NULLS NOT DISTINCT. Giuliano's original case errors on the partial path now too. If the precheck on that thread settles on a different condition, this should follow it rather than drift. Last thing, and the part I'd most like other eyes on. We now walk into the subquery and allow it if the caller could have read those relations themselves: SELECT on the columns the predicate reads, not through a security_invoker view, and not subject to RLS policies. That last one is why an ACL check isn't enough. The predicate runs as the owner, who is exempt from policies on their own tables, so a caller who is subject to them would learn about rows the policies exist to hide. Leakproof is now required for functions unless the caller is the owner. I was hoping that would only hit custom predicate functions and leave predicates over built-in operators working. I was wrong. numeric, jsonb, enum, array and row comparisons aren't marked leakproof, so WHERE amt > 100 needs ownership and WHERE id > 100 doesn't. Arithmetic isn't marked, so WHERE a + b > 100 does. Aggregates aren't, so count(*) in a subquery does. Nothing in contrib is marked, so citext equality does too. And the check runs on the parsed condition rather than a folded one, so x > '1.0' is accepted where x > 1.0::float8 is refused. I don't think the leakproof requirement can go right now. A subquery declares what it reads in its range table, so the walk above can judge it. A function body is opaque, and Zsolt's case is a STABLE wrapper around a VOLATILE body, so leakproof is the only thing that refuses it. But everything I just listed is something built in, or superuser installed, and none of it is code a caller could have written, which is the risk leakproof is standing in for here. Is "not leakproof" the right proxy for "caller-supplied code running as the owner" when it also catches every numeric comparison in the catalog? Is there a better way to handle this? It makes the "MAINTAIN" refresh way less useful. Adam [1] https://www.postgresql.org/message-id/flat/40d694df-39fd-4a4a-9459-9d6489165f60%40gogi.tv
