MIME-Version: 1.0
References: 
 <CAMjNa7eFzTQ5=oZMQiB2bMkez5KP4A77JC7SRjeVEkOrh7cUHw@mail.gmail.com>
 <CACLU5mST1LhC3ibaKGNch_=06S2cmbjR4PnoUSupKs+rtgdeyw@mail.gmail.com>
 <CAMjNa7fJUwcOxf+qV8g+tCQ-3E-YAiKgE_Qs6u-xjdxe12T0SQ@mail.gmail.com>
 <CAMjNa7egcgUMf2tdQ1qeTYj1J1bBvyth3thoZPioujusFsBd4Q@mail.gmail.com>
 <CAOj6k6fDKg4EOpzmO1sJYARwyu8dkmX-BFCEymQc_6okw9ORVw@mail.gmail.com>
 <CAOj6k6e7fw8RjAWXc04_A=sg4=jsU0CK7Qi13fwkPW5hMz6a5w@mail.gmail.com>
In-Reply-To: 
 <CAOj6k6e7fw8RjAWXc04_A=sg4=jsU0CK7Qi13fwkPW5hMz6a5w@mail.gmail.com>
From: Adam Brusselback <adambrusselback@gmail.com>
Date: Mon, 23 Mar 2026 10:23:43 -0400
Message-ID: 
 <CAMjNa7esgLwWU8fmPU7V_AM6iLg1dRmpiAufL-Q9jdWpopfTNg@mail.gmail.com>
Subject: Re: [Patch] Add WHERE clause support to REFRESH MATERIALIZED VIEW
To: Dharin Shah <dharinshah95@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000002fd14e064db1c975"
Archived-At: 
 <https://www.postgresql.org/message-id/CAMjNa7esgLwWU8fmPU7V_AM6iLg1dRmpiAufL-Q9jdWpopfTNg%40mail.gmail.com>
Precedence: bulk

--0000000000002fd14e064db1c975
Content-Type: text/plain; charset="UTF-8"

Hi Dharin, thanks for the review.

> 1. indnatts vs indnkeyatts

Good catch. Will fix.

> 2. Subqueries -> Error

That comment is wrong, I never added a check for that because it turned out
to be unnecessary. Will remove.

> 3. Concurrency gap / safety model

To answer your questions directly:

1. The goal is to be safe against concurrent partial refreshes on
overlapping rows, not just concurrent DML on base tables.
2. The intent is maintenance-like and safe by default.

Because we lose the physical lock on the row after the DELETE, I plan to
enforce that safety default via transaction-level advisory locks acquired
before the DELETE with somethin like:

    SELECT pg_advisory_xact_lock(matviewOid, hashtext(ROW(key_cols)::text))
    FROM matview
    WHERE (condition);

Concurrent refreshes on the same logical rows will serialize while
non-overlapping rows still run in parallel.

This also made me think about whether the CONCURRENTLY keyword is doing the
right thing here. Here's how the guarantees break down across all the
refresh modes:

Refresh Command / State                 | Base Table Lock  | Concurrent
Reads? | Concurrent Writes? | Same-Row Concurrent Refreshes
----------------------------------------+------------------+-------------------+--------------------+------------------------------
Standard Full Refresh                   | ACCESS EXCLUSIVE | Blocked
    | Blocked            | Blocked (Table Level)
CONCURRENTLY (Full)                     | EXCLUSIVE        | Allowed
    | Blocked            | Blocked (Table Level)
Partial (WHERE) - Current Patch         | ROW EXCLUSIVE    | Allowed
    | Allowed            | Race condition (Fails)
Partial (WHERE) - With Advisory Locks   | ROW EXCLUSIVE    | Allowed
    | Allowed            | Serialized (Waits)
Partial (CONCURRENTLY WHERE)            | EXCLUSIVE        | Allowed
    | Blocked            | Serialized (Waits)

Because of this, the `CONCURRENTLY` distinction gets inverted with a
`WHERE` clause. With a full refresh, `CONCURRENTLY` is the more permissive
option (allowing readers). But here, the bare `WHERE` path allows both
reads and writes, while `CONCURRENTLY WHERE` blocks writers. Non-concurrent
ends up being the more permissive option, which goes against what the
keyword generally implies.

