Re: Reduce build times of pg_trgm GIN indexes

Hi!

On Wed, 02 Sep 2026 at 11:16, David Geier <geidav(dot)pg(at)gmail(dot)com> wrote:
> Hi!
>
>> Thanks for this—looks like a good improvement.
>
> Thanks for reviewing the patch. Attached is v9 with all reviewing
> comments from below addressed.
>

Thanks for updating the patches.

>> 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()?
>
> Done. If there's no consensus that this optimization won't cause
> regressions we can also split it out from this patchset. I would then
> open a new thread with additional testing.
>
>> 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.
>
> Done. Also renamed FlipSign() to flip_sign() for consistency.
>
>> 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.
>
> Done.
>
>> 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.
>
> Done.
>
> I realized that one elog(ERROR) got removed and another one with a
> different message got added. I haven't updated the translation files
> because, judging from the git log messages, that is done separately.
>
> How to best go about complying to the existing code style? I've been
> under the impression that especially indentation is mostly fixed up
> retroactively by pgindent. Do you run pgindent on the patch set prior to
> submitting the patch?

I'm relying on the IDE's auto-formatting, which fixes most coding style issues.

The committer will run pgindent anyway [1] — so I didn't run it beforehand.

[1] https://wiki.postgresql.org/wiki/Committing_checklist

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.