Am Mittwoch, dem 10.01.2024 um 08:00 +0800 schrieb jian he:
> you do `CREATE INDEX ON pgbench_accounts USING gist(aid);` > but the original patch didn't change contrib/btree_gist/btree_int4.c > So I doubt your benchmark is related to the original patch. > or maybe I missed something. > The patch originally does this: +ALTER OPERATOR FAMILY gist_int4_ops USING gist ADD + FUNCTION 11 (int4, int4) btint4sortsupport (internal) ; This adds sortsupport function to int4 as well. We reuse existing btint4sortsupport() function, so no need to change btree_int4.c. > also per doc: > ` > sortsupport > Returns a comparator function to sort data in a way that preserves > locality. It is used by CREATE INDEX and REINDEX commands. The > quality > of the created index depends on how well the sort order determined by > the comparator function preserves locality of the inputs. > ` > from the doc, add sortsupport function will only influence index > build time? > Thats the point of this patch. Though it influences the index quality in a way which seems to cause the measured performance regression upthread. > > per https://www.postgresql.org/docs/current/gist-extensibility.html > QUOTE: > All the GiST support methods are normally called in short-lived > memory > contexts; that is, CurrentMemoryContext will get reset after each > tuple is processed. It is therefore not very important to worry about > pfree'ing everything you palloc. However, in some cases it's useful > for a support method to > ENDOF_QUOTE > > so removing the following part should be OK. > + if ((Datum) range_a != a) > + pfree(range_a); > + > + if ((Datum) range_b != b) > + pfree(range_b); > Probably, i think we get a different range objects in case of detoasting in this case. > comparison solely on the lower bounds looks strange to me. > if lower bound is the same, then compare upper bound, so the > range_gist_cmp function is consistent with function range_compare. > so following change: > > + else > + result = range_cmp_bounds(typcache, &lower1, &lower2); > to > ` > else > { > result = range_cmp_bounds(typcache, &lower1, &lower2); > if (result == 0) > result = range_cmp_bounds(typcache, &upper1, &upper2); > } > ` > > does contrib/btree_gist/btree_gist--1.7--1.8.sql function be declared > as strict ? (I am not sure) > other than that, the whole patch looks good. > > That's something surely to consider.
