pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

Started by Alexander Korotkovalmost 7 years ago7 messagescomitters

aekorotkov@gmail.com

almost 7 years ago

Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

This commit improves subject in two ways:

* It removes ugliness of 02f90879e7, which stores distance values and null
flags in two separate arrays after GISTSearchItem struct. Instead we pack
both distance value and null flag in IndexOrderByDistance struct. Alignment
overhead should be negligible, because we typically deal with at most few
"col op const" expressions in ORDER BY clause.
* It fixes handling of "col op NULL" expression in KNN-SP-GiST. Now, these
expression are not passed to support functions, which can't deal with them.
Instead, NULL result is implicitly assumed. It future we may decide to
teach support functions to deal with NULL arguments, but current solution is
bugfix suitable for backpatch.

Reported-by: Nikita Glukhov
Discussion: /messages/by-id/826f57ee-afc7-8977-c44c-6111d18b02ec@postgrespro.ru
Author: Nikita Glukhov
Reviewed-by: Alexander Korotkov
Backpatch-through: 9.4

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6cae9d2c10e151f741e7bc64a8b70bb2615c367c

Modified Files
--------------
src/backend/access/gist/gistget.c | 68 ++++++++-------------
src/backend/access/gist/gistscan.c | 16 ++---
src/backend/access/index/indexam.c | 22 +++----
src/backend/access/spgist/spgscan.c | 74 +++++++++++++++++++----
src/include/access/genam.h | 10 ++-
src/include/access/gist_private.h | 27 ++-------
src/include/access/spgist_private.h | 8 ++-
src/test/regress/expected/create_index_spgist.out | 10 +++
src/test/regress/sql/create_index_spgist.sql | 5 ++
src/tools/pgindent/typedefs.list | 1 +
10 files changed, 140 insertions(+), 101 deletions(-)

Erik Rijkers

er@xs4all.nl

almost 7 years ago

In reply to: Alexander Korotkov (#1)

Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

On 2019-09-19 21:11, Alexander Korotkov wrote:

Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

Oops:

create_index ... ok 634 ms
create_index_spgist ... FAILED 438 ms
create_view ... ok 329 ms

Tom Lane

tgl@sss.pgh.pa.us

almost 7 years ago

In reply to: Erik Rijkers (#2)

Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

Erik Rijkers <er@xs4all.nl> writes:

Oops:
create_index ... ok 634 ms
create_index_spgist ... FAILED 438 ms
create_view ... ok 329 ms

I'm betting the issue is breaking the Datum abstraction here:

-                   scan->xs_orderbyvals[i] = Float8GetDatum(distanceValues[i]);
+                   scan->xs_orderbyvals[i] = item->distances[i].value;

AFAICS, item->distances[i].value is a double not a Datum, so dropping
the Float8GetDatum call is just wrong.

regards, tom lane

Alexander Korotkov

aekorotkov@gmail.com

almost 7 years ago

In reply to: Tom Lane (#3)

Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

On Thu, Sep 19, 2019 at 11:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Erik Rijkers <er@xs4all.nl> writes:

Oops:
create_index ... ok 634 ms
create_index_spgist ... FAILED 438 ms
create_view ... ok 329 ms

I'm betting the issue is breaking the Datum abstraction here:
-                   scan->xs_orderbyvals[i] =
Float8GetDatum(distanceValues[i]);
+                   scan->xs_orderbyvals[i] = item->distances[i].value;

Overseen by me. Will fix immediately.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov

aekorotkov@gmail.com

almost 7 years ago

In reply to: Alexander Korotkov (#4)

Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

On Thu, Sep 19, 2019 at 11:31 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:

On Thu, Sep 19, 2019 at 11:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Erik Rijkers <er@xs4all.nl> writes:

Oops:
create_index ... ok 634 ms
create_index_spgist ... FAILED 438 ms
create_view ... ok 329 ms

I'm betting the issue is breaking the Datum abstraction here:
-                   scan->xs_orderbyvals[i] = Float8GetDatum(distanceValues[i]);
+                   scan->xs_orderbyvals[i] = item->distances[i].value;
Overseen by me. Will fix immediately.

Fix pushed from 11 to 9.5, where I made this error during backpatching.

However, I also see set of failures in master, which seems related to
this patch:
* https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-09-19%2020%3A09%3A42
* https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2019-09-19%2020%3A04%3A22
* https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tern&dt=2019-09-19%2019%3A22%3A01

Will investigate them.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov

aekorotkov@gmail.com

almost 7 years ago

In reply to: Alexander Korotkov (#5)

Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

On Thu, Sep 19, 2019 at 11:43 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:

On Thu, Sep 19, 2019 at 11:31 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
On Thu, Sep 19, 2019 at 11:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Erik Rijkers <er@xs4all.nl> writes:

Oops:
create_index ... ok 634 ms
create_index_spgist ... FAILED 438 ms
create_view ... ok 329 ms

I'm betting the issue is breaking the Datum abstraction here:
-                   scan->xs_orderbyvals[i] = Float8GetDatum(distanceValues[i]);
+                   scan->xs_orderbyvals[i] = item->distances[i].value;
Overseen by me. Will fix immediately.
Fix pushed from 11 to 9.5, where I made this error during backpatching.

However, I also see set of failures in master, which seems related to
this patch:
* https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dromedary&dt=2019-09-19%2020%3A09%3A42
* https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2019-09-19%2020%3A04%3A22
* https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tern&dt=2019-09-19%2019%3A22%3A01

Will investigate them.

Both dromedary and tern, where segfault happened, are 32-bit. Bug
seems related to USE_FLOAT8_BYVAL or something.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Tom Lane

tgl@sss.pgh.pa.us

almost 7 years ago

In reply to: Alexander Korotkov (#6)

Re: pgsql: Improve handling of NULLs in KNN-GiST and KNN-SP-GiST

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

Both dromedary and tern, where segfault happened, are 32-bit. Bug
seems related to USE_FLOAT8_BYVAL or something.

Yeah, I was just about to write back that there's an independent problem.
Look at the logic inside the loop in index_store_float8_orderby_distances:

scan->xs_orderbynulls[i] = distances[i].isnull;

if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;

if (orderByTypes[i] == FLOAT8OID)
{
#ifndef USE_FLOAT8_BYVAL
/* must free any old value to avoid memory leakage */
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
if (!scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}

The pfree is being done way too late, as you've already stomped on
both the isnull flag and the pointer value. I think the first
three lines quoted above need to be moved to after the #ifndef
stanza (and, hence, duplicated for the float4 case), like

#ifndef USE_FLOAT8_BYVAL
/* must free any old value to avoid memory leakage */
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
scan->xs_orderbynulls[i] = distances[i].isnull;
if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
else
scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);

Another issue here is that the short-circuit path above (for !distances)
leaks memory, in not-passbyval cases. I'd be inclined to get rid of the
short circuit and just handle the case within the main loop, so really
that'd be more like

if (distances)
{
scan->xs_orderbynulls[i] = distances[i].isnull;
if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
else
scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}
else
{
scan->xs_orderbynulls[i] = true;
scan->xs_orderbyvals[i] = (Datum) 0;
}

regards, tom lane