Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.600.51.1.1\))
Subject: Re: Tepid: selective index updates for heap relations
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <84a886d1-37af-4da2-ad1b-15cea9c7e965@app.fastmail.com>
Date: Tue, 28 Jul 2026 11:36:54 +0800
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8736715E-C8F7-4D47-95C6-3B02ED4F699A@gmail.com>
References: <af02b486-6adc-494c-a357-1dd72f655dcf@app.fastmail.com>
 <CAPpHfdvOLrULXQn+s+bEG4QiVrps+_3mg2EjUN39OYUKoBE20w@mail.gmail.com>
 <97d5e5a3-f8e2-49ec-bd52-5b43e5d2b30a@app.fastmail.com>
 <b078fb42-cecb-4868-8a65-11f9ce484868@app.fastmail.com>
 <3dc5d57e-6c77-4713-b97e-bf0a5a891226@app.fastmail.com>
 <ed69804a-aa09-489b-af3f-6c804b2efd94@app.fastmail.com>
 <efe30c88-cf01-4c48-b76e-777547babcea@app.fastmail.com>
 <ae6f4b6e-ec56-40b9-b770-a1f3036e2196@app.fastmail.com>
 <CALj2ACV3YaHh4zxaOFkNqKeQzkX0-uW1ghRd4LwdZsOjRB1T4Q@mail.gmail.com>
 <84a886d1-37af-4da2-ad1b-15cea9c7e965@app.fastmail.com>
To: Greg Burd <greg@burd.me>
Archived-At: 
 <https://www.postgresql.org/message-id/8736715E-C8F7-4D47-95C6-3B02ED4F699A%40gmail.com>
Precedence: bulk


