Re: Yet another fast GiST build

From: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
To: emre(at)hasegeli(dot)com
Cc: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Peter Geoghegan <pg(at)bowt(dot)ie>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Darafei Komяpa Praliaskouski <me(at)komzpa(dot)net>, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>
Subject: Re: Yet another fast GiST build
Date: 2021-12-16 09:18:52
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Emre!

Thank you for the review.

> 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.
All patches in the patchset expected to be squashed into 1 during commit.

> I couldn't understand patch number 2 "Remove DEBUG1 verification". It
> seems like something rather useful.
If failed on buildfarm on some nodes. There were somewhat extroneous error messages.
Currently Step 1 and 2 are separete to ensure that opclasses are used correctly.


Best regards, Andrey Borodin.

Attachment Content-Type Size
v4-0001-Add-sortsupport-for-gist_btree-opclasses-for-fast.patch application/octet-stream 86.8 KB
v4-0002-Remove-DEBUG1-verification-of-the-fact-that-index.patch text/x-diff 40.7 KB
v4-0003-Fix-varlen-keys-sorting.patch text/x-diff 3.3 KB
v4-0004-Review-notes-from-Emre-Hasegeli.patch text/x-diff 2.0 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrei Zubkov 2021-12-16 09:23:59 Re: [PATCH] pg_statio_all_tables: several rows per table due to invalid TOAST index
Previous Message 2021-12-16 08:25:35 RE: row filtering for logical replication