Horton

Re: Yet another fast GiST build

Heikki Linnakangas <hlinnaka@iki.fi>

View thread View on postgr.es
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Peter Geoghegan <pg@bowt.ie>, Tom Lane <tgl@sss.pgh.pa.us>, Justin Pryzby <pryzby@telsasoft.com>, Darafei Komяpa Praliaskouski <me@komzpa.net>, Pavel Borisov <pashkin.elfe@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Michael Paquier <michael@paquier.xyz>, Alexander Korotkov <a.korotkov@postgrespro.ru>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Ibrar Ahmed <ibrar.ahmad@gmail.com>
Date: 2021-04-07T20:18:26Z
Lists: pgsql-hackers 
On 07/04/2021 22:41, Andrey Borodin wrote:
> I see there is a problem with "SET client_min_messages = DEBUG1;" on
> buildfarm. I think these checks were necessary to make sure test
> paths are triggered. When we know that code paths are tested, maybe
> we can omit checks?
Yeah. We don't have very reliable coverage of different GiST build 
methods, as noted earlier in this thread. But that's not this patch's fault.

I've been investigating the varbit issue, and realized that all the 
comparison functions in this patch for varlen datatypes are broken. The 
representation that the sortsupport function sees is the one that the 
'compress' function returns. The fixed-length versions got this right, 
but the varlen versions assume that the input is a Datum of the original 
datatype. It happens to not crash, because the representation returned 
by gbt_var_compress() is also a varlena, and all of the comparison 
functions tolerate the garbage inputs, but it's bogus. The correct 
pattern would be something like this (without the debugging NOTICE, of 
course):

> static int
> gbt_text_sort_build_cmp(Datum a, Datum b, SortSupport ssup)
> {
> 	GBT_VARKEY_R ra = gbt_var_key_readable((GBT_VARKEY *) PG_DETOAST_DATUM(a));
> 	GBT_VARKEY_R rb = gbt_var_key_readable((GBT_VARKEY *) PG_DETOAST_DATUM(b));
> 
> 	int x = DatumGetInt32(DirectFunctionCall2Coll(bttextcmp,
> 												 ssup->ssup_collation,
> 												 PointerGetDatum(a),
> 												 PointerGetDatum(b)));
> 	elog(NOTICE, "cmp: %s vs %s: %d",
> 		 TextDatumGetCString(ra.lower),
> 		 TextDatumGetCString(rb.lower),
> 		 x);
> 	return x;
> }
- Heikki

Commits

  1. Add sortsupport for gist_btree opclasses, for faster index builds.

  2. pageinspect: Fix relcache leak in gist_page_items().

  3. Fix test failure with wal_level=minimal.

  4. Enhance nbtree index tuple deletion.

  5. Fix portability issues in the new gist pageinspect test.

  6. Add functions to 'pageinspect' to inspect GiST indexes.

  7. Fix missing validation for the new GiST sortsupport functions.

  8. Fix compilation warning in xlog.c

  9. Set right-links during sorted GiST index build.

  10. Fix checksum calculation in the new sorting GiST build.

  11. Add support for building GiST index by sorting.