> On Jul 21, 2026, at 05:34, Greg Burd <greg@burd.me> wrote:
>=20
>=20
> On Fri, Jul 17, 2026, at 4:12 PM, Bharath Rupireddy wrote:
>> Hi,
>>=20
>> On Thu, Jul 16, 2026 at 3:05=E2=80=AFAM Greg Burd <greg@burd.me> =
wrote:
>>>=20
>>> Rebased onto 3cf5264557b to address conflicts, no other changes.
>>=20
>> Thanks for working on this. I previously played around with PHOT and
>> WARM a bit and built some context, but I need to refresh my memory =
and
>> give things a re-read.
>=20
> Thanks for looking at it!
>=20
>> The patches cover a lot. Reducing the diff to the core and posting =
the
>> stats, amcheck, and logical replication patches later would make
>> review easier. I managed to get through 0001 and 0002 so far.
>=20
> Yes, the 0001 and 0002 work is really a direct re-posting of the same =
two patches on my cf-5556 work that moves HeapDetermineColumnsInfo() =
into the executor.  That was necessary work for the "HOT for Expression =
Indexes" but even more so for this work because this involves changing =
the contract between the table AM and the index AM as goverened by the =
executor to allow for cases other than TU_All/Summarizing/None.
>=20
> Yes, it is dense and I should try to chop it up a bit more.  I think =
at this stage it the viability of the idea hinges on the format changes =
and their long term implications.  If those aren't accepted (and I'm on =
the fence myself on the "bit14" idea) then I have to get other =
foundational changes in before this could fly so I wasn't super =
concerned about having the final patch layout.
>=20
> The tight integration of the TID abstraction across the table AM, the =
index AM and the executor is something that is hard to ignore and hard =
to work around.  The TID abstraction is a heap specific thing (IMO) that =
works very well but is a very leaky and at times a highly limiting one.
>=20
>> I would be more interested in focusing the initial discussion on the
>> key design decisions. Where to store the modified index bitmap (it is
>> per-row version). How we get vacuuming right. How bitmap scans and
>> index scans produce correct results. How on-disk row format changes
>> are acceptable. How upgrades work for migrating existing databases.
>> How replication (logical and streaming) works and any impact on
>> downstream systems (replicas, subscribers, etc.). How it impacts
>> tooling (in-core like amcheck, pg_upgrade, pageinspect, and external
>> ones).
>=20
> Yes, all good topics.
>=20
>> Most importantly, the trade-offs. We are going to have more heap =
bloat
>> at the cost of less index bloat, which is fine since most OLTP
>> workloads go via indexes, but does it impact analytical workloads =
that
>> scan the heap? How does it impact vacuum performance? Do we know how
>> many customers in practice modify indexed columns (not exact numbers,
>> but some data points to keep the motivation up for this feature)? The
>> fact that the modified index column bitmap is now stored per tuple
>> version means fewer rows fit per page, which means more pages per
>> relation, and does it also mean fewer HOT updates because updated =
rows
>> cannot fit in the same page since some space is used for storing the
>> bitmap, causing more index maintenance in turn?
>=20
> Yes, there are trade-offs to call out.  The heap pages will a) have =
more meta-data on them in the form of the bitmaps on UPDATE that =
collapse into dead tuples and b) yes there will be longer redirect =
chains that are harder to fully collect during vacuum and they will =
prevent pages from being marked frozen more frequently.  That is true, =
and that has a cost.
>=20
>> Also, the naming (SIU, Tepid, HOT-INDEXED, HEAP_INDEXED_UPDATED, and
>> previously PHOT, WARM) is a bit confusing to me. Could we simplify =
the
>> names and use them consistently?
>=20
> I agree, and naming is hard.  Happy to take on any ideas here.  I've =
been calling it "tepid" and explaining it as "selective index updates" =
and in a way that is "partially HOT (PHOT)" but in other ways the tepid =
model isn't a "heap-only tuple (HOT)" at all, it's not heap-only it is =
heap and SOME indexes but not ALL.  IDK what to call it, but maybe =
that's something to work out if/when the idea(s) and trade-offs are =
accepted and we're finalizing the code for commit?
>=20
>> Some quick comments on the patches.
>>=20
>> 0001:
>>=20
>> 1/ Can 0001 be discussed in a separate thread? It seems to provide
>> good coverage for HOT updates on its own and is worth discussing and
>> perhaps getting committed separately.
>=20
> This was pre-amble for changes that I'd planned for the HOT for =
Expressions patches.  It's not strictly necessary in tepid, I should =
just drop it.
>=20
>> 2/
>>=20
>> +SELECT id FROM hot_xml_test WHERE xpath('/person/name/text()', doc) =
=3D
>> ARRAY['Alice2'::text];
>> +ERROR:  operator does not exist: xml[] =3D text[]
>> +LINE 1: ..._xml_test WHERE xpath('/person/name/text()', doc) =3D =
ARRAY['A...
>> +                                                             ^
>> +DETAIL:  No operator of that name accepts the given argument types.
>> +HINT:  You might need to add explicit type casts.
>>=20
>> +INSERT INTO hot_xml_test VALUES
>> +    (1, '<person><name>Alice</name><age>30</age></person>'),
>> +    (2, '<person><name>Bob</name><age>25</age></person>');
>> +ERROR:  could not identify a comparison function for type xml
>> +SELECT * FROM get_hot_count('hot_xml_test');
>>=20
>> Are these expected?
>=20
> Yeah no, my mistake.  The more I looked at the XML tests the more they =
didn't really add value so I've removed them.
>=20
>> 0002:
>>=20
>> 1/
>> -SELECT * FROM base_tbl;
>> +SELECT * FROM base_tbl ORDER BY a;
>>=20
>> ERROR:  cannot insert a non-DEFAULT value into column "b"
>> DETAIL:  Column "b" is a generated column.
>> -SELECT * FROM gtest1v;
>> +SELECT * FROM gtest1v ORDER BY a;
>>=20
>> -DELETE FROM main_view WHERE a IN (20,21);
>> +DELETE FROM main_view WHERE a =3D 20 AND b =3D 31;
>> NOTICE:  main_view BEFORE DELETE STATEMENT (before_view_del_stmt)
>> NOTICE:  main_view INSTEAD OF DELETE ROW (instead_of_del)
>> -NOTICE:  OLD: (21,10)
>> -NOTICE:  main_view INSTEAD OF DELETE ROW (instead_of_del)
>> NOTICE:  OLD: (20,31)
>> +NOTICE:  main_view AFTER DELETE STATEMENT (after_view_del_stmt)
>> +DELETE 1
>>=20
>> The commit message says this fixes nondeterministic behavior in
>> existing tests due to row ordering. I think these are unrelated to
>> this work and could be discussed and committed separately.
>=20
> Sure, possibly.  The instability became apparent when working on the =
first two patches.
>=20
>> 2/ ExecUpdateModifiedIdxAttrs() replaces HeapDetermineColumnsInfo().
>> Why do we need to move modified index attribute computation to the
>> executor and make every TTS and table AM pay that cost? HOT and
>> modified index attributes are purely heap AM specific. If the =
executor
>> ever needs the list of modified index attributes, why not let the AMs
>> provide it as an out parameter (similar to how we pass the HOT hint =
in
>> TU_UpdateIndexes format)?
>=20
> The contract between the index and table AMs is governed by the the =
executor, or it should be (IMO), and the TU_All/Summarizing/None model =
that exists now is very much a heap-ism (leaky, too heap-MVCC specific) =
and so breaks down in the face of any table AM that has a different MVCC =
model.  IMO the executor should be defaulting to only updating the =
indexes that are impacted by an update as a rule but allow for table AMs =
to influence that.  Why?  Because a table-agnostic executor should have =
as a goal not updating indexes unless those updates are required by the =
modifications underway.  Doing more than that amount of work as a rule =
is assuming things the executor shouldn't know about in the table and/or =
index implementations.  So, the executor (after patch 0002) will only =
update indexes that overlap with the modified attributes.
>=20
>> I read the commit message saying that finding this set of attributes
>> is not heap-specific but more general to all table AMs and could
>> inform other decisions about when index inserts are required. But is
>> it needed for this feature? If not, I think it can be discussed
>> separately.
>=20
> Yes, how else is the heap supposed to be able to communicate which =
subset of indexes to update in a generic manner?  I tried other methods =
and they all felt (to me) like they were working around a leaky =
abstraction rather than fixing the root cause.  Your view may differ.
>=20
>> 3/
>> - SELECT FROM injection_points_detach('heap_update-before-pin');
>> - SELECT FROM injection_points_wakeup('heap_update-before-pin');
>> + SELECT FROM =
injection_points_detach('simple_heap_update-before-pin');
>> + SELECT FROM =
injection_points_wakeup('simple_heap_update-before-pin');
>>=20
>> Once we find the need for 0002, can we just leave the injection point
>> name as-is to reduce the mechanical diff?
>=20
> I can re-try and find out if it matters.
>=20
>> 4/ Nits.
>> Typos.
>> + * are in the UPDATE statment and are known to be referenced by at =
least one
>> + * ExecGetAllUpdatedCols().  Desipte the name it provides the set of
>=20
> Fixed.
>=20
>> No need to specify test names in the comments, because they can =
change anytime.
>=20
> I disagree this time because the comment calls out a very very subtle =
hidden issue that is induced by that test and I felt should be =
documented so the next hacker could understand and avoid it earlier in =
the process than I did.
>=20
>> * heap_modifiy_tuple(). There is one test in tsearch.sql that does =
just
>> + * that, modifies an indexed attribute that isn't specified in the =
SQL and
>>=20
>> 5/
>> + /* attidx is zero-based, attrnum is the normal attribute number */
>> + AttrNumber attrnum =3D attidx + FirstLowInvalidHeapAttributeNumber;
>>=20
>> Is every TTS implementer expected to support all system columns that
>> FirstLowInvalidHeapAttributeNumber implies? Asking because 0002 moved
>> this code to the executor in ExecCompareSlotAttrs.
>=20
> Interesting point.  Yes, I did move a heap-ism into the executor =
(facepalm), something I claim to be against.  :)
>=20
> I'm going to take the abstraction critique seriously and move the =
mechanism of "which attributes changed" behind the slot/table-AM =
boundary, while keeping the policy "only maintain indexes whose =
attributes overlap the change" in the executor.
>=20
> Concretely: ExecCompareSlotAttrs() as written leaks heap assumptions =
upward (it enumerates over FirstLowInvalidHeapAttributeNumber and =
hand-handles system columns like tableoid), which is fair to call out. =
The comparison of two versions of a row is something the AM should =
answer for its own attributes and its own notion of "a version changed"; =
the executor should only collect that changed-set and intersect it with =
each index's attribute set to decide which indexes need fresh entries. =
That split is the honest form of "the executor governs the =
table-AM/index-AM contract, the table AM owns its storage mechanics."
>=20
> The reason I can't instead adopt the suggested out-parameter model, =
have the AM return modified_attrs from table_tuple_update(), is a matter =
of ordering, and I think it's the same ordering that a couple of the =
concurrency questions in the thread are also tripping on, so it's worth =
stating plainly:
>=20
> modified_attrs is an input to table_tuple_update(), not a byproduct of =
it. The heap AM reads it during the update to decide, before it writes =
anything, (a) whether the update can stay HOT / selectively-indexed vs. =
must update all indexes (HeapUpdateHotAllowable), (b) the tuple lock =
mode (HeapUpdateDetermineLockmode), and (c) whether the replica-identity =
key changed. The executor then uses the same set after the update to =
drive which indexes get fresh entries. A value returned from the update =
call would arrive after every one of those decisions had already been =
made it's structurally too late to inform them. So the changed-attribute =
set has to be available to, and passed into, the table AM, not produced =
by it.
>=20
> The clean way to satisfy both constraints that comes to mind is a =
slot/AM-level comparison the executor calls to build the set (AM owns =
"what changed"), then passes into the update (executor owns "which =
indexes"). That keeps the decision points in the right order and removes =
the heap-ism from the executor. To that end I've added to the table AM: =
table_modified_attrs() callback in an updated 0002 and in heap =
heapam_modified_attrs() in a new 0003 patch.
>=20
> (Separately and related to that: this is also why the "reduces =
buffer-lock hold time" line in the 0002 commit message is both wrong and =
misleading the computation was always pre-lock and I'm dropping it.)
>=20
>> 6/ The commit message says that having ExecUpdateModifiedIdxAttrs() =
in
>> the executor reduces the time the buffer lock is held by computing
>> modified index columns before table_tuple_update(). How is this
>> correct from a concurrency perspective? If another transaction
>> modifies the same tuple between when the executor compares columns =
and
>> when heap_update acquires the buffer lock, what happens? The executor
>> locks the old tuple explicitly only when it detects concurrent =
updates
>> or deletes to the same tuple, but does not hold the tuple lock the
>> first time.
>=20
> Good catch, and the commit message is misleading here I'll reword it. =
The "reduces the time the buffer lock is held" line oversells a benefit =
that isn't real and, worse, invites exactly the concurrency worry you =
raised. In today's tree  HeapDetermineColumnsInfo() already runs before =
heap_update() takes the buffer lock (it's computed up front from =
oldtup/newtup), so moving the computation into the executor doesn't =
change when, relative to the buffer lock, the comparison happens. The =
motivation for the move is the table-AM/index-AM contract, not lock-hold =
latency; I'll drop that paragraph so it stops implying otherwise.
>=20
> On the concurrency correctness itself, there's no window to exploit:
>=20
> - ExecUpdateAct() computes modified_attrs from (oldSlot, newSlot) and =
immediately calls table_tuple_update() in the same function, with no =
lock dropped and no yield between the two. There is no "compare, then =
later acquire the buffer lock" gap =E2=80=94 the comparison is not =
protected by, nor waiting on, any buffer lock, in either the old or new =
arrangement.
>=20
> - Concurrency is still detected exactly where it always was: inside =
heap_update(), under the buffer lock, via the xmax/visibility check. If =
another transaction updated the row first, heap_update() returns =
TM_Updated (unchanged by this patch).
>=20
> - On TM_Updated, ExecUpdate() runs the normal EPQ path: =
table_tuple_lock() the latest version, EvalPlanQual(), re-fetch the =
latest oldSlot and rebuild the new slot, then goto redo_act =E2=80=94 =
which re-enters ExecUpdateAct() and recomputes modified_attrs against =
the fresh versions.
>=20
> - modified_attrs is consumed only in ExecUpdateEpilogue() (the =
index-insert step), which runs only on the TM_Ok path. A modified_attrs =
computed against a version that lost the race is discarded and =
recomputed before any index maintenance happens.
>=20
> So the set of indexes we maintain is always derived from the same =
(old, new) pair the update was actually applied to; a concurrent update =
forces a re-read and recompute before index inserts, identical in effect =
to how HeapDetermineColumnsInfo() behaves today. The executor doesn't =
need to hold a tuple lock across the comparison for the same reason it =
doesn't today =E2=80=94 the AM's TM_Updated/EPQ protocol is the =
serialization point.
>=20
> I'll fix the injection-point/comment nits and drop the misleading =
commit-message paragraph in the next revision.
>=20
>> I will continue reading the other patches in the coming weeks.
>>=20
>> --
>> Bharath Rupireddy
>> Amazon Web Services: https://aws.amazon.com
>=20
> best.
>=20
> -greg<v67-tepid.tgz>

Thanks for the patch.

I have spent some time reviewing v67 0001-0009. git am failed at 0010, =
so a rebase is needed.

I think I have only got a rough idea of what this feature does and how =
it works so far. I believe I will still need much more time to =
understand the details.

I just want to raise a few design comments I have so far:

* Does it make sense to add a table-level option to make this feature =
opt-in? =46rom my understanding, this feature will benefit tables that =
have many indexes across multiple fields. Although benchmarks are =
attached, I am afraid they may not be able to simulate all users' data =
models. Making it opt-in would allow users to easily evaluate the =
feature with their own tables and enable it only on tables that really =
benefit from it. I realize that turning it on is easy, but turning it =
off may require table rewritten and index rebuilds. However, I think =
that is acceptable, as many other ALTER TABLE operations may also =
require so.

* When an index entry points to a stale tuple chain, it still returns =
the entry and sets xs_hot_indexed_stale, which means callers of =
index_getnext_slot() need to check this new state. This seems to weaken =
the current index scan API contract. Is there any thought behind =
exposing this state to callers instead of filtering stale entries =
internally?

I need to stop here to handle some other work. I will come back with =
more comments later.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/