> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent > <boekewurm+postg...@gmail.com> wrote: > > On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregb...@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 >> 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
