Re: Reduce build times of pg_trgm GIN indexes

Hi!

> Thanks for this—looks like a good improvement.

Thanks for reviewing the patch. Attached is v9 with all reviewing
comments from below addressed.

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

--
David Geier