One option is to swap the two implementations to restore that intuition.
`CONCURRENTLY WHERE` becomes the advisory locks approach (maximum
throughput), and bare `WHERE` becomes the diff approach (conservative,
blocks writers). On the other hand, `CONCURRENTLY` has historically meant
the diff-based algorithm specifically, not just a lower lock level.

I don't have a strong opinion here and would rather let the community
decide. The updated patch will leave the algorithms as-is for now. Happy to
swap them if that's the preferred direction.

Will post an updated patch soon.

Thanks,
Adam Brusselback

--0000000000002fd14e064db1c975
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Dharin, thanks for the review.<br><br>&gt; 1. indnatts =
vs indnkeyatts<br><br>Good catch. Will fix.<br><br>&gt; 2. Subqueries -&gt;=
 Error<br><br>That comment is wrong, I never added a check for that because=
 it turned out to be unnecessary. Will remove.<br><br>&gt; 3. Concurrency g=
ap / safety model<br><br>To answer your questions directly:<br><br>1. The g=
oal is to be safe against concurrent partial refreshes on overlapping rows,=
 not just concurrent DML on base tables.<br>2. The intent is maintenance-li=
ke and safe by default.<br><br>Because we lose the physical lock on the row=
 after the DELETE, I plan to enforce that safety default via transaction-le=
vel advisory locks acquired before the DELETE with somethin like:<br><br>=
=C2=A0 =C2=A0 SELECT pg_advisory_xact_lock(matviewOid, hashtext(ROW(key_col=
s)::text))<br><div>=C2=A0 =C2=A0 FROM matview=C2=A0</div><div>=C2=A0 =C2=A0=
 WHERE (condition);</div><br>Concurrent refreshes on the same logical rows =
will serialize while non-overlapping rows still run in parallel.<br><br>Thi=
s also made me think about whether the CONCURRENTLY keyword is doing the ri=
ght thing here. Here&#39;s how the guarantees break down across all the ref=
resh modes:<br><br>Refresh Command / State =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 | Base Table Lock =C2=A0| Concurrent Reads? | Conc=
urrent Writes? | Same-Row Concurrent Refreshes<br>-------------------------=
---------------+------------------+-------------------+--------------------=
+------------------------------<br>Standard Full Refresh =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | ACCESS EXCLUSIVE | Blocked =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | Blocked =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0| Blocked (Table Level)<br>CONCURRENTLY (Full) =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | EXCLUSIVE =C2=A0 =
=C2=A0 =C2=A0 =C2=A0| Allowed =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | Blocked =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| Blocked (Table Level)<br>Partial=
 (WHERE) - Current Patch =C2=A0 =C2=A0 =C2=A0 =C2=A0 | ROW EXCLUSIVE =C2=A0=
 =C2=A0| Allowed =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | Allowed =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0| Race condition (Fails)<br>Partial (WHERE) - W=
ith Advisory Locks =C2=A0 | ROW EXCLUSIVE =C2=A0 =C2=A0| Allowed =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 | Allowed =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0| Serialized (Waits)<br>Partial (CONCURRENTLY WHERE) =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0| EXCLUSIVE =C2=A0 =C2=A0 =C2=A0 =C2=A0| Allowed =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | Blocked =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0| Serialized (Waits)<br><br>Because of this, the `CONCURRENTLY=
` distinction gets inverted with a `WHERE` clause. With a full refresh, `CO=
NCURRENTLY` is the more permissive option (allowing readers). But here, the=
 bare `WHERE` path allows both reads and writes, while `CONCURRENTLY WHERE`=
 blocks writers. Non-concurrent ends up being the more permissive option, w=
hich goes against what the keyword generally implies.<br><br>One option is =
to swap the two implementations to restore that intuition. `CONCURRENTLY WH=
ERE` becomes the advisory locks approach (maximum throughput), and bare `WH=
ERE` becomes the diff approach (conservative, blocks writers). On the other=
 hand, `CONCURRENTLY` has historically meant the diff-based algorithm speci=
fically, not just a lower lock level.<br><br>I don&#39;t have a strong opin=
ion here and would rather let the community decide. The updated patch will =
leave the algorithms as-is for now. Happy to swap them if that&#39;s the pr=
eferred direction.<br><br>Will post an updated patch soon.<br><br>Thanks,<b=
r>Adam Brusselback</div>

--0000000000002fd14e064db1c975--