Hi!
On Wed, 02 Sep 2026 at 11:16, David Geier <[email protected]> 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.
