Horton

Re: Yet another fast GiST build

Emre Hasegeli <emre@hasegeli.com>

View thread View on postgr.es
From: Emre Hasegeli <emre@hasegeli.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, 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-07-05T06:27:50Z
Lists: pgsql-hackers 
I tried reviewing the remaining patches.  It seems to work correctly,
and passes the tests on my laptop.

> In this pattern I flipped PointerGetDatum(a) to PointerGetDatum(ra.lower), because it seems to me correct. I've followed rule of thumb: every sort function must extract and use "lower" somehow. Though I suspect numeric a bit. Is it regular varlena?

As far as I understand, we cannot use the sortsupport functions from
the btree operator classes because the btree_gist extension handles
things differently.  This is unfortunate and a source of bugs [1], but
we cannot do anything about it.

Given that the lower and upper datums must be the same for the leaf
nodes, it makes sense to me to compare one of them.

Using numeric_cmp() for numeric in line with using bttextcmp() for text.

> +   /*
> +    * Numeric has abbreviation routines in numeric.c, but we don't try to use
> +    * them here. Maybe later.
> +    */

This is also true for text.  Perhaps we should also add a comment there.

> PFA patchset with v6 intact + two fixes of discovered issues.

> +   /* Use byteacmp(), like gbt_bitcmp() does */

We can improve this comment by incorporating Heikki's previous email:

> Ok, I think I understand that now. In btree_gist, the *_cmp() function
> operates on non-leaf values, and *_lt(), *_gt() et al operate on leaf
> values. For all other datatypes, the leaf and non-leaf representation is
> the same, but for bit/varbit, the non-leaf representation is different.
> The leaf representation is VarBit, and non-leaf is just the bits without
> the 'bit_len' field. That's why it is indeed correct for gbt_bitcmp() to
> just use byteacmp(), whereas gbt_bitlt() et al compares the 'bit_len'
> field separately. That's subtle, and 100% uncommented.

I think patch number 3 should be squashed to patch number 1.

I couldn't understand patch number 2 "Remove DEBUG1 verification".  It
seems like something rather useful.

[1] https://www.postgresql.org/message-id/flat/201010112055.o9BKtZf7011251%40wwwmaster.postgresql.org

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.