Re: Reduce build times of pg_trgm GIN indexes

Hi, David

Thanks for this—looks like a good improvement.

On Tue, 01 Sep 2026 at 13:05, David Geier <geidav(dot)pg(at)gmail(dot)com> 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.