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>
In-Reply-To: <CAOj6k6fDKg4EOpzmO1sJYARwyu8dkmX-BFCEymQc_6okw9ORVw@mail.gmail.com>
From: Dharin Shah <dharinshah95@gmail.com>
Date: Thu, 15 Jan 2026 21:50:54 +0100
Message-ID: <CAOj6k6e7fw8RjAWXc04_A=sg4=jsU0CK7Qi13fwkPW5hMz6a5w@mail.gmail.com>
Subject: Re: [Patch] Add WHERE clause support to REFRESH MATERIALIZED VIEW
To: Adam Brusselback <adambrusselback@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000005c301e06487362cb"
Archived-At: <https://www.postgresql.org/message-id/CAOj6k6e7fw8RjAWXc04_A%3Dsg4%3DjsU0CK7Qi13fwkPW5hMz6a5w%40mail.gmail.com>
Precedence: bulk

--0000000000005c301e06487362cb
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

>
> (repro_include_issue.sql)


Typo fix : test_include_bug.sql (attached file)

Thanks,
Dharin

On Thu, Jan 15, 2026 at 7:46=E2=80=AFPM Dharin Shah <dharinshah95@gmail.com=
> wrote:

> Hey Adam,
>
> Apologies for the delay, and as promised on discord, I did a review of th=
e
> current patch (cf 6305) and wanted to share findings that line up with th=
e
> thread=E2=80=99s design discussion, plus one additional correctness bug t=
hat I
> could reproduce.
>
> 1. In the non-concurrent REFRESH ... WHERE .... path, the UPSERT SQL is
> built using the unique index metadata. The code currently uses indnatts
> when building the ON Conflict (...) target list. That includes INCLUDE
> columns, so for an index like:
>
> CREATE UNIQUE INDEX ON mv(id) INCLUDE (extra);
> the generated statement becomes effectively ON CONFLICT (id, extra) ...,
> which fails with:
> ERROR: there is no unique or exclusion constraint matching the ON CONFLIC=
T
> specification
>
> The fix appears straightforward: use indnkeyatts (key attributes only)
> when generating the conflict target, and also when deciding which columns
> are =E2=80=9Ckey=E2=80=9D for the UPDATE SET clause. I=E2=80=99ve attache=
d a minimal repro SQL
> script (repro_include_issue.sql)
>
> 2. Another small test quality issue: the regression script has a comment
> =E2=80=9CSubqueries -> Error=E2=80=9D but the expected output shows no er=
ror for the
> schema-qualified subquery. There is no explicit check forbidding subqueri=
es
> in transformRefreshWhereClause(), so schema-qualified subqueries appear
> allowed.
>
> Moving on to broader questions
>
>
>
>> I believe the issue is that the DELETE -> INSERT strategy leaves a
>> consistency gap. Since we relied on ROW EXCLUSIVE locks to allow concurr=
ent
>> reads, the moment we delete the rows, we lose the physical lock on them.=
 If
>> a concurrent transaction inserts a colliding row during that gap, the
>> materialized view ends up inconsistent with the base query (or hits a
>> constraint violation).
>
>
> Consistency gap in the non-concurrent mode matches what I=E2=80=99d expec=
t: with
> ROW EXCLUSIVE you allow concurrent readers/writers, and a pure DELETE =E2=
=86=92
> INSERT approach can create a window where the old tuple is gone and a
> concurrent session can insert a conflicting logical row.
>
> That said, I think it would help the patch to explicitly define the
> intended safety model:
> 1. Is the goal to be safe against concurrent DML on base tables only
> (i.e., refresh sees a snapshot and updates MV accordingly), or also to be
> safe against concurrent partial refreshes and direct writes to the MV (wh=
en
> maintenance is enabled)?
> 2. Should the non-concurrent partial refresh be =E2=80=9Cbest effort=E2=
=80=9D like normal
> DML (user coordinates), or should it be =E2=80=9Cmaintenance-like=E2=80=
=9D (serialized /
> logically safe by default)?
>
> If the intent is =E2=80=9Csafe by default=E2=80=9D, I=E2=80=99d encourage=
 documenting very clearly
