public inbox for [email protected]  

help / color / mirror / Atom feed

From: Burd, Greg <[email protected]>
To: Matthias van de Meent <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Expanding HOT updates for expression and partial indexes
Date: Thu, 6 Mar 2025 12:40:22 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAEze2WjJqjyJk-7XBtiYBovJMg5tLZkS5MGii56oCOa1Z+MKWQ@mail.gmail.com>
References: <[email protected]>
	<CAEze2WjjOg+gE1VUZ2Omd-26MniaY6-UJghqzLZMHpVkDEUy8w@mail.gmail.com>
	<[email protected]>
	<CAEze2WgyAUtW64eP8uxZmHUy-RfBtuiKaUybvi=Jc3bwgg3g4Q@mail.gmail.com>
	<[email protected]>
	<CAEze2WjCJokQbBOQqK0qJFw0H=cu27XB=hrgQOhy9TQ=41RPmg@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAEze2WjJqjyJk-7XBtiYBovJMg5tLZkS5MGii56oCOa1Z+MKWQ@mail.gmail.com>


> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <[email protected]> wrote:
> 
> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <[email protected]> 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 unnecessarily, which I found existed in the estate.  I've added a few fields in IndexInfo that are not populated by default but necessary when checking expression indexes, those fields are populated on demand and only once limiting their 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 updated
> 
> 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 = other.value FROM (UPDATE table2 SET value
> = 2 ) other WHERE other.id = 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.).

Valid, and possible.  I'll check and find a way to pass along the known-correct RRI index into that array.

>> * 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.

I think passing the known-correct RRI index is the way to go as I need information from both ri_IndexRelationInfo/Desc[] arrays.

>> * augments IndexInfo only when needed for testing expressions and only once
> 
> 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?

There's a test that avoids doing it more than once, but I'm going to rename this as BuildExpressionIndexInfo() and call it from ExecOpenIndices() if there are expressions on the index.  I think that's cleaner and there's precedent for it in the form of BuildSpeculativeIndexInfo().

> 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.

I was just working on that.  Good idea.

>> * 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.

Me neither, that's why I mentioned it.  It was my attempt to avoid the work to create/destroy temp slots over and over that led to that idea.  It's working, but needs more thought.

>> * retains existing summarized index HOT update logic
> 
> Great, thanks!
> 
> Kind regards,
> 
> Matthias van de Meent
> Neon (https://neon.tech)

I might widen this patch a bit to include support for testing equality of index tuples using custom operators when they exist for the index.  In the use case I'm solving for we use a custom operator for equality that is not the same as a memcmp().  Do you have thoughts on that?  It may be hard to accomplish this as the notion of an equality operator is specific to the index access method and not well-defined outside that AFAICT.  If that's the case I'd have to augment the definition of an index access method to provide that information.

-greg

view thread (6+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Expanding HOT updates for expression and partial indexes
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox