Re: [Patch] Add WHERE clause support to REFRESH MATERIALIZED VIEW

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