> what=E2=80=99s guaranteed, and adding regression/README-style notes for f=
ootguns
>
> From a reviewer standpoint, I think the feature concept is sound and
> valuable, but it needs a crisp statement of semantics and safety
> boundaries. The tricky part is exactly what you called out: incremental
> refresh implies concurrency questions that aren=E2=80=99t present with fu=
ll rebuild
> + strong locks.
>
> I=E2=80=99m happy to keep reviewing iterations (especially around the adv=
isory
> lock approach), and I=E2=80=99ll attach the reproduction scripts and note=
s I used.
>
> As a possible staging approach: it might be simplest to start with a
> conservative serialization model for non-concurrent WHERE (while still
> allowing readers), and then iterate toward finer-grained logical locking
> if/when needed for throughput.
>
>
> Thanks,
> Dharin
>
>
> On Sun, Jan 4, 2026 at 3:56=E2=80=AFAM Adam Brusselback <adambrusselback@=
gmail.com>
> wrote:
>
>> Hi all,
>>
>> I've been running some more concurrency tests against this patch
>> (specifically looking for race conditions), and I found a flaw in the
>> implementation for the  REFRESH ... WHERE ... mode (without CONCURRENTLY=
).
>>
>> I believe the issue is that the DELETE -> INSERT strategy leaves a
>> consistency gap. Since we relied on ROW EXCLUSIVE locks to allow concurr=
ent
>> reads, the moment we delete the rows, we lose the physical lock on them.=
 If
>> a concurrent transaction inserts a colliding row during that gap, the
>> materialized view ends up inconsistent with the base query (or hits a
>> constraint violation).
>>
>> I initially was using SELECT ... FOR UPDATE to lock the rows before
>> modification, but that lock is (now that I know) obviously lost when the
>> row is deleted.
>>
>> My plan is to replace that row-locking strategy with transaction-level
>> advisory locks inside the refresh logic:
>>
>> Before the DELETE, run a SELECT pg_advisory_xact_lock(mv_oid,
>> hashtext(ROW(unique_keys)::text)) for the rows matching the WHERE clause=
.
>>
>> This effectively locks the "logical" ID of the row, preventing concurren=
t
>> refreshes on the same ID even while the physical tuple is temporarily go=
ne.
>> Hash collisions should not have any correctness issues that I can think =
of.
>>
>> However, before I sink time into implementing that fix:
>>
>> Is there general interest in having REFRESH MATERIALIZED VIEW ... WHERE
>> ... in core?
>> If the community feels this feature is a footgun or conceptually wrong
>> for Postgres, I'd rather know now before spending more time on this.
>>
>> If the feature concept is sound, does the advisory lock approach seem
>> like the right way to handle the concurrency safety here?
>>
>> Thanks,
>> Adam Brusselback
>>
>

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

<div dir=3D"ltr"><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px =
0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">(repro_i=
nclude_issue.sql)</blockquote><div><br></div><div>Typo fix : test_include_b=
ug.sql (attached file)<br><br></div><div>Thanks,<br>Dharin</div></div><br><=
div class=3D"gmail_quote gmail_quote_container"><div dir=3D"ltr" class=3D"g=
mail_attr">On Thu, Jan 15, 2026 at 7:46=E2=80=AFPM Dharin Shah &lt;<a href=
=3D"mailto:dharinshah95@gmail.com">dharinshah95@gmail.com</a>&gt; wrote:<br=
></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;=
border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir=3D"ltr">H=
ey Adam,<div><br></div><div>Apologies for the delay, and as promised on dis=
cord, I did a review of the current patch (cf 6305) and wanted to share fin=
dings that line up with the thread=E2=80=99s design discussion, plus one ad=
ditional correctness bug that I could reproduce.</div><div><br>1. In the no=
n-concurrent REFRESH ... WHERE .... path, the UPSERT SQL is built using the=
 unique index metadata. The code currently uses indnatts when building the =
ON Conflict (...) target list. That includes INCLUDE columns, so for an ind=
ex like:</div><div><br></div><div>CREATE UNIQUE INDEX ON mv(id) INCLUDE (ex=
tra);</div><div>the generated statement becomes effectively ON CONFLICT (id=
, extra) ..., which fails with:</div><div>ERROR: there is no unique or excl=
usion constraint matching the ON CONFLICT specification</div><div><div><br>=
</div><div>The fix appears straightforward: use indnkeyatts (key attributes=
 only) when generating the conflict target, and also when deciding which co=
