Hi, David
Thanks for this—looks like a good improvement. On Tue, 01 Sep 2026 at 13:05, David Geier <[email protected]> wrote: > Hi! > > I've rebased the patch set on latest master. > > I'm hoping we can make some progress with the patch set, given that it > gives a huge performance improvement, allowing to create GIN indexes on > much bigger tables. > > @Heikki and @Matthias: anything specific missing from your point-of-view > that is blocking this patch set from moving forward? > >> Attached is the rebased patch set as well as a new patch that optimizes >> ginInsertBAEntries(). Performance improvements are as follows, measured >> with the same benchmark I used in the first mail of this thread. >> Runtimes and deltas are in milliseconds. >> >> Code | movies | delta | lineitem | delta >> -----------------------------------|--------|--------|------------------ >> master | 11,160 | - | 248,146 | - >> v7-0001-Make-btint4cmp-branchless | 9,509 | 1,651 | 236,760 | 11,386 >> v7-0002-Use-radix-sort | 6,123 | 3,386 | 214,632 | 22,128 >> v7-0003-Replace-RB-tree | 4,755 | 1,368 | 144,252 | 70,380 > > For details of the implementation see my previous mail. Here are some comments on v8 patches. v8-0001 ======= 1. @@ -194,12 +195,7 @@ btint4cmp(PG_FUNCTION_ARGS) int32 a = PG_GETARG_INT32(0); int32 b = PG_GETARG_INT32(1); - if (a > b) - PG_RETURN_INT32(A_GREATER_THAN_B); - else if (a == b) - PG_RETURN_INT32(0); - else - PG_RETURN_INT32(A_LESS_THAN_B); + PG_RETURN_INT32(pg_cmp_s32(a, b)); } While we are in this area, would it make sense to apply the same treatment to btint8cmp() using pg_cmp_s64()? v8-0002 ======= 1. +static inline unsigned char FlipSign(char x) Coding style nit: suggest formatting this as: +static inline unsigned char +FlipSign(char x) 2. +static void radix_sort_trigrams_signed(trgm *trg, int count) Same as above. 3. + for (int i=0; i<count; i++) + for (int j=0; j<3; j++) Spaces are required between operators and their operands. 4. + for (int i=2; i>=0; i--) + { + trgm *old_from = from; + trgm *next = to; + + for (int j=0; j<256; j++) + { + starts[j] = next; + next += freqs[i][j]; + } + + for (int j=0; j<count; j++) Same as above. v8-0003 ======= 1. +typedef struct GinHashKey { - GinEntryAccumulator *eo = (GinEntryAccumulator *) existing; - const GinEntryAccumulator *en = (const GinEntryAccumulator *) newdata; - BuildAccumulator *accum = (BuildAccumulator *) arg; + OffsetNumber attnum; + GinNullCategory category; + Datum key; +} GinHashKey; ... +typedef struct GinHashEntry +{ + GinHashKey hashkey; + uint32 hash; + char status; + ItemPointerData * items; + uint32 numItems; + uint32 allocatedItems; +} GinHashEntry; + +typedef struct GinSortEntry +{ + GinHashKey hashkey; + ItemPointerData * items; + uint32 numItems; +} GinSortEntry; Since this patch introduces new typedefs, GinHashKey, GinHashEntry and GinSortEntry, typedefs.list should probably be updated as well. 2. + ItemPointerData * items; This is inconsistent with our coding style. 3. -typedef struct GinEntryAccumulator -{ - RBTNode rbtnode; - Datum key; - GinNullCategory category; - OffsetNumber attnum; - bool shouldSort; - ItemPointerData *list; - uint32 maxcount; /* allocated size of list[] */ - uint32 count; /* current number of list[] entries */ -} GinEntryAccumulator; Remove GinEntryAccumulator from typedefs.list as well. > > -- > David Geier -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
