MIME-Version: 1.0
References: <78574B24-BE0A-42C5-8075-3FA9FA63B8FC@amazon.com>
 <CAEze2WjjOg+gE1VUZ2Omd-26MniaY6-UJghqzLZMHpVkDEUy8w@mail.gmail.com>
 <DBA06D41-32C8-45D5-A3FA-15A412692351@amazon.com> <CAEze2WgyAUtW64eP8uxZmHUy-RfBtuiKaUybvi=Jc3bwgg3g4Q@mail.gmail.com>
 <EA519A89-282D-4535-88C6-C79588FF1DA8@amazon.com> <CAEze2WjCJokQbBOQqK0qJFw0H=cu27XB=hrgQOhy9TQ=41RPmg@mail.gmail.com>
 <238EEE41-B206-4590-8C20-DA52C25A2291@amazon.com> <D50C3248-FF76-4E6C-B891-B6E147F56A44@amazon.com>
 <51C77060-059F-4BB3-8EAF-83F08656F6D2@amazon.com>
In-Reply-To: <51C77060-059F-4BB3-8EAF-83F08656F6D2@amazon.com>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Thu, 6 Mar 2025 00:39:03 +0100
Message-ID: <CAEze2WjJqjyJk-7XBtiYBovJMg5tLZkS5MGii56oCOa1Z+MKWQ@mail.gmail.com>
Subject: Re: Expanding HOT updates for expression and partial indexes
To: "Burd, Greg" <gregburd@amazon.com>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAEze2WjJqjyJk-7XBtiYBovJMg5tLZkS5MGii56oCOa1Z%2BMKWQ%40mail.gmail.com>
Precedence: bulk

On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd@amazon.com> wrote:
>
> Hello,
>
> I've rebased and updated the patch a bit.  The biggest change is that the=
 performance penalty measured with v1 of this patch is essentially gone in =
v10.  The overhead was due to re-creating IndexInfo information unnecessari=
ly, which I found existed in the estate.  I've added a few fields in IndexI=
nfo that are not populated by default but necessary when checking expressio=
n indexes, those fields are populated on demand and only once limiting thei=
r overhead.

This review is based on a light reading of patch v10. I have not read
all 90kB, and am unlikely to finish a full review soon:

> * assumes estate->es_result_relations[0] is the ResultRelInfo being updat=
ed

I'm not sure that's a valid assumption. I suspect it might be false in
cases of nested updates, like

$ UPDATE table1 SET value =3D other.value FROM (UPDATE table2 SET value
=3D 2 ) other WHERE other.id =3D table1.id;

If this table1 or table2 has expression indexes I suspect it may
result in this assertion failing (but I haven't spun up a server with
the patch).
Alternatively, please also check that it doesn't break if any of these
two tables is partitioned with multiple partitions (and/or has
expression indexes, etc.).

> * uses ri_IndexRelationInfo[] from within estate rather than re-creating =
it

As I mentioned above, I think it's safer to pass the known-correct RRI
(known by callers of table_tuple_update) down the stack.

> * augments IndexInfo only when needed for testing expressions and only on=
ce

ExecExpressionIndexesUpdated seems to always loop over all indexes,
always calling AttributeIndexInfo which always updates the fields in
the IndexInfo when the index has only !byval attributes (e.g. text,
json, or other such varlena types). You say it happens only once, have
I missed something?

I'm also somewhat concerned about the use of typecache lookups on
index->rd_opcintype[i], rather than using
TupleDescCompactAttr(index->rd_att, i); the latter of which I think
should be faster, especially when multiple wide indexes are scanned
with various column types. In hot loops of single-tuple update
statements I think this may make a few 0.1%pt difference - not a lot,
but worth considering.

> * only creates a local old/new TupleTableSlot when not present in estate

I'm not sure it's safe for us to touch that RRI's tupleslots.

> * retains existing summarized index HOT update logic

Great, thanks!

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)