lumns are =E2=80=9Ckey=E2=80=9D for the UPDATE SET clause. I=E2=80=99ve att=
ached a minimal repro SQL script (repro_include_issue.sql)</div><div><br></=
div><div>2. Another small test quality issue: the regression script has a c=
omment =E2=80=9CSubqueries -&gt; Error=E2=80=9D but the expected output sho=
ws no error for the schema-qualified subquery. There is no explicit check f=
orbidding subqueries in transformRefreshWhereClause(), so schema-qualified =
subqueries appear allowed.</div><div><div><br>Moving on to broader question=
s=C2=A0</div></div><div><br></div><div>=C2=A0</div><blockquote class=3D"gma=
il_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,2=
04,204);padding-left:1ex">I believe the issue is that the DELETE -&gt; INSE=
RT strategy leaves a consistency gap. Since we relied on ROW EXCLUSIVE lock=
s to allow concurrent reads, the moment we delete the rows, we lose the phy=
sical lock on them. If a concurrent transaction inserts a colliding row dur=
ing that gap, the materialized view ends up inconsistent with the base quer=
y (or hits a constraint violation).</blockquote><div><br></div><div>Consist=
ency gap in the non-concurrent mode matches what I=E2=80=99d expect: with R=
OW EXCLUSIVE you allow concurrent readers/writers, and a pure DELETE =E2=86=
=92 INSERT approach can create a window where the old tuple is gone and a c=
oncurrent session can insert a conflicting logical row.=C2=A0</div><div><br=
></div><div>That said, I think it would help the patch to explicitly define=
 the intended safety model:<br>1. Is the goal to be safe against concurrent=
 DML on base tables only (i.e., refresh sees a snapshot and updates MV acco=
rdingly), or also to be safe against concurrent partial refreshes and direc=
t writes to the MV (when maintenance is enabled)?<br>2. Should the non-conc=
urrent partial refresh be =E2=80=9Cbest effort=E2=80=9D like normal DML (us=
er coordinates), or should it be =E2=80=9Cmaintenance-like=E2=80=9D (serial=
ized / logically safe by default)?<br></div><div><br></div><div>If the inte=
nt is =E2=80=9Csafe by default=E2=80=9D, I=E2=80=99d encourage documenting =
very clearly what=E2=80=99s guaranteed, and adding regression/README-style =
notes for footguns=C2=A0</div><div><br></div><div>From a reviewer standpoin=
t, I think the feature concept is sound and valuable, but it needs a crisp =
statement of semantics and safety boundaries. The tricky part is exactly wh=
at you called out: incremental refresh implies concurrency questions that a=
ren=E2=80=99t present with full rebuild + strong locks.<br><br>I=E2=80=99m =
happy to keep reviewing iterations (especially around the advisory lock app=
roach), and I=E2=80=99ll attach the reproduction scripts and notes I used.<=
/div><div><br></div><div>As a possible staging approach: it might be simple=
st to start with a conservative serialization model for non-concurrent WHER=
E (while still allowing readers), and then iterate toward finer-grained log=
ical locking if/when needed for throughput.</div><div><br></div><div><br></=
div><div>Thanks,</div><div>Dharin</div><br></div></div><br><div class=3D"gm=
ail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Sun, Jan 4, 2026 at 3:5=
6=E2=80=AFAM Adam Brusselback &lt;<a href=3D"mailto:adambrusselback@gmail.c=
om" target=3D"_blank">adambrusselback@gmail.com</a>&gt; wrote:<br></div><bl=
ockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-lef=
t:1px solid rgb(204,204,204);padding-left:1ex"><div dir=3D"ltr">Hi all,<br>=
<br>I&#39;ve been running some more concurrency tests against this patch (s=
pecifically looking for race conditions), and I found a flaw in the impleme=
ntation for the=C2=A0 REFRESH ... WHERE ... mode (without CONCURRENTLY).<br=
><br>I believe the issue is that the DELETE -&gt; INSERT strategy leaves a =
consistency gap. Since we relied on ROW EXCLUSIVE locks to allow concurrent=
 reads, the moment we delete the rows, we lose the physical lock on them. I=
f a concurrent transaction inserts a colliding row during that gap, the mat=
erialized view ends up inconsistent with the base query (or hits a constrai=
nt violation).<br><br>I initially was using SELECT ... FOR UPDATE to lock t=
he rows before modification, but that lock is (now that I know) obviously l=
ost when the row is deleted.<br><br>My plan is to replace that row-locking =
strategy with transaction-level advisory locks inside the refresh logic:<br=
><br>Before the DELETE, run a SELECT pg_advisory_xact_lock(mv_oid, hashtext=
(ROW(unique_keys)::text)) for the rows matching the WHERE clause.<br><br>Th=
is effectively locks the &quot;logical&quot; ID of the row, preventing conc=
urrent refreshes on the same ID even while the physical tuple is temporaril=
y gone. Hash collisions should not have any correctness issues that I can t=
hink of.<br><br>However, before I sink time into implementing that fix:<br>=
<br>Is there general interest in having REFRESH MATERIALIZED VIEW ... WHERE=
 ... in core?<br>If the community feels this feature is a footgun or concep=
tually wrong for Postgres, I&#39;d rather know now before spending more tim=
e on this.<br><br>If the feature concept is sound, does the advisory lock a=
pproach seem like the right way to handle the concurrency safety here?<br><=
br>Thanks,<br>Adam Brusselback</div>
</blockquote></div>
</blockquote></div>

--0000000000005c301e06487362cb--