public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/4] Fix GiST and SP-GiST ordering by distance for NULLs and NaNs
9+ messages / 6 participants
[nested] [flat]
* [PATCH 1/5] Fix GiST and SP-GiST ordering by distance for NULLs
@ 2019-09-10 14:26 Nikita Glukhov <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Nikita Glukhov @ 2019-09-10 14:26 UTC (permalink / raw)
---
src/backend/access/gist/gistget.c | 68 ++++++++-------------
src/backend/access/gist/gistscan.c | 18 +++---
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, 142 insertions(+), 101 deletions(-)
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index db633a9..22d790d 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -112,9 +112,8 @@ gistkillitems(IndexScanDesc scan)
* Similarly, *recheck_distances_p is set to indicate whether the distances
* need to be rechecked, and it is also ignored for non-leaf entries.
*
- * If we are doing an ordered scan, so->distancesValues[] and
- * so->distancesNulls[] is filled with distance data from the distance()
- * functions before returning success.
+ * If we are doing an ordered scan, so->distances[] is filled with distance
+ * data from the distance() functions before returning success.
*
* We must decompress the key in the IndexTuple before passing it to the
* sk_funcs (which actually are the opclass Consistent or Distance methods).
@@ -135,8 +134,7 @@ gistindex_keytest(IndexScanDesc scan,
GISTSTATE *giststate = so->giststate;
ScanKey key = scan->keyData;
int keySize = scan->numberOfKeys;
- double *distance_value_p;
- bool *distance_null_p;
+ IndexOrderByDistance *distance_p;
Relation r = scan->indexRelation;
*recheck_p = false;
@@ -155,8 +153,8 @@ gistindex_keytest(IndexScanDesc scan,
elog(ERROR, "invalid GiST tuple found on leaf page");
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- so->distanceValues[i] = -get_float8_infinity();
- so->distanceNulls[i] = false;
+ so->distances[i].value = -get_float8_infinity();
+ so->distances[i].isnull = false;
}
return true;
}
@@ -240,8 +238,7 @@ gistindex_keytest(IndexScanDesc scan,
/* OK, it passes --- now let's compute the distances */
key = scan->orderByData;
- distance_value_p = so->distanceValues;
- distance_null_p = so->distanceNulls;
+ distance_p = so->distances;
keySize = scan->numberOfOrderBys;
while (keySize > 0)
{
@@ -256,8 +253,8 @@ gistindex_keytest(IndexScanDesc scan,
if ((key->sk_flags & SK_ISNULL) || isNull)
{
/* Assume distance computes as null */
- *distance_value_p = 0.0;
- *distance_null_p = true;
+ distance_p->value = 0.0;
+ distance_p->isnull = true;
}
else
{
@@ -294,13 +291,12 @@ gistindex_keytest(IndexScanDesc scan,
ObjectIdGetDatum(key->sk_subtype),
PointerGetDatum(&recheck));
*recheck_distances_p |= recheck;
- *distance_value_p = DatumGetFloat8(dist);
- *distance_null_p = false;
+ distance_p->value = DatumGetFloat8(dist);
+ distance_p->isnull = false;
}
key++;
- distance_value_p++;
- distance_null_p++;
+ distance_p++;
keySize--;
}
@@ -313,8 +309,7 @@ gistindex_keytest(IndexScanDesc scan,
*
* scan: index scan we are executing
* pageItem: search queue item identifying an index page to scan
- * myDistanceValues: distances array associated with pageItem, or NULL at the root
- * myDistanceNulls: null flags for myDistanceValues array, or NULL at the root
+ * myDistances: distances array associated with pageItem, or NULL at the root
* tbm: if not NULL, gistgetbitmap's output bitmap
* ntids: if not NULL, gistgetbitmap's output tuple counter
*
@@ -332,8 +327,7 @@ gistindex_keytest(IndexScanDesc scan,
*/
static void
gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
- double *myDistanceValues, bool *myDistanceNulls,
- TIDBitmap *tbm, int64 *ntids)
+ IndexOrderByDistance *myDistances, TIDBitmap *tbm, int64 *ntids)
{
GISTScanOpaque so = (GISTScanOpaque) scan->opaque;
GISTSTATE *giststate = so->giststate;
@@ -370,7 +364,7 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
GISTSearchItem *item;
/* This can't happen when starting at the root */
- Assert(myDistanceValues != NULL && myDistanceNulls != NULL);
+ Assert(myDistances != NULL);
oldcxt = MemoryContextSwitchTo(so->queueCxt);
@@ -380,10 +374,8 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
item->data.parentlsn = pageItem->data.parentlsn;
/* Insert it into the queue using same distances as for this page */
- memcpy(GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- myDistanceValues, sizeof(double) * scan->numberOfOrderBys);
- memcpy(GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- myDistanceNulls, sizeof(bool) * scan->numberOfOrderBys);
+ memcpy(item->distances, myDistances,
+ sizeof(item->distances[0]) * scan->numberOfOrderBys);
pairingheap_add(so->queue, &item->phNode);
@@ -527,10 +519,8 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
}
/* Insert it into the queue using new distance data */
- memcpy(GISTSearchItemDistanceValues(item, nOrderBys),
- so->distanceValues, sizeof(double) * nOrderBys);
- memcpy(GISTSearchItemDistanceNulls(item, nOrderBys),
- so->distanceNulls, sizeof(bool) * nOrderBys);
+ memcpy(item->distances, so->distances,
+ sizeof(item->distances[0]) * nOrderBys);
pairingheap_add(so->queue, &item->phNode);
@@ -595,8 +585,7 @@ getNextNearest(IndexScanDesc scan)
scan->xs_recheck = item->data.heap.recheck;
index_store_float8_orderby_distances(scan, so->orderByTypes,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
+ item->distances,
item->data.heap.recheckDistances);
/* in an index-only scan, also return the reconstructed tuple. */
@@ -609,10 +598,7 @@ getNextNearest(IndexScanDesc scan)
/* visit an index page, extract its items into queue */
CHECK_FOR_INTERRUPTS();
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- NULL, NULL);
+ gistScanPage(scan, item, item->distances, NULL, NULL);
}
pfree(item);
@@ -650,7 +636,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
fakeItem.blkno = GIST_ROOT_BLKNO;
memset(&fakeItem.data.parentlsn, 0, sizeof(GistNSN));
- gistScanPage(scan, &fakeItem, NULL, NULL, NULL, NULL);
+ gistScanPage(scan, &fakeItem, NULL, NULL, NULL);
}
if (scan->numberOfOrderBys > 0)
@@ -744,10 +730,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
* this page, we fall out of the inner "do" and loop around to
* return them.
*/
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- NULL, NULL);
+ gistScanPage(scan, item, item->distances, NULL, NULL);
pfree(item);
} while (so->nPageData == 0);
@@ -778,7 +761,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
fakeItem.blkno = GIST_ROOT_BLKNO;
memset(&fakeItem.data.parentlsn, 0, sizeof(GistNSN));
- gistScanPage(scan, &fakeItem, NULL, NULL, tbm, &ntids);
+ gistScanPage(scan, &fakeItem, NULL, tbm, &ntids);
/*
* While scanning a leaf page, ItemPointers of matching heap tuples will
@@ -793,10 +776,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
CHECK_FOR_INTERRUPTS();
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- tbm, &ntids);
+ gistScanPage(scan, item, item->distances, tbm, &ntids);
pfree(item);
}
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index e72bf08..323a19d 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -14,6 +14,8 @@
*/
#include "postgres.h"
+#include <math.h>
+
#include "access/gist_private.h"
#include "access/gistscan.h"
#include "access/relscan.h"
@@ -33,26 +35,23 @@ pairingheap_GISTSearchItem_cmp(const pairingheap_node *a, const pairingheap_node
const GISTSearchItem *sb = (const GISTSearchItem *) b;
IndexScanDesc scan = (IndexScanDesc) arg;
int i;
- double *da = GISTSearchItemDistanceValues(sa, scan->numberOfOrderBys),
- *db = GISTSearchItemDistanceValues(sb, scan->numberOfOrderBys);
- bool *na = GISTSearchItemDistanceNulls(sa, scan->numberOfOrderBys),
- *nb = GISTSearchItemDistanceNulls(sb, scan->numberOfOrderBys);
/* Order according to distance comparison */
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- if (na[i])
+ if (sa->distances[i].isnull)
{
- if (!nb[i])
+ if (!sb->distances[i].isnull)
return -1;
}
- else if (nb[i])
+ else if (sb->distances[i].isnull)
{
return 1;
}
else
{
- int cmp = -float8_cmp_internal(da[i], db[i]);
+ int cmp = -float8_cmp_internal(sa->distances[i].value,
+ sb->distances[i].value);
if (cmp != 0)
return cmp;
@@ -100,8 +99,7 @@ gistbeginscan(Relation r, int nkeys, int norderbys)
so->queueCxt = giststate->scanCxt; /* see gistrescan */
/* workspaces with size dependent on numberOfOrderBys: */
- so->distanceValues = palloc(sizeof(double) * scan->numberOfOrderBys);
- so->distanceNulls = palloc(sizeof(bool) * scan->numberOfOrderBys);
+ so->distances = palloc(sizeof(so->distances[0]) * scan->numberOfOrderBys);
so->qual_ok = true; /* in case there are zero keys */
if (scan->numberOfOrderBys > 0)
{
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 2e8f53a..442f414 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -847,14 +847,14 @@ index_getprocinfo(Relation irel,
*/
void
index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
- double *distanceValues,
- bool *distanceNulls, bool recheckOrderBy)
+ IndexOrderByDistance *distances,
+ bool recheckOrderBy)
{
int i;
scan->xs_recheckorderby = recheckOrderBy;
- if (!distanceValues)
+ if (!distances)
{
Assert(!scan->xs_recheckorderby);
@@ -869,11 +869,11 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- if (distanceNulls && distanceNulls[i])
- {
+ scan->xs_orderbynulls[i] = distances[i].isnull;
+
+ if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
- scan->xs_orderbynulls[i] = true;
- }
+
if (orderByTypes[i] == FLOAT8OID)
{
#ifndef USE_FLOAT8_BYVAL
@@ -881,8 +881,8 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
- scan->xs_orderbyvals[i] = Float8GetDatum(distanceValues[i]);
- scan->xs_orderbynulls[i] = false;
+ if (!scan->xs_orderbynulls[i])
+ scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}
else if (orderByTypes[i] == FLOAT4OID)
{
@@ -892,8 +892,8 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
- scan->xs_orderbyvals[i] = Float4GetDatum((float4) distanceValues[i]);
- scan->xs_orderbynulls[i] = false;
+ if (!scan->xs_orderbynulls[i])
+ scan->xs_orderbyvals[i] = Float4GetDatum((float4) distances[i].value);
}
else
{
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 2bd4037..053bf09 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -107,13 +107,13 @@ spgAllocSearchItem(SpGistScanOpaque so, bool isnull, double *distances)
{
/* allocate distance array only for non-NULL items */
SpGistSearchItem *item =
- palloc(SizeOfSpGistSearchItem(isnull ? 0 : so->numberOfOrderBys));
+ palloc(SizeOfSpGistSearchItem(isnull ? 0 : so->numberOfNonNullOrderBys));
item->isNull = isnull;
- if (!isnull && so->numberOfOrderBys > 0)
+ if (!isnull && so->numberOfNonNullOrderBys > 0)
memcpy(item->distances, distances,
- so->numberOfOrderBys * sizeof(double));
+ sizeof(item->distances[0]) * so->numberOfNonNullOrderBys);
return item;
}
@@ -208,6 +208,34 @@ spgPrepareScanKeys(IndexScanDesc scan)
so->numberOfOrderBys = scan->numberOfOrderBys;
so->orderByData = scan->orderByData;
+ if (so->numberOfOrderBys <= 0)
+ so->numberOfNonNullOrderBys = 0;
+ else
+ {
+ int j = 0;
+
+ /*
+ * Remove all NULL keys, but remember their offsets in the original
+ * array.
+ */
+ for (i = 0; i < scan->numberOfOrderBys; i++)
+ {
+ ScanKey skey = &so->orderByData[i];
+
+ if (skey->sk_flags & SK_ISNULL)
+ so->nonNullOrderByOffsets[i] = -1;
+ else
+ {
+ if (i != j)
+ so->orderByData[j] = *skey;
+
+ so->nonNullOrderByOffsets[i] = j++;
+ }
+ }
+
+ so->numberOfNonNullOrderBys = j;
+ }
+
if (scan->numberOfKeys <= 0)
{
/* If no quals, whole-index scan is required */
@@ -295,6 +323,8 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
/* This will be filled in spgrescan, but allocate the space here */
so->orderByTypes = (Oid *)
palloc(sizeof(Oid) * scan->numberOfOrderBys);
+ so->nonNullOrderByOffsets = (int *)
+ palloc(sizeof(int) * scan->numberOfOrderBys);
/* These arrays have constant contents, so we can fill them now */
so->zeroDistances = (double *)
@@ -394,6 +424,7 @@ spgendscan(IndexScanDesc scan)
if (scan->numberOfOrderBys > 0)
{
pfree(so->orderByTypes);
+ pfree(so->nonNullOrderByOffsets);
pfree(so->zeroDistances);
pfree(so->infDistances);
pfree(scan->xs_orderbyvals);
@@ -465,7 +496,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
in.scankeys = so->keyData;
in.nkeys = so->numberOfKeys;
in.orderbys = so->orderByData;
- in.norderbys = so->numberOfOrderBys;
+ in.norderbys = so->numberOfNonNullOrderBys;
in.reconstructedValue = item->value;
in.traversalValue = item->traversalValue;
in.level = item->level;
@@ -492,7 +523,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
if (result)
{
/* item passes the scankeys */
- if (so->numberOfOrderBys > 0)
+ if (so->numberOfNonNullOrderBys > 0)
{
/* the scan is ordered -> add the item to the queue */
MemoryContext oldCxt = MemoryContextSwitchTo(so->traversalCxt);
@@ -531,7 +562,7 @@ spgInitInnerConsistentIn(spgInnerConsistentIn *in,
in->scankeys = so->keyData;
in->orderbys = so->orderByData;
in->nkeys = so->numberOfKeys;
- in->norderbys = so->numberOfOrderBys;
+ in->norderbys = so->numberOfNonNullOrderBys;
in->reconstructedValue = item->value;
in->traversalMemoryContext = so->traversalCxt;
in->traversalValue = item->traversalValue;
@@ -751,7 +782,7 @@ redirect:
if (item->isLeaf)
{
/* We store heap items in the queue only in case of ordered search */
- Assert(so->numberOfOrderBys > 0);
+ Assert(so->numberOfNonNullOrderBys > 0);
storeRes(so, &item->heapPtr, item->value, item->isNull,
item->recheck, item->recheckDistances, item->distances);
reportedSome = true;
@@ -874,7 +905,7 @@ spggetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
static void
storeGettuple(SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isnull, bool recheck, bool recheckDistances,
- double *distances)
+ double *nonNullDistances)
{
Assert(so->nPtrs < MaxIndexTuplesPerPage);
so->heapPtrs[so->nPtrs] = *heapPtr;
@@ -883,13 +914,33 @@ storeGettuple(SpGistScanOpaque so, ItemPointer heapPtr,
if (so->numberOfOrderBys > 0)
{
- if (isnull)
+ if (isnull || so->numberOfNonNullOrderBys <= 0)
so->distances[so->nPtrs] = NULL;
else
{
- Size size = sizeof(double) * so->numberOfOrderBys;
+ IndexOrderByDistance *distances =
+ palloc(sizeof(distances[0]) * so->numberOfOrderBys);
+ int i;
+
+ for (i = 0; i < so->numberOfOrderBys; i++)
+ {
+ int offset = so->nonNullOrderByOffsets[i];
+
+ if (offset >= 0)
+ {
+ /* Copy non-NULL distance value */
+ distances[i].value = nonNullDistances[offset];
+ distances[i].isnull = false;
+ }
+ else
+ {
+ /* Set distance's NULL flag. */
+ distances[i].value = 0.0;
+ distances[i].isnull = true;
+ }
+ }
- so->distances[so->nPtrs] = memcpy(palloc(size), distances, size);
+ so->distances[so->nPtrs] = distances;
}
}
@@ -929,7 +980,6 @@ spggettuple(IndexScanDesc scan, ScanDirection dir)
if (so->numberOfOrderBys > 0)
index_store_float8_orderby_distances(scan, so->orderByTypes,
so->distances[so->iPtr],
- NULL,
so->recheckDistances[so->iPtr]);
so->iPtr++;
return true;
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 6c56717..aaa56ae 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -118,6 +118,13 @@ typedef enum IndexUniqueCheck
} IndexUniqueCheck;
+/* Nullable ORDER BY distance */
+typedef struct IndexOrderByDistance
+{
+ double value;
+ bool isnull;
+} IndexOrderByDistance;
+
/*
* generalized index_ interface routines (in indexam.c)
*/
@@ -179,8 +186,7 @@ extern FmgrInfo *index_getprocinfo(Relation irel, AttrNumber attnum,
uint16 procnum);
extern void index_store_float8_orderby_distances(IndexScanDesc scan,
Oid *orderByTypes,
- double *distanceValues,
- bool *distanceNulls,
+ IndexOrderByDistance *distances,
bool recheckOrderBy);
/*
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index ed5b643..ab134c1 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -138,29 +138,15 @@ typedef struct GISTSearchItem
GISTSearchHeapItem heap; /* heap info, if heap tuple */
} data;
- /*
- * This data structure is followed by arrays of distance values and
- * distance null flags. Size of both arrays is
- * IndexScanDesc->numberOfOrderBys. See macros below for accessing those
- * arrays.
- */
+ /* numberOfOrderBys entries */
+ IndexOrderByDistance distances[FLEXIBLE_ARRAY_MEMBER];
} GISTSearchItem;
#define GISTSearchItemIsHeap(item) ((item).blkno == InvalidBlockNumber)
-#define SizeOfGISTSearchItem(n_distances) (DOUBLEALIGN(sizeof(GISTSearchItem)) + \
- (sizeof(double) + sizeof(bool)) * (n_distances))
-
-/*
- * We actually don't need n_distances compute pointer to distance values.
- * Nevertheless take n_distances as argument to have same arguments list for
- * GISTSearchItemDistanceValues() and GISTSearchItemDistanceNulls().
- */
-#define GISTSearchItemDistanceValues(item, n_distances) \
- ((double *) ((Pointer) (item) + DOUBLEALIGN(sizeof(GISTSearchItem))))
-
-#define GISTSearchItemDistanceNulls(item, n_distances) \
- ((bool *) ((Pointer) (item) + DOUBLEALIGN(sizeof(GISTSearchItem)) + sizeof(double) * (n_distances)))
+#define SizeOfGISTSearchItem(n_distances) \
+ (offsetof(GISTSearchItem, distances) + \
+ sizeof(IndexOrderByDistance) * (n_distances))
/*
* GISTScanOpaqueData: private state for a scan of a GiST index
@@ -176,8 +162,7 @@ typedef struct GISTScanOpaqueData
bool firstCall; /* true until first gistgettuple call */
/* pre-allocated workspace arrays */
- double *distanceValues; /* output area for gistindex_keytest */
- bool *distanceNulls;
+ IndexOrderByDistance *distances; /* output area for gistindex_keytest */
/* info about killed items if any (killedItems is NULL if never used) */
OffsetNumber *killedItems; /* offset numbers of killed items */
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index e0d1178..3d322c9 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -169,8 +169,12 @@ typedef struct SpGistScanOpaqueData
int numberOfKeys; /* number of index qualifier conditions */
ScanKey keyData; /* array of index qualifier descriptors */
int numberOfOrderBys; /* number of ordering operators */
+ int numberOfNonNullOrderBys; /* number of ordering operators
+ * with non-NULL arguments */
ScanKey orderByData; /* array of ordering op descriptors */
Oid *orderByTypes; /* array of ordering op return types */
+ int *nonNullOrderByOffsets; /* array of offset of non-NULL ordering
+ * keys in the original array */
Oid indexCollation; /* collation of index column */
/* Opclass defined functions: */
@@ -195,7 +199,9 @@ typedef struct SpGistScanOpaqueData
bool recheckDistances[MaxIndexTuplesPerPage]; /* distance recheck
* flags */
HeapTuple reconTups[MaxIndexTuplesPerPage]; /* reconstructed tuples */
- double *distances[MaxIndexTuplesPerPage]; /* distances (for recheck) */
+
+ /* distances (for recheck) */
+ IndexOrderByDistance *distances[MaxIndexTuplesPerPage];
/*
* Note: using MaxIndexTuplesPerPage above is a bit hokey since
diff --git a/src/test/regress/expected/create_index_spgist.out b/src/test/regress/expected/create_index_spgist.out
index 81b4a67..5ccea91 100644
--- a/src/test/regress/expected/create_index_spgist.out
+++ b/src/test/regress/expected/create_index_spgist.out
@@ -555,6 +555,16 @@ WHERE seq.dist IS DISTINCT FROM idx.dist;
---+------+---+---+------+---
(0 rows)
+-- check ORDER BY distance to NULL
+SELECT (SELECT p FROM kd_point_tbl ORDER BY p <-> pt LIMIT 1)
+FROM (VALUES (point '1,2'), (NULL), ('1234,5678')) pts(pt);
+ p
+-------------
+ (59,21)
+ (9853,112)
+ (1239,5647)
+(3 rows)
+
EXPLAIN (COSTS OFF)
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
QUERY PLAN
diff --git a/src/test/regress/sql/create_index_spgist.sql b/src/test/regress/sql/create_index_spgist.sql
index 8e6c453..629936f 100644
--- a/src/test/regress/sql/create_index_spgist.sql
+++ b/src/test/regress/sql/create_index_spgist.sql
@@ -225,6 +225,11 @@ SELECT * FROM quad_point_tbl_ord_seq3 seq FULL JOIN kd_point_tbl_ord_idx3 idx
ON seq.n = idx.n
WHERE seq.dist IS DISTINCT FROM idx.dist;
+-- check ORDER BY distance to NULL
+SELECT (SELECT p FROM kd_point_tbl ORDER BY p <-> pt LIMIT 1)
+FROM (VALUES (point '1,2'), (NULL), ('1234,5678')) pts(pt);
+
+
EXPLAIN (COSTS OFF)
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index f3cdfa8..fb65265 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1043,6 +1043,7 @@ IndexList
IndexOnlyScan
IndexOnlyScanState
IndexOptInfo
+IndexOrderByDistance
IndexPath
IndexRuntimeKeyInfo
IndexScan
--
2.7.4
--------------5AE546B531D96C8320FA33AB
Content-Type: text/x-patch;
name="0002-Use-IndexOrderByDistance-in-SP-GiST-user-functions-v3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0002-Use-IndexOrderByDistance-in-SP-GiST-user-functions-v3.p";
filename*1="atch"
^ permalink raw reply [nested|flat] 9+ messages in thread
* [PATCH 1/4] Fix GiST and SP-GiST ordering by distance for NULLs and NaNs
@ 2019-09-10 14:26 Nikita Glukhov <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Nikita Glukhov @ 2019-09-10 14:26 UTC (permalink / raw)
---
src/backend/access/gist/gistget.c | 68 ++++++++---------------
src/backend/access/gist/gistscan.c | 18 +++---
src/backend/access/index/indexam.c | 22 ++++----
src/backend/access/spgist/spgkdtreeproc.c | 2 +-
src/backend/access/spgist/spgproc.c | 19 +++++--
src/backend/access/spgist/spgquadtreeproc.c | 2 +-
src/backend/access/spgist/spgscan.c | 60 ++++++++++++--------
src/backend/utils/adt/geo_spgist.c | 40 +++++++++----
src/include/access/genam.h | 10 +++-
src/include/access/gist_private.h | 27 ++-------
src/include/access/spgist.h | 4 +-
src/include/access/spgist_private.h | 18 +++---
src/test/regress/expected/create_index_spgist.out | 10 ++++
src/test/regress/sql/create_index_spgist.sql | 5 ++
14 files changed, 167 insertions(+), 138 deletions(-)
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index db633a9..22d790d 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -112,9 +112,8 @@ gistkillitems(IndexScanDesc scan)
* Similarly, *recheck_distances_p is set to indicate whether the distances
* need to be rechecked, and it is also ignored for non-leaf entries.
*
- * If we are doing an ordered scan, so->distancesValues[] and
- * so->distancesNulls[] is filled with distance data from the distance()
- * functions before returning success.
+ * If we are doing an ordered scan, so->distances[] is filled with distance
+ * data from the distance() functions before returning success.
*
* We must decompress the key in the IndexTuple before passing it to the
* sk_funcs (which actually are the opclass Consistent or Distance methods).
@@ -135,8 +134,7 @@ gistindex_keytest(IndexScanDesc scan,
GISTSTATE *giststate = so->giststate;
ScanKey key = scan->keyData;
int keySize = scan->numberOfKeys;
- double *distance_value_p;
- bool *distance_null_p;
+ IndexOrderByDistance *distance_p;
Relation r = scan->indexRelation;
*recheck_p = false;
@@ -155,8 +153,8 @@ gistindex_keytest(IndexScanDesc scan,
elog(ERROR, "invalid GiST tuple found on leaf page");
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- so->distanceValues[i] = -get_float8_infinity();
- so->distanceNulls[i] = false;
+ so->distances[i].value = -get_float8_infinity();
+ so->distances[i].isnull = false;
}
return true;
}
@@ -240,8 +238,7 @@ gistindex_keytest(IndexScanDesc scan,
/* OK, it passes --- now let's compute the distances */
key = scan->orderByData;
- distance_value_p = so->distanceValues;
- distance_null_p = so->distanceNulls;
+ distance_p = so->distances;
keySize = scan->numberOfOrderBys;
while (keySize > 0)
{
@@ -256,8 +253,8 @@ gistindex_keytest(IndexScanDesc scan,
if ((key->sk_flags & SK_ISNULL) || isNull)
{
/* Assume distance computes as null */
- *distance_value_p = 0.0;
- *distance_null_p = true;
+ distance_p->value = 0.0;
+ distance_p->isnull = true;
}
else
{
@@ -294,13 +291,12 @@ gistindex_keytest(IndexScanDesc scan,
ObjectIdGetDatum(key->sk_subtype),
PointerGetDatum(&recheck));
*recheck_distances_p |= recheck;
- *distance_value_p = DatumGetFloat8(dist);
- *distance_null_p = false;
+ distance_p->value = DatumGetFloat8(dist);
+ distance_p->isnull = false;
}
key++;
- distance_value_p++;
- distance_null_p++;
+ distance_p++;
keySize--;
}
@@ -313,8 +309,7 @@ gistindex_keytest(IndexScanDesc scan,
*
* scan: index scan we are executing
* pageItem: search queue item identifying an index page to scan
- * myDistanceValues: distances array associated with pageItem, or NULL at the root
- * myDistanceNulls: null flags for myDistanceValues array, or NULL at the root
+ * myDistances: distances array associated with pageItem, or NULL at the root
* tbm: if not NULL, gistgetbitmap's output bitmap
* ntids: if not NULL, gistgetbitmap's output tuple counter
*
@@ -332,8 +327,7 @@ gistindex_keytest(IndexScanDesc scan,
*/
static void
gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
- double *myDistanceValues, bool *myDistanceNulls,
- TIDBitmap *tbm, int64 *ntids)
+ IndexOrderByDistance *myDistances, TIDBitmap *tbm, int64 *ntids)
{
GISTScanOpaque so = (GISTScanOpaque) scan->opaque;
GISTSTATE *giststate = so->giststate;
@@ -370,7 +364,7 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
GISTSearchItem *item;
/* This can't happen when starting at the root */
- Assert(myDistanceValues != NULL && myDistanceNulls != NULL);
+ Assert(myDistances != NULL);
oldcxt = MemoryContextSwitchTo(so->queueCxt);
@@ -380,10 +374,8 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
item->data.parentlsn = pageItem->data.parentlsn;
/* Insert it into the queue using same distances as for this page */
- memcpy(GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- myDistanceValues, sizeof(double) * scan->numberOfOrderBys);
- memcpy(GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- myDistanceNulls, sizeof(bool) * scan->numberOfOrderBys);
+ memcpy(item->distances, myDistances,
+ sizeof(item->distances[0]) * scan->numberOfOrderBys);
pairingheap_add(so->queue, &item->phNode);
@@ -527,10 +519,8 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
}
/* Insert it into the queue using new distance data */
- memcpy(GISTSearchItemDistanceValues(item, nOrderBys),
- so->distanceValues, sizeof(double) * nOrderBys);
- memcpy(GISTSearchItemDistanceNulls(item, nOrderBys),
- so->distanceNulls, sizeof(bool) * nOrderBys);
+ memcpy(item->distances, so->distances,
+ sizeof(item->distances[0]) * nOrderBys);
pairingheap_add(so->queue, &item->phNode);
@@ -595,8 +585,7 @@ getNextNearest(IndexScanDesc scan)
scan->xs_recheck = item->data.heap.recheck;
index_store_float8_orderby_distances(scan, so->orderByTypes,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
+ item->distances,
item->data.heap.recheckDistances);
/* in an index-only scan, also return the reconstructed tuple. */
@@ -609,10 +598,7 @@ getNextNearest(IndexScanDesc scan)
/* visit an index page, extract its items into queue */
CHECK_FOR_INTERRUPTS();
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- NULL, NULL);
+ gistScanPage(scan, item, item->distances, NULL, NULL);
}
pfree(item);
@@ -650,7 +636,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
fakeItem.blkno = GIST_ROOT_BLKNO;
memset(&fakeItem.data.parentlsn, 0, sizeof(GistNSN));
- gistScanPage(scan, &fakeItem, NULL, NULL, NULL, NULL);
+ gistScanPage(scan, &fakeItem, NULL, NULL, NULL);
}
if (scan->numberOfOrderBys > 0)
@@ -744,10 +730,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
* this page, we fall out of the inner "do" and loop around to
* return them.
*/
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- NULL, NULL);
+ gistScanPage(scan, item, item->distances, NULL, NULL);
pfree(item);
} while (so->nPageData == 0);
@@ -778,7 +761,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
fakeItem.blkno = GIST_ROOT_BLKNO;
memset(&fakeItem.data.parentlsn, 0, sizeof(GistNSN));
- gistScanPage(scan, &fakeItem, NULL, NULL, tbm, &ntids);
+ gistScanPage(scan, &fakeItem, NULL, tbm, &ntids);
/*
* While scanning a leaf page, ItemPointers of matching heap tuples will
@@ -793,10 +776,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
CHECK_FOR_INTERRUPTS();
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- tbm, &ntids);
+ gistScanPage(scan, item, item->distances, tbm, &ntids);
pfree(item);
}
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index e72bf08..323a19d 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -14,6 +14,8 @@
*/
#include "postgres.h"
+#include <math.h>
+
#include "access/gist_private.h"
#include "access/gistscan.h"
#include "access/relscan.h"
@@ -33,26 +35,23 @@ pairingheap_GISTSearchItem_cmp(const pairingheap_node *a, const pairingheap_node
const GISTSearchItem *sb = (const GISTSearchItem *) b;
IndexScanDesc scan = (IndexScanDesc) arg;
int i;
- double *da = GISTSearchItemDistanceValues(sa, scan->numberOfOrderBys),
- *db = GISTSearchItemDistanceValues(sb, scan->numberOfOrderBys);
- bool *na = GISTSearchItemDistanceNulls(sa, scan->numberOfOrderBys),
- *nb = GISTSearchItemDistanceNulls(sb, scan->numberOfOrderBys);
/* Order according to distance comparison */
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- if (na[i])
+ if (sa->distances[i].isnull)
{
- if (!nb[i])
+ if (!sb->distances[i].isnull)
return -1;
}
- else if (nb[i])
+ else if (sb->distances[i].isnull)
{
return 1;
}
else
{
- int cmp = -float8_cmp_internal(da[i], db[i]);
+ int cmp = -float8_cmp_internal(sa->distances[i].value,
+ sb->distances[i].value);
if (cmp != 0)
return cmp;
@@ -100,8 +99,7 @@ gistbeginscan(Relation r, int nkeys, int norderbys)
so->queueCxt = giststate->scanCxt; /* see gistrescan */
/* workspaces with size dependent on numberOfOrderBys: */
- so->distanceValues = palloc(sizeof(double) * scan->numberOfOrderBys);
- so->distanceNulls = palloc(sizeof(bool) * scan->numberOfOrderBys);
+ so->distances = palloc(sizeof(so->distances[0]) * scan->numberOfOrderBys);
so->qual_ok = true; /* in case there are zero keys */
if (scan->numberOfOrderBys > 0)
{
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 2e8f53a..442f414 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -847,14 +847,14 @@ index_getprocinfo(Relation irel,
*/
void
index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
- double *distanceValues,
- bool *distanceNulls, bool recheckOrderBy)
+ IndexOrderByDistance *distances,
+ bool recheckOrderBy)
{
int i;
scan->xs_recheckorderby = recheckOrderBy;
- if (!distanceValues)
+ if (!distances)
{
Assert(!scan->xs_recheckorderby);
@@ -869,11 +869,11 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- if (distanceNulls && distanceNulls[i])
- {
+ scan->xs_orderbynulls[i] = distances[i].isnull;
+
+ if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
- scan->xs_orderbynulls[i] = true;
- }
+
if (orderByTypes[i] == FLOAT8OID)
{
#ifndef USE_FLOAT8_BYVAL
@@ -881,8 +881,8 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
- scan->xs_orderbyvals[i] = Float8GetDatum(distanceValues[i]);
- scan->xs_orderbynulls[i] = false;
+ if (!scan->xs_orderbynulls[i])
+ scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}
else if (orderByTypes[i] == FLOAT4OID)
{
@@ -892,8 +892,8 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
- scan->xs_orderbyvals[i] = Float4GetDatum((float4) distanceValues[i]);
- scan->xs_orderbynulls[i] = false;
+ if (!scan->xs_orderbynulls[i])
+ scan->xs_orderbyvals[i] = Float4GetDatum((float4) distances[i].value);
}
else
{
diff --git a/src/backend/access/spgist/spgkdtreeproc.c b/src/backend/access/spgist/spgkdtreeproc.c
index 9d479fe..66ab1ac 100644
--- a/src/backend/access/spgist/spgkdtreeproc.c
+++ b/src/backend/access/spgist/spgkdtreeproc.c
@@ -271,7 +271,7 @@ spg_kd_inner_consistent(PG_FUNCTION_ARGS)
BOX infArea;
BOX *area;
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
out->traversalValues = (void **) palloc(sizeof(void *) * in->nNodes);
if (in->level == 0)
diff --git a/src/backend/access/spgist/spgproc.c b/src/backend/access/spgist/spgproc.c
index 688638a..1059456 100644
--- a/src/backend/access/spgist/spgproc.c
+++ b/src/backend/access/spgist/spgproc.c
@@ -59,20 +59,29 @@ point_box_distance(Point *point, BOX *box)
* is expected to be point, non-leaf key is expected to be box. Scan key
* arguments are expected to be points.
*/
-double *
+IndexOrderByDistance *
spg_key_orderbys_distances(Datum key, bool isLeaf,
ScanKey orderbys, int norderbys)
{
int sk_num;
- double *distances = (double *) palloc(norderbys * sizeof(double)),
+ IndexOrderByDistance *distances = palloc(sizeof(distances[0]) * norderbys),
*distance = distances;
for (sk_num = 0; sk_num < norderbys; ++sk_num, ++orderbys, ++distance)
{
- Point *point = DatumGetPointP(orderbys->sk_argument);
+ if (orderbys->sk_flags & SK_ISNULL)
+ {
+ distance->isnull = true;
+ distance->value = 0;
+ }
+ else
+ {
+ Point *point = DatumGetPointP(orderbys->sk_argument);
- *distance = isLeaf ? point_point_distance(point, DatumGetPointP(key))
- : point_box_distance(point, DatumGetBoxP(key));
+ distance->isnull = false;
+ distance->value = isLeaf ? point_point_distance(point, DatumGetPointP(key))
+ : point_box_distance(point, DatumGetBoxP(key));
+ }
}
return distances;
diff --git a/src/backend/access/spgist/spgquadtreeproc.c b/src/backend/access/spgist/spgquadtreeproc.c
index e50108e..3f59bea 100644
--- a/src/backend/access/spgist/spgquadtreeproc.c
+++ b/src/backend/access/spgist/spgquadtreeproc.c
@@ -247,7 +247,7 @@ spg_quad_inner_consistent(PG_FUNCTION_ARGS)
*/
if (in->norderbys > 0)
{
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
out->traversalValues = (void **) palloc(sizeof(void *) * in->nNodes);
if (in->level == 0)
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 2bd4037..a837789 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -28,7 +28,8 @@
typedef void (*storeRes_func) (SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isNull, bool recheck,
- bool recheckDistances, double *distances);
+ bool recheckDistances,
+ IndexOrderByDistance *distances);
/*
* Pairing heap comparison function for the SpGistSearchItem queue.
@@ -58,14 +59,23 @@ pairingheap_SpGistSearchItem_cmp(const pairingheap_node *a,
/* Order according to distance comparison */
for (i = 0; i < so->numberOfOrderBys; i++)
{
- if (isnan(sa->distances[i]) && isnan(sb->distances[i]))
+ if (sa->distances[i].isnull)
+ {
+ if (!sb->distances[i].isnull)
+ return -1;
+ continue;
+ }
+ else if (sb->distances[i].isnull)
+ return 1;
+
+ if (isnan(sa->distances[i].value) && isnan(sb->distances[i].value))
continue; /* NaN == NaN */
- if (isnan(sa->distances[i]))
+ if (isnan(sa->distances[i].value))
return -1; /* NaN > number */
- if (isnan(sb->distances[i]))
+ if (isnan(sb->distances[i].value))
return 1; /* number < NaN */
- if (sa->distances[i] != sb->distances[i])
- return (sa->distances[i] < sb->distances[i]) ? 1 : -1;
+ if (sa->distances[i].value != sb->distances[i].value)
+ return (sa->distances[i].value < sb->distances[i].value) ? 1 : -1;
}
}
@@ -103,7 +113,8 @@ spgAddSearchItemToQueue(SpGistScanOpaque so, SpGistSearchItem *item)
}
static SpGistSearchItem *
-spgAllocSearchItem(SpGistScanOpaque so, bool isnull, double *distances)
+spgAllocSearchItem(SpGistScanOpaque so, bool isnull,
+ IndexOrderByDistance *distances)
{
/* allocate distance array only for non-NULL items */
SpGistSearchItem *item =
@@ -113,7 +124,7 @@ spgAllocSearchItem(SpGistScanOpaque so, bool isnull, double *distances)
if (!isnull && so->numberOfOrderBys > 0)
memcpy(item->distances, distances,
- so->numberOfOrderBys * sizeof(double));
+ sizeof(item->distances[0]) * so->numberOfOrderBys);
return item;
}
@@ -297,15 +308,17 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
palloc(sizeof(Oid) * scan->numberOfOrderBys);
/* These arrays have constant contents, so we can fill them now */
- so->zeroDistances = (double *)
- palloc(sizeof(double) * scan->numberOfOrderBys);
- so->infDistances = (double *)
- palloc(sizeof(double) * scan->numberOfOrderBys);
+ so->zeroDistances =
+ palloc(sizeof(so->zeroDistances[0]) * scan->numberOfOrderBys);
+ so->infDistances =
+ palloc(sizeof(so->infDistances[0]) * scan->numberOfOrderBys);
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- so->zeroDistances[i] = 0.0;
- so->infDistances[i] = get_float8_infinity();
+ so->zeroDistances[i].value = 0.0;
+ so->zeroDistances[i].isnull = false;
+ so->infDistances[i].value = get_float8_infinity();
+ so->infDistances[i].isnull = false;
}
scan->xs_orderbyvals = (Datum *)
@@ -409,7 +422,7 @@ spgendscan(IndexScanDesc scan)
static SpGistSearchItem *
spgNewHeapItem(SpGistScanOpaque so, int level, ItemPointer heapPtr,
Datum leafValue, bool recheck, bool recheckDistances,
- bool isnull, double *distances)
+ bool isnull, IndexOrderByDistance *distances)
{
SpGistSearchItem *item = spgAllocSearchItem(so, isnull, distances);
@@ -439,7 +452,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
bool *reportedSome, storeRes_func storeRes)
{
Datum leafValue;
- double *distances;
+ IndexOrderByDistance *distances;
bool result;
bool recheck;
bool recheckDistances;
@@ -549,7 +562,7 @@ spgMakeInnerItem(SpGistScanOpaque so,
SpGistSearchItem *parentItem,
SpGistNodeTuple tuple,
spgInnerConsistentOut *out, int i, bool isnull,
- double *distances)
+ IndexOrderByDistance *distances)
{
SpGistSearchItem *item = spgAllocSearchItem(so, isnull, distances);
@@ -633,7 +646,7 @@ spgInnerTest(SpGistScanOpaque so, SpGistSearchItem *item,
{
int nodeN = out.nodeNumbers[i];
SpGistSearchItem *innerItem;
- double *distances;
+ IndexOrderByDistance *distances;
Assert(nodeN >= 0 && nodeN < nNodes);
@@ -847,7 +860,7 @@ redirect:
static void
storeBitmap(SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isnull, bool recheck, bool recheckDistances,
- double *distances)
+ IndexOrderByDistance *distances)
{
Assert(!recheckDistances && !distances);
tbm_add_tuples(so->tbm, heapPtr, 1, recheck);
@@ -874,7 +887,7 @@ spggetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
static void
storeGettuple(SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isnull, bool recheck, bool recheckDistances,
- double *distances)
+ IndexOrderByDistance *distances)
{
Assert(so->nPtrs < MaxIndexTuplesPerPage);
so->heapPtrs[so->nPtrs] = *heapPtr;
@@ -887,9 +900,11 @@ storeGettuple(SpGistScanOpaque so, ItemPointer heapPtr,
so->distances[so->nPtrs] = NULL;
else
{
- Size size = sizeof(double) * so->numberOfOrderBys;
+ Size size = sizeof(distances[0]) * so->numberOfOrderBys;
+
+ so->distances[so->nPtrs] = palloc(size);
- so->distances[so->nPtrs] = memcpy(palloc(size), distances, size);
+ memcpy(so->distances[so->nPtrs], distances, size);
}
}
@@ -929,7 +944,6 @@ spggettuple(IndexScanDesc scan, ScanDirection dir)
if (so->numberOfOrderBys > 0)
index_store_float8_orderby_distances(scan, so->orderByTypes,
so->distances[so->iPtr],
- NULL,
so->recheckDistances[so->iPtr]);
so->iPtr++;
return true;
diff --git a/src/backend/utils/adt/geo_spgist.c b/src/backend/utils/adt/geo_spgist.c
index 8e29770..cde6264 100644
--- a/src/backend/utils/adt/geo_spgist.c
+++ b/src/backend/utils/adt/geo_spgist.c
@@ -580,24 +580,33 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
if (in->norderbys > 0 && in->nNodes > 0)
{
- double *distances = palloc(sizeof(double) * in->norderbys);
+ IndexOrderByDistance *distances = palloc(sizeof(distances[0]) * in->norderbys);
int j;
for (j = 0; j < in->norderbys; j++)
{
- Point *pt = DatumGetPointP(in->orderbys[j].sk_argument);
+ if (in->orderbys[j].sk_flags & SK_ISNULL)
+ {
+ distances[j].value = 0;
+ distances[j].isnull = true;
+ }
+ else
+ {
+ Point *pt = DatumGetPointP(in->orderbys[j].sk_argument);
- distances[j] = pointToRectBoxDistance(pt, rect_box);
+ distances[j].value = pointToRectBoxDistance(pt, rect_box);
+ distances[j].isnull = false;
+ }
}
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
out->distances[0] = distances;
for (i = 1; i < in->nNodes; i++)
{
- out->distances[i] = palloc(sizeof(double) * in->norderbys);
+ out->distances[i] = palloc(sizeof(distances[0]) * in->norderbys);
memcpy(out->distances[i], distances,
- sizeof(double) * in->norderbys);
+ sizeof(distances[0]) * in->norderbys);
}
}
@@ -622,7 +631,7 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
out->nodeNumbers = (int *) palloc(sizeof(int) * in->nNodes);
out->traversalValues = (void **) palloc(sizeof(void *) * in->nNodes);
if (in->norderbys > 0)
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
/*
* We switch memory context, because we want to allocate memory for new
@@ -703,16 +712,25 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
if (in->norderbys > 0)
{
- double *distances = palloc(sizeof(double) * in->norderbys);
+ IndexOrderByDistance *distances = palloc(sizeof(distances[0]) * in->norderbys);
int j;
out->distances[out->nNodes] = distances;
for (j = 0; j < in->norderbys; j++)
{
- Point *pt = DatumGetPointP(in->orderbys[j].sk_argument);
-
- distances[j] = pointToRectBoxDistance(pt, next_rect_box);
+ if (in->orderbys[j].sk_flags & SK_ISNULL)
+ {
+ distances[j].value = 0;
+ distances[j].isnull = true;
+ }
+ else
+ {
+ Point *pt = DatumGetPointP(in->orderbys[j].sk_argument);
+
+ distances[j].value = pointToRectBoxDistance(pt, next_rect_box);
+ distances[j].isnull = false;
+ }
}
}
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 6c56717..aaa56ae 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -118,6 +118,13 @@ typedef enum IndexUniqueCheck
} IndexUniqueCheck;
+/* Nullable ORDER BY distance */
+typedef struct IndexOrderByDistance
+{
+ double value;
+ bool isnull;
+} IndexOrderByDistance;
+
/*
* generalized index_ interface routines (in indexam.c)
*/
@@ -179,8 +186,7 @@ extern FmgrInfo *index_getprocinfo(Relation irel, AttrNumber attnum,
uint16 procnum);
extern void index_store_float8_orderby_distances(IndexScanDesc scan,
Oid *orderByTypes,
- double *distanceValues,
- bool *distanceNulls,
+ IndexOrderByDistance *distances,
bool recheckOrderBy);
/*
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index ed5b643..ab134c1 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -138,29 +138,15 @@ typedef struct GISTSearchItem
GISTSearchHeapItem heap; /* heap info, if heap tuple */
} data;
- /*
- * This data structure is followed by arrays of distance values and
- * distance null flags. Size of both arrays is
- * IndexScanDesc->numberOfOrderBys. See macros below for accessing those
- * arrays.
- */
+ /* numberOfOrderBys entries */
+ IndexOrderByDistance distances[FLEXIBLE_ARRAY_MEMBER];
} GISTSearchItem;
#define GISTSearchItemIsHeap(item) ((item).blkno == InvalidBlockNumber)
-#define SizeOfGISTSearchItem(n_distances) (DOUBLEALIGN(sizeof(GISTSearchItem)) + \
- (sizeof(double) + sizeof(bool)) * (n_distances))
-
-/*
- * We actually don't need n_distances compute pointer to distance values.
- * Nevertheless take n_distances as argument to have same arguments list for
- * GISTSearchItemDistanceValues() and GISTSearchItemDistanceNulls().
- */
-#define GISTSearchItemDistanceValues(item, n_distances) \
- ((double *) ((Pointer) (item) + DOUBLEALIGN(sizeof(GISTSearchItem))))
-
-#define GISTSearchItemDistanceNulls(item, n_distances) \
- ((bool *) ((Pointer) (item) + DOUBLEALIGN(sizeof(GISTSearchItem)) + sizeof(double) * (n_distances)))
+#define SizeOfGISTSearchItem(n_distances) \
+ (offsetof(GISTSearchItem, distances) + \
+ sizeof(IndexOrderByDistance) * (n_distances))
/*
* GISTScanOpaqueData: private state for a scan of a GiST index
@@ -176,8 +162,7 @@ typedef struct GISTScanOpaqueData
bool firstCall; /* true until first gistgettuple call */
/* pre-allocated workspace arrays */
- double *distanceValues; /* output area for gistindex_keytest */
- bool *distanceNulls;
+ IndexOrderByDistance *distances; /* output area for gistindex_keytest */
/* info about killed items if any (killedItems is NULL if never used) */
OffsetNumber *killedItems; /* offset numbers of killed items */
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index d787ab2..bcbcb1b 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -161,7 +161,7 @@ typedef struct spgInnerConsistentOut
int *levelAdds; /* increment level by this much for each */
Datum *reconstructedValues; /* associated reconstructed values */
void **traversalValues; /* opclass-specific traverse values */
- double **distances; /* associated distances */
+ IndexOrderByDistance **distances; /* associated distances */
} spgInnerConsistentOut;
/*
@@ -188,7 +188,7 @@ typedef struct spgLeafConsistentOut
Datum leafValue; /* reconstructed original data, if any */
bool recheck; /* set true if operator must be rechecked */
bool recheckDistances; /* set true if distances must be rechecked */
- double *distances; /* associated distances */
+ IndexOrderByDistance *distances; /* associated distances */
} spgLeafConsistentOut;
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index e0d1178..2875c1f 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -145,11 +145,12 @@ typedef struct SpGistSearchItem
bool recheckDistances; /* distance recheck is needed */
/* array with numberOfOrderBys entries */
- double distances[FLEXIBLE_ARRAY_MEMBER];
+ IndexOrderByDistance distances[FLEXIBLE_ARRAY_MEMBER];
} SpGistSearchItem;
#define SizeOfSpGistSearchItem(n_distances) \
- (offsetof(SpGistSearchItem, distances) + sizeof(double) * (n_distances))
+ (offsetof(SpGistSearchItem, distances) + \
+ sizeof(IndexOrderByDistance) * (n_distances))
/*
* Private state of an index scan
@@ -178,8 +179,8 @@ typedef struct SpGistScanOpaqueData
FmgrInfo leafConsistentFn;
/* Pre-allocated workspace arrays: */
- double *zeroDistances;
- double *infDistances;
+ IndexOrderByDistance *zeroDistances;
+ IndexOrderByDistance *infDistances;
/* These fields are only used in amgetbitmap scans: */
TIDBitmap *tbm; /* bitmap being filled */
@@ -195,7 +196,9 @@ typedef struct SpGistScanOpaqueData
bool recheckDistances[MaxIndexTuplesPerPage]; /* distance recheck
* flags */
HeapTuple reconTups[MaxIndexTuplesPerPage]; /* reconstructed tuples */
- double *distances[MaxIndexTuplesPerPage]; /* distances (for recheck) */
+
+ /* distances (for recheck) */
+ IndexOrderByDistance *distances[MaxIndexTuplesPerPage];
/*
* Note: using MaxIndexTuplesPerPage above is a bit hokey since
@@ -459,8 +462,9 @@ extern bool spgdoinsert(Relation index, SpGistState *state,
ItemPointer heapPtr, Datum datum, bool isnull);
/* spgproc.c */
-extern double *spg_key_orderbys_distances(Datum key, bool isLeaf,
- ScanKey orderbys, int norderbys);
+extern IndexOrderByDistance *spg_key_orderbys_distances(Datum key, bool isLeaf,
+ ScanKey orderbys,
+ int norderbys);
extern BOX *box_copy(BOX *orig);
#endif /* SPGIST_PRIVATE_H */
diff --git a/src/test/regress/expected/create_index_spgist.out b/src/test/regress/expected/create_index_spgist.out
index 81b4a67..5ccea91 100644
--- a/src/test/regress/expected/create_index_spgist.out
+++ b/src/test/regress/expected/create_index_spgist.out
@@ -555,6 +555,16 @@ WHERE seq.dist IS DISTINCT FROM idx.dist;
---+------+---+---+------+---
(0 rows)
+-- check ORDER BY distance to NULL
+SELECT (SELECT p FROM kd_point_tbl ORDER BY p <-> pt LIMIT 1)
+FROM (VALUES (point '1,2'), (NULL), ('1234,5678')) pts(pt);
+ p
+-------------
+ (59,21)
+ (9853,112)
+ (1239,5647)
+(3 rows)
+
EXPLAIN (COSTS OFF)
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
QUERY PLAN
diff --git a/src/test/regress/sql/create_index_spgist.sql b/src/test/regress/sql/create_index_spgist.sql
index 8e6c453..629936f 100644
--- a/src/test/regress/sql/create_index_spgist.sql
+++ b/src/test/regress/sql/create_index_spgist.sql
@@ -225,6 +225,11 @@ SELECT * FROM quad_point_tbl_ord_seq3 seq FULL JOIN kd_point_tbl_ord_idx3 idx
ON seq.n = idx.n
WHERE seq.dist IS DISTINCT FROM idx.dist;
+-- check ORDER BY distance to NULL
+SELECT (SELECT p FROM kd_point_tbl ORDER BY p <-> pt LIMIT 1)
+FROM (VALUES (point '1,2'), (NULL), ('1234,5678')) pts(pt);
+
+
EXPLAIN (COSTS OFF)
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
--
2.7.4
--------------E276088F2DBC9E82E62FE42F--
^ permalink raw reply [nested|flat] 9+ messages in thread
* [PATCH 1/4] Fix GiST and SP-GiST ordering by distance for NULLs
@ 2019-09-10 14:26 Nikita Glukhov <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Nikita Glukhov @ 2019-09-10 14:26 UTC (permalink / raw)
---
src/backend/access/gist/gistget.c | 68 ++++-------
src/backend/access/gist/gistscan.c | 18 ++-
src/backend/access/index/indexam.c | 22 ++--
src/backend/access/spgist/spgkdtreeproc.c | 2 +-
src/backend/access/spgist/spgproc.c | 9 +-
src/backend/access/spgist/spgquadtreeproc.c | 2 +-
src/backend/access/spgist/spgscan.c | 136 +++++++++++++++++-----
src/backend/utils/adt/geo_spgist.c | 18 +--
src/include/access/genam.h | 10 +-
src/include/access/gist_private.h | 27 +----
src/include/access/spgist.h | 4 +-
src/include/access/spgist_private.h | 22 ++--
src/test/regress/expected/create_index_spgist.out | 10 ++
src/test/regress/sql/create_index_spgist.sql | 5 +
src/tools/pgindent/typedefs.list | 1 +
15 files changed, 212 insertions(+), 142 deletions(-)
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index db633a9..22d790d 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -112,9 +112,8 @@ gistkillitems(IndexScanDesc scan)
* Similarly, *recheck_distances_p is set to indicate whether the distances
* need to be rechecked, and it is also ignored for non-leaf entries.
*
- * If we are doing an ordered scan, so->distancesValues[] and
- * so->distancesNulls[] is filled with distance data from the distance()
- * functions before returning success.
+ * If we are doing an ordered scan, so->distances[] is filled with distance
+ * data from the distance() functions before returning success.
*
* We must decompress the key in the IndexTuple before passing it to the
* sk_funcs (which actually are the opclass Consistent or Distance methods).
@@ -135,8 +134,7 @@ gistindex_keytest(IndexScanDesc scan,
GISTSTATE *giststate = so->giststate;
ScanKey key = scan->keyData;
int keySize = scan->numberOfKeys;
- double *distance_value_p;
- bool *distance_null_p;
+ IndexOrderByDistance *distance_p;
Relation r = scan->indexRelation;
*recheck_p = false;
@@ -155,8 +153,8 @@ gistindex_keytest(IndexScanDesc scan,
elog(ERROR, "invalid GiST tuple found on leaf page");
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- so->distanceValues[i] = -get_float8_infinity();
- so->distanceNulls[i] = false;
+ so->distances[i].value = -get_float8_infinity();
+ so->distances[i].isnull = false;
}
return true;
}
@@ -240,8 +238,7 @@ gistindex_keytest(IndexScanDesc scan,
/* OK, it passes --- now let's compute the distances */
key = scan->orderByData;
- distance_value_p = so->distanceValues;
- distance_null_p = so->distanceNulls;
+ distance_p = so->distances;
keySize = scan->numberOfOrderBys;
while (keySize > 0)
{
@@ -256,8 +253,8 @@ gistindex_keytest(IndexScanDesc scan,
if ((key->sk_flags & SK_ISNULL) || isNull)
{
/* Assume distance computes as null */
- *distance_value_p = 0.0;
- *distance_null_p = true;
+ distance_p->value = 0.0;
+ distance_p->isnull = true;
}
else
{
@@ -294,13 +291,12 @@ gistindex_keytest(IndexScanDesc scan,
ObjectIdGetDatum(key->sk_subtype),
PointerGetDatum(&recheck));
*recheck_distances_p |= recheck;
- *distance_value_p = DatumGetFloat8(dist);
- *distance_null_p = false;
+ distance_p->value = DatumGetFloat8(dist);
+ distance_p->isnull = false;
}
key++;
- distance_value_p++;
- distance_null_p++;
+ distance_p++;
keySize--;
}
@@ -313,8 +309,7 @@ gistindex_keytest(IndexScanDesc scan,
*
* scan: index scan we are executing
* pageItem: search queue item identifying an index page to scan
- * myDistanceValues: distances array associated with pageItem, or NULL at the root
- * myDistanceNulls: null flags for myDistanceValues array, or NULL at the root
+ * myDistances: distances array associated with pageItem, or NULL at the root
* tbm: if not NULL, gistgetbitmap's output bitmap
* ntids: if not NULL, gistgetbitmap's output tuple counter
*
@@ -332,8 +327,7 @@ gistindex_keytest(IndexScanDesc scan,
*/
static void
gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
- double *myDistanceValues, bool *myDistanceNulls,
- TIDBitmap *tbm, int64 *ntids)
+ IndexOrderByDistance *myDistances, TIDBitmap *tbm, int64 *ntids)
{
GISTScanOpaque so = (GISTScanOpaque) scan->opaque;
GISTSTATE *giststate = so->giststate;
@@ -370,7 +364,7 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
GISTSearchItem *item;
/* This can't happen when starting at the root */
- Assert(myDistanceValues != NULL && myDistanceNulls != NULL);
+ Assert(myDistances != NULL);
oldcxt = MemoryContextSwitchTo(so->queueCxt);
@@ -380,10 +374,8 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
item->data.parentlsn = pageItem->data.parentlsn;
/* Insert it into the queue using same distances as for this page */
- memcpy(GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- myDistanceValues, sizeof(double) * scan->numberOfOrderBys);
- memcpy(GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- myDistanceNulls, sizeof(bool) * scan->numberOfOrderBys);
+ memcpy(item->distances, myDistances,
+ sizeof(item->distances[0]) * scan->numberOfOrderBys);
pairingheap_add(so->queue, &item->phNode);
@@ -527,10 +519,8 @@ gistScanPage(IndexScanDesc scan, GISTSearchItem *pageItem,
}
/* Insert it into the queue using new distance data */
- memcpy(GISTSearchItemDistanceValues(item, nOrderBys),
- so->distanceValues, sizeof(double) * nOrderBys);
- memcpy(GISTSearchItemDistanceNulls(item, nOrderBys),
- so->distanceNulls, sizeof(bool) * nOrderBys);
+ memcpy(item->distances, so->distances,
+ sizeof(item->distances[0]) * nOrderBys);
pairingheap_add(so->queue, &item->phNode);
@@ -595,8 +585,7 @@ getNextNearest(IndexScanDesc scan)
scan->xs_recheck = item->data.heap.recheck;
index_store_float8_orderby_distances(scan, so->orderByTypes,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
+ item->distances,
item->data.heap.recheckDistances);
/* in an index-only scan, also return the reconstructed tuple. */
@@ -609,10 +598,7 @@ getNextNearest(IndexScanDesc scan)
/* visit an index page, extract its items into queue */
CHECK_FOR_INTERRUPTS();
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- NULL, NULL);
+ gistScanPage(scan, item, item->distances, NULL, NULL);
}
pfree(item);
@@ -650,7 +636,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
fakeItem.blkno = GIST_ROOT_BLKNO;
memset(&fakeItem.data.parentlsn, 0, sizeof(GistNSN));
- gistScanPage(scan, &fakeItem, NULL, NULL, NULL, NULL);
+ gistScanPage(scan, &fakeItem, NULL, NULL, NULL);
}
if (scan->numberOfOrderBys > 0)
@@ -744,10 +730,7 @@ gistgettuple(IndexScanDesc scan, ScanDirection dir)
* this page, we fall out of the inner "do" and loop around to
* return them.
*/
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- NULL, NULL);
+ gistScanPage(scan, item, item->distances, NULL, NULL);
pfree(item);
} while (so->nPageData == 0);
@@ -778,7 +761,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
fakeItem.blkno = GIST_ROOT_BLKNO;
memset(&fakeItem.data.parentlsn, 0, sizeof(GistNSN));
- gistScanPage(scan, &fakeItem, NULL, NULL, tbm, &ntids);
+ gistScanPage(scan, &fakeItem, NULL, tbm, &ntids);
/*
* While scanning a leaf page, ItemPointers of matching heap tuples will
@@ -793,10 +776,7 @@ gistgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
CHECK_FOR_INTERRUPTS();
- gistScanPage(scan, item,
- GISTSearchItemDistanceValues(item, scan->numberOfOrderBys),
- GISTSearchItemDistanceNulls(item, scan->numberOfOrderBys),
- tbm, &ntids);
+ gistScanPage(scan, item, item->distances, tbm, &ntids);
pfree(item);
}
diff --git a/src/backend/access/gist/gistscan.c b/src/backend/access/gist/gistscan.c
index e72bf08..323a19d 100644
--- a/src/backend/access/gist/gistscan.c
+++ b/src/backend/access/gist/gistscan.c
@@ -14,6 +14,8 @@
*/
#include "postgres.h"
+#include <math.h>
+
#include "access/gist_private.h"
#include "access/gistscan.h"
#include "access/relscan.h"
@@ -33,26 +35,23 @@ pairingheap_GISTSearchItem_cmp(const pairingheap_node *a, const pairingheap_node
const GISTSearchItem *sb = (const GISTSearchItem *) b;
IndexScanDesc scan = (IndexScanDesc) arg;
int i;
- double *da = GISTSearchItemDistanceValues(sa, scan->numberOfOrderBys),
- *db = GISTSearchItemDistanceValues(sb, scan->numberOfOrderBys);
- bool *na = GISTSearchItemDistanceNulls(sa, scan->numberOfOrderBys),
- *nb = GISTSearchItemDistanceNulls(sb, scan->numberOfOrderBys);
/* Order according to distance comparison */
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- if (na[i])
+ if (sa->distances[i].isnull)
{
- if (!nb[i])
+ if (!sb->distances[i].isnull)
return -1;
}
- else if (nb[i])
+ else if (sb->distances[i].isnull)
{
return 1;
}
else
{
- int cmp = -float8_cmp_internal(da[i], db[i]);
+ int cmp = -float8_cmp_internal(sa->distances[i].value,
+ sb->distances[i].value);
if (cmp != 0)
return cmp;
@@ -100,8 +99,7 @@ gistbeginscan(Relation r, int nkeys, int norderbys)
so->queueCxt = giststate->scanCxt; /* see gistrescan */
/* workspaces with size dependent on numberOfOrderBys: */
- so->distanceValues = palloc(sizeof(double) * scan->numberOfOrderBys);
- so->distanceNulls = palloc(sizeof(bool) * scan->numberOfOrderBys);
+ so->distances = palloc(sizeof(so->distances[0]) * scan->numberOfOrderBys);
so->qual_ok = true; /* in case there are zero keys */
if (scan->numberOfOrderBys > 0)
{
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 2e8f53a..442f414 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -847,14 +847,14 @@ index_getprocinfo(Relation irel,
*/
void
index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
- double *distanceValues,
- bool *distanceNulls, bool recheckOrderBy)
+ IndexOrderByDistance *distances,
+ bool recheckOrderBy)
{
int i;
scan->xs_recheckorderby = recheckOrderBy;
- if (!distanceValues)
+ if (!distances)
{
Assert(!scan->xs_recheckorderby);
@@ -869,11 +869,11 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- if (distanceNulls && distanceNulls[i])
- {
+ scan->xs_orderbynulls[i] = distances[i].isnull;
+
+ if (scan->xs_orderbynulls[i])
scan->xs_orderbyvals[i] = (Datum) 0;
- scan->xs_orderbynulls[i] = true;
- }
+
if (orderByTypes[i] == FLOAT8OID)
{
#ifndef USE_FLOAT8_BYVAL
@@ -881,8 +881,8 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
- scan->xs_orderbyvals[i] = Float8GetDatum(distanceValues[i]);
- scan->xs_orderbynulls[i] = false;
+ if (!scan->xs_orderbynulls[i])
+ scan->xs_orderbyvals[i] = Float8GetDatum(distances[i].value);
}
else if (orderByTypes[i] == FLOAT4OID)
{
@@ -892,8 +892,8 @@ index_store_float8_orderby_distances(IndexScanDesc scan, Oid *orderByTypes,
if (!scan->xs_orderbynulls[i])
pfree(DatumGetPointer(scan->xs_orderbyvals[i]));
#endif
- scan->xs_orderbyvals[i] = Float4GetDatum((float4) distanceValues[i]);
- scan->xs_orderbynulls[i] = false;
+ if (!scan->xs_orderbynulls[i])
+ scan->xs_orderbyvals[i] = Float4GetDatum((float4) distances[i].value);
}
else
{
diff --git a/src/backend/access/spgist/spgkdtreeproc.c b/src/backend/access/spgist/spgkdtreeproc.c
index 9d479fe..66ab1ac 100644
--- a/src/backend/access/spgist/spgkdtreeproc.c
+++ b/src/backend/access/spgist/spgkdtreeproc.c
@@ -271,7 +271,7 @@ spg_kd_inner_consistent(PG_FUNCTION_ARGS)
BOX infArea;
BOX *area;
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
out->traversalValues = (void **) palloc(sizeof(void *) * in->nNodes);
if (in->level == 0)
diff --git a/src/backend/access/spgist/spgproc.c b/src/backend/access/spgist/spgproc.c
index 688638a..41203ed 100644
--- a/src/backend/access/spgist/spgproc.c
+++ b/src/backend/access/spgist/spgproc.c
@@ -59,20 +59,21 @@ point_box_distance(Point *point, BOX *box)
* is expected to be point, non-leaf key is expected to be box. Scan key
* arguments are expected to be points.
*/
-double *
+IndexOrderByDistance *
spg_key_orderbys_distances(Datum key, bool isLeaf,
ScanKey orderbys, int norderbys)
{
int sk_num;
- double *distances = (double *) palloc(norderbys * sizeof(double)),
+ IndexOrderByDistance *distances = palloc(sizeof(distances[0]) * norderbys),
*distance = distances;
for (sk_num = 0; sk_num < norderbys; ++sk_num, ++orderbys, ++distance)
{
Point *point = DatumGetPointP(orderbys->sk_argument);
- *distance = isLeaf ? point_point_distance(point, DatumGetPointP(key))
- : point_box_distance(point, DatumGetBoxP(key));
+ distance->isnull = false;
+ distance->value = isLeaf ? point_point_distance(point, DatumGetPointP(key))
+ : point_box_distance(point, DatumGetBoxP(key));
}
return distances;
diff --git a/src/backend/access/spgist/spgquadtreeproc.c b/src/backend/access/spgist/spgquadtreeproc.c
index e50108e..3f59bea 100644
--- a/src/backend/access/spgist/spgquadtreeproc.c
+++ b/src/backend/access/spgist/spgquadtreeproc.c
@@ -247,7 +247,7 @@ spg_quad_inner_consistent(PG_FUNCTION_ARGS)
*/
if (in->norderbys > 0)
{
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
out->traversalValues = (void **) palloc(sizeof(void *) * in->nNodes);
if (in->level == 0)
diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 2bd4037..41354d8e 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -28,7 +28,8 @@
typedef void (*storeRes_func) (SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isNull, bool recheck,
- bool recheckDistances, double *distances);
+ bool recheckDistances,
+ IndexOrderByDistance *distances);
/*
* Pairing heap comparison function for the SpGistSearchItem queue.
@@ -56,16 +57,25 @@ pairingheap_SpGistSearchItem_cmp(const pairingheap_node *a,
else
{
/* Order according to distance comparison */
- for (i = 0; i < so->numberOfOrderBys; i++)
+ for (i = 0; i < so->numberOfNonNullOrderBys; i++)
{
- if (isnan(sa->distances[i]) && isnan(sb->distances[i]))
+ if (sa->distances[i].isnull)
+ {
+ if (!sb->distances[i].isnull)
+ return -1;
+ continue;
+ }
+ else if (sb->distances[i].isnull)
+ return 1;
+
+ if (isnan(sa->distances[i].value) && isnan(sb->distances[i].value))
continue; /* NaN == NaN */
- if (isnan(sa->distances[i]))
+ if (isnan(sa->distances[i].value))
return -1; /* NaN > number */
- if (isnan(sb->distances[i]))
+ if (isnan(sb->distances[i].value))
return 1; /* number < NaN */
- if (sa->distances[i] != sb->distances[i])
- return (sa->distances[i] < sb->distances[i]) ? 1 : -1;
+ if (sa->distances[i].value != sb->distances[i].value)
+ return (sa->distances[i].value < sb->distances[i].value) ? 1 : -1;
}
}
@@ -103,17 +113,18 @@ spgAddSearchItemToQueue(SpGistScanOpaque so, SpGistSearchItem *item)
}
static SpGistSearchItem *
-spgAllocSearchItem(SpGistScanOpaque so, bool isnull, double *distances)
+spgAllocSearchItem(SpGistScanOpaque so, bool isnull,
+ IndexOrderByDistance *distances)
{
/* allocate distance array only for non-NULL items */
SpGistSearchItem *item =
- palloc(SizeOfSpGistSearchItem(isnull ? 0 : so->numberOfOrderBys));
+ palloc(SizeOfSpGistSearchItem(isnull ? 0 : so->numberOfNonNullOrderBys));
item->isNull = isnull;
- if (!isnull && so->numberOfOrderBys > 0)
+ if (!isnull && so->numberOfNonNullOrderBys > 0)
memcpy(item->distances, distances,
- so->numberOfOrderBys * sizeof(double));
+ sizeof(item->distances[0]) * so->numberOfNonNullOrderBys);
return item;
}
@@ -208,6 +219,34 @@ spgPrepareScanKeys(IndexScanDesc scan)
so->numberOfOrderBys = scan->numberOfOrderBys;
so->orderByData = scan->orderByData;
+ if (so->numberOfOrderBys <= 0)
+ so->numberOfNonNullOrderBys = 0;
+ else
+ {
+ int j = 0;
+
+ /*
+ * Remove all NULL keys, but remember their offsets in the original
+ * array.
+ */
+ for (i = 0; i < scan->numberOfOrderBys; i++)
+ {
+ ScanKey skey = &so->orderByData[i];
+
+ if (skey->sk_flags & SK_ISNULL)
+ so->nonNullOrderByOffsets[i] = -1;
+ else
+ {
+ if (i != j)
+ so->orderByData[j] = *skey;
+
+ so->nonNullOrderByOffsets[i] = j++;
+ }
+ }
+
+ so->numberOfNonNullOrderBys = j;
+ }
+
if (scan->numberOfKeys <= 0)
{
/* If no quals, whole-index scan is required */
@@ -295,17 +334,21 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
/* This will be filled in spgrescan, but allocate the space here */
so->orderByTypes = (Oid *)
palloc(sizeof(Oid) * scan->numberOfOrderBys);
+ so->nonNullOrderByOffsets = (int *)
+ palloc(sizeof(int) * scan->numberOfOrderBys);
/* These arrays have constant contents, so we can fill them now */
- so->zeroDistances = (double *)
- palloc(sizeof(double) * scan->numberOfOrderBys);
- so->infDistances = (double *)
- palloc(sizeof(double) * scan->numberOfOrderBys);
+ so->zeroDistances =
+ palloc(sizeof(so->zeroDistances[0]) * scan->numberOfOrderBys);
+ so->infDistances =
+ palloc(sizeof(so->infDistances[0]) * scan->numberOfOrderBys);
for (i = 0; i < scan->numberOfOrderBys; i++)
{
- so->zeroDistances[i] = 0.0;
- so->infDistances[i] = get_float8_infinity();
+ so->zeroDistances[i].value = 0.0;
+ so->zeroDistances[i].isnull = false;
+ so->infDistances[i].value = get_float8_infinity();
+ so->infDistances[i].isnull = false;
}
scan->xs_orderbyvals = (Datum *)
@@ -394,6 +437,7 @@ spgendscan(IndexScanDesc scan)
if (scan->numberOfOrderBys > 0)
{
pfree(so->orderByTypes);
+ pfree(so->nonNullOrderByOffsets);
pfree(so->zeroDistances);
pfree(so->infDistances);
pfree(scan->xs_orderbyvals);
@@ -409,7 +453,7 @@ spgendscan(IndexScanDesc scan)
static SpGistSearchItem *
spgNewHeapItem(SpGistScanOpaque so, int level, ItemPointer heapPtr,
Datum leafValue, bool recheck, bool recheckDistances,
- bool isnull, double *distances)
+ bool isnull, IndexOrderByDistance *distances)
{
SpGistSearchItem *item = spgAllocSearchItem(so, isnull, distances);
@@ -439,7 +483,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
bool *reportedSome, storeRes_func storeRes)
{
Datum leafValue;
- double *distances;
+ IndexOrderByDistance *distances;
bool result;
bool recheck;
bool recheckDistances;
@@ -465,7 +509,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
in.scankeys = so->keyData;
in.nkeys = so->numberOfKeys;
in.orderbys = so->orderByData;
- in.norderbys = so->numberOfOrderBys;
+ in.norderbys = so->numberOfNonNullOrderBys;
in.reconstructedValue = item->value;
in.traversalValue = item->traversalValue;
in.level = item->level;
@@ -492,7 +536,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
if (result)
{
/* item passes the scankeys */
- if (so->numberOfOrderBys > 0)
+ if (so->numberOfNonNullOrderBys > 0)
{
/* the scan is ordered -> add the item to the queue */
MemoryContext oldCxt = MemoryContextSwitchTo(so->traversalCxt);
@@ -531,7 +575,7 @@ spgInitInnerConsistentIn(spgInnerConsistentIn *in,
in->scankeys = so->keyData;
in->orderbys = so->orderByData;
in->nkeys = so->numberOfKeys;
- in->norderbys = so->numberOfOrderBys;
+ in->norderbys = so->numberOfNonNullOrderBys;
in->reconstructedValue = item->value;
in->traversalMemoryContext = so->traversalCxt;
in->traversalValue = item->traversalValue;
@@ -549,7 +593,7 @@ spgMakeInnerItem(SpGistScanOpaque so,
SpGistSearchItem *parentItem,
SpGistNodeTuple tuple,
spgInnerConsistentOut *out, int i, bool isnull,
- double *distances)
+ IndexOrderByDistance *distances)
{
SpGistSearchItem *item = spgAllocSearchItem(so, isnull, distances);
@@ -633,7 +677,7 @@ spgInnerTest(SpGistScanOpaque so, SpGistSearchItem *item,
{
int nodeN = out.nodeNumbers[i];
SpGistSearchItem *innerItem;
- double *distances;
+ IndexOrderByDistance *distances;
Assert(nodeN >= 0 && nodeN < nNodes);
@@ -751,7 +795,7 @@ redirect:
if (item->isLeaf)
{
/* We store heap items in the queue only in case of ordered search */
- Assert(so->numberOfOrderBys > 0);
+ Assert(so->numberOfNonNullOrderBys > 0);
storeRes(so, &item->heapPtr, item->value, item->isNull,
item->recheck, item->recheckDistances, item->distances);
reportedSome = true;
@@ -847,7 +891,7 @@ redirect:
static void
storeBitmap(SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isnull, bool recheck, bool recheckDistances,
- double *distances)
+ IndexOrderByDistance *distances)
{
Assert(!recheckDistances && !distances);
tbm_add_tuples(so->tbm, heapPtr, 1, recheck);
@@ -874,7 +918,7 @@ spggetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
static void
storeGettuple(SpGistScanOpaque so, ItemPointer heapPtr,
Datum leafValue, bool isnull, bool recheck, bool recheckDistances,
- double *distances)
+ IndexOrderByDistance *nonNullDistances)
{
Assert(so->nPtrs < MaxIndexTuplesPerPage);
so->heapPtrs[so->nPtrs] = *heapPtr;
@@ -883,13 +927,44 @@ storeGettuple(SpGistScanOpaque so, ItemPointer heapPtr,
if (so->numberOfOrderBys > 0)
{
- if (isnull)
+ if (isnull || so->numberOfNonNullOrderBys <= 0)
so->distances[so->nPtrs] = NULL;
else
{
- Size size = sizeof(double) * so->numberOfOrderBys;
+ Size size = sizeof(nonNullDistances[0]) * so->numberOfOrderBys;
+ IndexOrderByDistance *distances = palloc(size);
+
+ if (so->numberOfNonNullOrderBys >= so->numberOfOrderBys)
+ {
+ /*
+ * All distance keys are not NULL, so simply copy distance
+ * values.
+ */
+ memcpy(distances, nonNullDistances, size);
+ }
+ else
+ {
+ int i;
+
+ for (i = 0; i < so->numberOfOrderBys; i++)
+ {
+ int offset = so->nonNullOrderByOffsets[i];
+
+ if (offset >= 0)
+ {
+ /* Copy non-NULL distance value */
+ distances[i] = nonNullDistances[offset];
+ }
+ else
+ {
+ /* Set distance's NULL flag. */
+ distances[i].value = 0.0;
+ distances[i].isnull = true;
+ }
+ }
+ }
- so->distances[so->nPtrs] = memcpy(palloc(size), distances, size);
+ so->distances[so->nPtrs] = distances;
}
}
@@ -929,7 +1004,6 @@ spggettuple(IndexScanDesc scan, ScanDirection dir)
if (so->numberOfOrderBys > 0)
index_store_float8_orderby_distances(scan, so->orderByTypes,
so->distances[so->iPtr],
- NULL,
so->recheckDistances[so->iPtr]);
so->iPtr++;
return true;
diff --git a/src/backend/utils/adt/geo_spgist.c b/src/backend/utils/adt/geo_spgist.c
index 8e29770..448b574 100644
--- a/src/backend/utils/adt/geo_spgist.c
+++ b/src/backend/utils/adt/geo_spgist.c
@@ -580,24 +580,25 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
if (in->norderbys > 0 && in->nNodes > 0)
{
- double *distances = palloc(sizeof(double) * in->norderbys);
+ IndexOrderByDistance *distances = palloc(sizeof(distances[0]) * in->norderbys);
int j;
for (j = 0; j < in->norderbys; j++)
{
Point *pt = DatumGetPointP(in->orderbys[j].sk_argument);
- distances[j] = pointToRectBoxDistance(pt, rect_box);
+ distances[j].value = pointToRectBoxDistance(pt, rect_box);
+ distances[j].isnull = false;
}
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
out->distances[0] = distances;
for (i = 1; i < in->nNodes; i++)
{
- out->distances[i] = palloc(sizeof(double) * in->norderbys);
+ out->distances[i] = palloc(sizeof(distances[0]) * in->norderbys);
memcpy(out->distances[i], distances,
- sizeof(double) * in->norderbys);
+ sizeof(distances[0]) * in->norderbys);
}
}
@@ -622,7 +623,7 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
out->nodeNumbers = (int *) palloc(sizeof(int) * in->nNodes);
out->traversalValues = (void **) palloc(sizeof(void *) * in->nNodes);
if (in->norderbys > 0)
- out->distances = (double **) palloc(sizeof(double *) * in->nNodes);
+ out->distances = palloc(sizeof(out->distances[0]) * in->nNodes);
/*
* We switch memory context, because we want to allocate memory for new
@@ -703,7 +704,7 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
if (in->norderbys > 0)
{
- double *distances = palloc(sizeof(double) * in->norderbys);
+ IndexOrderByDistance *distances = palloc(sizeof(distances[0]) * in->norderbys);
int j;
out->distances[out->nNodes] = distances;
@@ -712,7 +713,8 @@ spg_box_quad_inner_consistent(PG_FUNCTION_ARGS)
{
Point *pt = DatumGetPointP(in->orderbys[j].sk_argument);
- distances[j] = pointToRectBoxDistance(pt, next_rect_box);
+ distances[j].value = pointToRectBoxDistance(pt, next_rect_box);
+ distances[j].isnull = false;
}
}
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 6c56717..aaa56ae 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -118,6 +118,13 @@ typedef enum IndexUniqueCheck
} IndexUniqueCheck;
+/* Nullable ORDER BY distance */
+typedef struct IndexOrderByDistance
+{
+ double value;
+ bool isnull;
+} IndexOrderByDistance;
+
/*
* generalized index_ interface routines (in indexam.c)
*/
@@ -179,8 +186,7 @@ extern FmgrInfo *index_getprocinfo(Relation irel, AttrNumber attnum,
uint16 procnum);
extern void index_store_float8_orderby_distances(IndexScanDesc scan,
Oid *orderByTypes,
- double *distanceValues,
- bool *distanceNulls,
+ IndexOrderByDistance *distances,
bool recheckOrderBy);
/*
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index ed5b643..ab134c1 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -138,29 +138,15 @@ typedef struct GISTSearchItem
GISTSearchHeapItem heap; /* heap info, if heap tuple */
} data;
- /*
- * This data structure is followed by arrays of distance values and
- * distance null flags. Size of both arrays is
- * IndexScanDesc->numberOfOrderBys. See macros below for accessing those
- * arrays.
- */
+ /* numberOfOrderBys entries */
+ IndexOrderByDistance distances[FLEXIBLE_ARRAY_MEMBER];
} GISTSearchItem;
#define GISTSearchItemIsHeap(item) ((item).blkno == InvalidBlockNumber)
-#define SizeOfGISTSearchItem(n_distances) (DOUBLEALIGN(sizeof(GISTSearchItem)) + \
- (sizeof(double) + sizeof(bool)) * (n_distances))
-
-/*
- * We actually don't need n_distances compute pointer to distance values.
- * Nevertheless take n_distances as argument to have same arguments list for
- * GISTSearchItemDistanceValues() and GISTSearchItemDistanceNulls().
- */
-#define GISTSearchItemDistanceValues(item, n_distances) \
- ((double *) ((Pointer) (item) + DOUBLEALIGN(sizeof(GISTSearchItem))))
-
-#define GISTSearchItemDistanceNulls(item, n_distances) \
- ((bool *) ((Pointer) (item) + DOUBLEALIGN(sizeof(GISTSearchItem)) + sizeof(double) * (n_distances)))
+#define SizeOfGISTSearchItem(n_distances) \
+ (offsetof(GISTSearchItem, distances) + \
+ sizeof(IndexOrderByDistance) * (n_distances))
/*
* GISTScanOpaqueData: private state for a scan of a GiST index
@@ -176,8 +162,7 @@ typedef struct GISTScanOpaqueData
bool firstCall; /* true until first gistgettuple call */
/* pre-allocated workspace arrays */
- double *distanceValues; /* output area for gistindex_keytest */
- bool *distanceNulls;
+ IndexOrderByDistance *distances; /* output area for gistindex_keytest */
/* info about killed items if any (killedItems is NULL if never used) */
OffsetNumber *killedItems; /* offset numbers of killed items */
diff --git a/src/include/access/spgist.h b/src/include/access/spgist.h
index d787ab2..bcbcb1b 100644
--- a/src/include/access/spgist.h
+++ b/src/include/access/spgist.h
@@ -161,7 +161,7 @@ typedef struct spgInnerConsistentOut
int *levelAdds; /* increment level by this much for each */
Datum *reconstructedValues; /* associated reconstructed values */
void **traversalValues; /* opclass-specific traverse values */
- double **distances; /* associated distances */
+ IndexOrderByDistance **distances; /* associated distances */
} spgInnerConsistentOut;
/*
@@ -188,7 +188,7 @@ typedef struct spgLeafConsistentOut
Datum leafValue; /* reconstructed original data, if any */
bool recheck; /* set true if operator must be rechecked */
bool recheckDistances; /* set true if distances must be rechecked */
- double *distances; /* associated distances */
+ IndexOrderByDistance *distances; /* associated distances */
} spgLeafConsistentOut;
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index e0d1178..1bfe7fa 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -145,11 +145,12 @@ typedef struct SpGistSearchItem
bool recheckDistances; /* distance recheck is needed */
/* array with numberOfOrderBys entries */
- double distances[FLEXIBLE_ARRAY_MEMBER];
+ IndexOrderByDistance distances[FLEXIBLE_ARRAY_MEMBER];
} SpGistSearchItem;
#define SizeOfSpGistSearchItem(n_distances) \
- (offsetof(SpGistSearchItem, distances) + sizeof(double) * (n_distances))
+ (offsetof(SpGistSearchItem, distances) + \
+ sizeof(IndexOrderByDistance) * (n_distances))
/*
* Private state of an index scan
@@ -169,8 +170,12 @@ typedef struct SpGistScanOpaqueData
int numberOfKeys; /* number of index qualifier conditions */
ScanKey keyData; /* array of index qualifier descriptors */
int numberOfOrderBys; /* number of ordering operators */
+ int numberOfNonNullOrderBys; /* number of ordering operators
+ * with non-NULL arguments */
ScanKey orderByData; /* array of ordering op descriptors */
Oid *orderByTypes; /* array of ordering op return types */
+ int *nonNullOrderByOffsets; /* array of offset of non-NULL ordering
+ * keys in the original array */
Oid indexCollation; /* collation of index column */
/* Opclass defined functions: */
@@ -178,8 +183,8 @@ typedef struct SpGistScanOpaqueData
FmgrInfo leafConsistentFn;
/* Pre-allocated workspace arrays: */
- double *zeroDistances;
- double *infDistances;
+ IndexOrderByDistance *zeroDistances;
+ IndexOrderByDistance *infDistances;
/* These fields are only used in amgetbitmap scans: */
TIDBitmap *tbm; /* bitmap being filled */
@@ -195,7 +200,9 @@ typedef struct SpGistScanOpaqueData
bool recheckDistances[MaxIndexTuplesPerPage]; /* distance recheck
* flags */
HeapTuple reconTups[MaxIndexTuplesPerPage]; /* reconstructed tuples */
- double *distances[MaxIndexTuplesPerPage]; /* distances (for recheck) */
+
+ /* distances (for recheck) */
+ IndexOrderByDistance *distances[MaxIndexTuplesPerPage];
/*
* Note: using MaxIndexTuplesPerPage above is a bit hokey since
@@ -459,8 +466,9 @@ extern bool spgdoinsert(Relation index, SpGistState *state,
ItemPointer heapPtr, Datum datum, bool isnull);
/* spgproc.c */
-extern double *spg_key_orderbys_distances(Datum key, bool isLeaf,
- ScanKey orderbys, int norderbys);
+extern IndexOrderByDistance *spg_key_orderbys_distances(Datum key, bool isLeaf,
+ ScanKey orderbys,
+ int norderbys);
extern BOX *box_copy(BOX *orig);
#endif /* SPGIST_PRIVATE_H */
diff --git a/src/test/regress/expected/create_index_spgist.out b/src/test/regress/expected/create_index_spgist.out
index 81b4a67..5ccea91 100644
--- a/src/test/regress/expected/create_index_spgist.out
+++ b/src/test/regress/expected/create_index_spgist.out
@@ -555,6 +555,16 @@ WHERE seq.dist IS DISTINCT FROM idx.dist;
---+------+---+---+------+---
(0 rows)
+-- check ORDER BY distance to NULL
+SELECT (SELECT p FROM kd_point_tbl ORDER BY p <-> pt LIMIT 1)
+FROM (VALUES (point '1,2'), (NULL), ('1234,5678')) pts(pt);
+ p
+-------------
+ (59,21)
+ (9853,112)
+ (1239,5647)
+(3 rows)
+
EXPLAIN (COSTS OFF)
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
QUERY PLAN
diff --git a/src/test/regress/sql/create_index_spgist.sql b/src/test/regress/sql/create_index_spgist.sql
index 8e6c453..629936f 100644
--- a/src/test/regress/sql/create_index_spgist.sql
+++ b/src/test/regress/sql/create_index_spgist.sql
@@ -225,6 +225,11 @@ SELECT * FROM quad_point_tbl_ord_seq3 seq FULL JOIN kd_point_tbl_ord_idx3 idx
ON seq.n = idx.n
WHERE seq.dist IS DISTINCT FROM idx.dist;
+-- check ORDER BY distance to NULL
+SELECT (SELECT p FROM kd_point_tbl ORDER BY p <-> pt LIMIT 1)
+FROM (VALUES (point '1,2'), (NULL), ('1234,5678')) pts(pt);
+
+
EXPLAIN (COSTS OFF)
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
SELECT count(*) FROM radix_text_tbl WHERE t = 'P0123456789abcdef';
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index f3cdfa8..fb65265 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1043,6 +1043,7 @@ IndexList
IndexOnlyScan
IndexOnlyScanState
IndexOptInfo
+IndexOrderByDistance
IndexPath
IndexRuntimeKeyInfo
IndexScan
--
2.7.4
--------------E0A4FE4CE26F0B5C9EE7D9CB--
^ permalink raw reply [nested|flat] 9+ messages in thread
* Why is src/test/modules/committs/t/002_standby.pl flaky?
@ 2021-12-30 20:01 Thomas Munro <[email protected]>
2021-12-30 20:35 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Andrew Dunstan <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Munro @ 2021-12-30 20:01 UTC (permalink / raw)
To: pgsql-hackers
Hi,
There's a wait for replay that is open coded (instead of using the
wait_for_catchup() routine), and sometimes the second of two such
waits at line 51 (in master) times out after 3 minutes with "standby
never caught up". It's happening on three particular Windows boxes,
but once also happened on the AIX box "tern".
branch | animal | count
---------------+-----------+-------
HEAD | drongo | 1
HEAD | fairywren | 8
REL_10_STABLE | drongo | 3
REL_10_STABLE | fairywren | 10
REL_10_STABLE | jacana | 3
REL_11_STABLE | drongo | 1
REL_11_STABLE | fairywren | 4
REL_11_STABLE | jacana | 3
REL_12_STABLE | drongo | 2
REL_12_STABLE | fairywren | 5
REL_12_STABLE | jacana | 1
REL_12_STABLE | tern | 1
REL_13_STABLE | fairywren | 3
REL_14_STABLE | drongo | 2
REL_14_STABLE | fairywren | 6
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-30%2014:42:30
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-30%2013:13:22
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-30%2006:03:07
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-22%2011:37:37
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-22%2010:46:07
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-22%2009:03:06
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-17%2004:59:17
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-17%2003:59:51
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-16%2004:37:58
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-15%2009:57:14
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-15%2002:38:43
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-14%2020:42:15
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-14%2012:08:41
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-14%2000:35:32
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-13%2023:40:11
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-13%2022:47:25
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-09%2006:59:10
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-09%2006:04:04
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-09%2001:36:09
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-08%2019:20:35
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-08%2018:04:28
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-08%2014:12:32
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-12-08%2011:15:58
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2021-12-08%2004:04:22
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2021-12-03%2017:31:49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-11-11%2015:58:55
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-10-02%2022:00:17
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-09-09%2005:16:43
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-08-24%2004:45:09
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tern&dt=2021-07-17%2010:57:49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2021-06-12%2016:05:32
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-02-07%2012:59:43
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2020-03-24%2012:49:50
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2020-02-01%2018:00:27
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2020-02-01%2017:26:27
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2020-01-30%2023:49:49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2019-12-22%2014:19:02
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-12-13%2000:12:11
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-12-09%2006:02:05
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-12-06%2003:07:42
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-11-02%2014:41:04
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-25%2013:12:08
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-24%2013:12:41
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-23%2023:10:00
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-23%2018:00:39
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-22%2015:05:57
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-18%2013:29:49
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-16%2014:54:46
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-15%2014:21:11
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-14%2013:15:07
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-13%2014:19:41
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2019-10-12%2016:32:06
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2019-10-10%2013:12:09
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky?
2021-12-30 20:01 Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]>
@ 2021-12-30 20:35 ` Andrew Dunstan <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Dunstan @ 2021-12-30 20:35 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; pgsql-hackers
On 12/30/21 15:01, Thomas Munro wrote:
> Hi,
>
> There's a wait for replay that is open coded (instead of using the
> wait_for_catchup() routine), and sometimes the second of two such
> waits at line 51 (in master) times out after 3 minutes with "standby
> never caught up". It's happening on three particular Windows boxes,
> but once also happened on the AIX box "tern".
>
> branch | animal | count
> ---------------+-----------+-------
> HEAD | drongo | 1
> HEAD | fairywren | 8
> REL_10_STABLE | drongo | 3
> REL_10_STABLE | fairywren | 10
> REL_10_STABLE | jacana | 3
> REL_11_STABLE | drongo | 1
> REL_11_STABLE | fairywren | 4
> REL_11_STABLE | jacana | 3
> REL_12_STABLE | drongo | 2
> REL_12_STABLE | fairywren | 5
> REL_12_STABLE | jacana | 1
> REL_12_STABLE | tern | 1
> REL_13_STABLE | fairywren | 3
> REL_14_STABLE | drongo | 2
> REL_14_STABLE | fairywren | 6
>
>
FYI, drongo and fairywren are run on the same AWS/EC2 Windows Server
2019 instance. Nothing else runs on it.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Why is src/test/modules/committs/t/002_standby.pl flaky?
@ 2024-01-22 04:26 Peter Smith <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Peter Smith @ 2024-01-22 04:26 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; +Cc: Thomas Munro <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers
2024-01 Commitfest.
Hi, This patch has a CF status of "Needs Review" [1], but it seems
like there were CFbot test failures last time it was run [1]. Please
have a look and post an updated version if necessary.
======
1[] https://commitfest.postgresql.org/46/3523/
[1] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/3523
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Streaming read-ready sequential scan code
@ 2024-04-04 09:37 Thomas Munro <[email protected]>
2024-04-04 16:39 ` Re: Streaming read-ready sequential scan code Andres Freund <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Thomas Munro @ 2024-04-04 09:37 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; David Rowley <[email protected]>
On Thu, Apr 4, 2024 at 10:31 PM Thomas Munro <[email protected]> wrote:
> Alright what about this?
Forgot to git add a change, new version.
From 6dea2983abf8d608c34e02351d70694de99f25f2 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Thu, 4 Apr 2024 20:31:26 +1300
Subject: [PATCH v2 1/2] Allow BufferAccessStrategy to limit pin count.
When pinning extra buffers to look ahead, users of a strategy are in
danger of pinning a lot of the buffers in the ring, or even more than
the ring size. For some strategies, that means "escaping" from the
ring, and in others it means forcing dirty data to disk very frequently
with associated WAL flushing. Since external code has no insight into
any of that, allow individual strategy types to expose a clamp that
should be applied when deciding how many buffers to pin at once.
Discussion: https://postgr.es/m/CAAKRu_aJXnqsyZt6HwFLnxYEBgE17oypkxbKbT1t1geE_wvH2Q%40mail.gmail.com
---
src/backend/storage/aio/read_stream.c | 5 ++++
src/backend/storage/buffer/freelist.c | 35 +++++++++++++++++++++++++++
src/include/storage/bufmgr.h | 1 +
3 files changed, 41 insertions(+)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 4f21262ff5..eab87f6f73 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -419,6 +419,7 @@ read_stream_begin_relation(int flags,
size_t size;
int16 queue_size;
int16 max_ios;
+ int strategy_pin_limit;
uint32 max_pinned_buffers;
Oid tablespace_id;
SMgrRelation smgr;
@@ -460,6 +461,10 @@ read_stream_begin_relation(int flags,
max_pinned_buffers = Min(max_pinned_buffers,
PG_INT16_MAX - io_combine_limit - 1);
+ /* Give the strategy a chance to limit the number of buffers we pin. */
+ strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
+ max_pinned_buffers = Min(strategy_pin_limit, max_pinned_buffers);
+
/* Don't allow this backend to pin more than its share of buffers. */
if (SmgrIsTemp(smgr))
LimitAdditionalLocalPins(&max_pinned_buffers);
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index 3611357fa3..c69590d6d8 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -629,6 +629,41 @@ GetAccessStrategyBufferCount(BufferAccessStrategy strategy)
return strategy->nbuffers;
}
+/*
+ * GetAccessStrategyPinLimit -- get cap of number of buffers that can be pinned
+ *
+ * Strategies can specify the maximum number of buffers that a user should pin
+ * at once when performing look-ahead. Callers should combine this number with
+ * other relevant limits and take the minimum.
+ */
+int
+GetAccessStrategyPinLimit(BufferAccessStrategy strategy)
+{
+ if (strategy == NULL)
+ return NBuffers;
+
+ switch (strategy->btype)
+ {
+ case BAS_BULKREAD:
+
+ /*
+ * Since BAS_BULKREAD uses StrategyRejectBuffer(), dirty buffers
+ * shouldn't be a problem and the caller is free to pin up to the
+ * entire ring at once.
+ */
+ return strategy->nbuffers;
+
+ default:
+
+ /*
+ * Tell call not to pin more than half the buffers in the ring.
+ * This is a trade-off between look ahead distance and deferring
+ * writeback and associated WAL traffic.
+ */
+ return strategy->nbuffers / 2;
+ }
+}
+
/*
* FreeAccessStrategy -- release a BufferAccessStrategy object
*
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index f380f9d9a6..07ba1a6050 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -318,6 +318,7 @@ extern BufferAccessStrategy GetAccessStrategy(BufferAccessStrategyType btype);
extern BufferAccessStrategy GetAccessStrategyWithSize(BufferAccessStrategyType btype,
int ring_size_kb);
extern int GetAccessStrategyBufferCount(BufferAccessStrategy strategy);
+extern int GetAccessStrategyPinLimit(BufferAccessStrategy strategy);
extern void FreeAccessStrategy(BufferAccessStrategy strategy);
--
2.39.3 (Apple Git-146)
From e610bc78a2e3ecee50bd897e35084469d00bbac5 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Thu, 4 Apr 2024 21:11:06 +1300
Subject: [PATCH v2 2/2] Increase default vacuum_buffer_usage_limit to 2MB.
The BAS_VACUUM ring size has been 256kB since commit d526575f. Commit
1cbbee03 made it configurable but retained the traditional default.
The correct default size has been debated for years, but 256kB is
certainly very small. VACUUM soon needs to write back data it dirtied
only 32 blocks ago, which usually requires flushing the WAL. New
experiments in prefetching pages for VACUUM exacerbated the problem by
crashing into dirty data even sooner. Let's make the default 2MB.
That's 1.5% of the default toy buffer pool size, and 0.2% of 1GB, which
would be a considered a small shared_buffers setting for a real system
these days. Users are still free to set the GUC to a different value.
Discussion: https://postgr.es/m/20240403221257.md4gfki3z75cdyf6%40awork3.anarazel.de
Discussion: https://postgre.es/m/CA%2BhUKGLY4Q4ZY4f1rvnFtv6%2BPkjNf8MejdPkcju3Qii9DYqqcQ%40mail.gmail.com
---
doc/src/sgml/config.sgml | 2 +-
src/backend/storage/buffer/freelist.c | 2 +-
src/backend/utils/init/globals.c | 2 +-
src/backend/utils/misc/guc_tables.c | 2 +-
src/backend/utils/misc/postgresql.conf.sample | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 624518e0b0..d8e1282e12 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1961,7 +1961,7 @@ include_dir 'conf.d'
valid sizes range from <literal>128 kB</literal> to
<literal>16 GB</literal>. If the specified size would exceed 1/8 the
size of <varname>shared_buffers</varname>, the size is silently capped
- to that value. The default value is <literal>256 kB</literal>. If
+ to that value. The default value is <literal>2MB</literal>. If
this value is specified without units, it is taken as kilobytes. This
parameter can be set at any time. It can be overridden for
<xref linkend="sql-vacuum"/> and <xref linkend="sql-analyze"/>
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index c69590d6d8..65a6b3b357 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -561,7 +561,7 @@ GetAccessStrategy(BufferAccessStrategyType btype)
ring_size_kb = 16 * 1024;
break;
case BAS_VACUUM:
- ring_size_kb = 256;
+ ring_size_kb = 2048;
break;
default:
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 3e38bb1311..cc61937eef 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -143,7 +143,7 @@ int max_parallel_workers = 8;
int MaxBackends = 0;
/* GUC parameters for vacuum */
-int VacuumBufferUsageLimit = 256;
+int VacuumBufferUsageLimit = 2048;
int VacuumCostPageHit = 1;
int VacuumCostPageMiss = 2;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c12784cbec..7d4e4387cf 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2275,7 +2275,7 @@ struct config_int ConfigureNamesInt[] =
GUC_UNIT_KB
},
&VacuumBufferUsageLimit,
- 256, 0, MAX_BAS_VAC_RING_SIZE_KB,
+ 2048, 0, MAX_BAS_VAC_RING_SIZE_KB,
check_vacuum_buffer_usage_limit, NULL, NULL
},
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index baecde2841..2166ea4a87 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -166,7 +166,7 @@
# mmap
# (change requires restart)
#min_dynamic_shared_memory = 0MB # (change requires restart)
-#vacuum_buffer_usage_limit = 256kB # size of vacuum and analyze buffer access strategy ring;
+#vacuum_buffer_usage_limit = 2MB # size of vacuum and analyze buffer access strategy ring;
# 0 to disable vacuum buffer access strategy;
# range 128kB to 16GB
--
2.39.3 (Apple Git-146)
Attachments:
[text/plain] v2-0001-Allow-BufferAccessStrategy-to-limit-pin-count.txt (3.8K, ../../CA+hUKG+jv4AVmxPEBD2bA=D4M+0idRmU8U0g+vYVRrEUL-6Vvw@mail.gmail.com/2-v2-0001-Allow-BufferAccessStrategy-to-limit-pin-count.txt)
download | inline diff:
From 6dea2983abf8d608c34e02351d70694de99f25f2 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Thu, 4 Apr 2024 20:31:26 +1300
Subject: [PATCH v2 1/2] Allow BufferAccessStrategy to limit pin count.
When pinning extra buffers to look ahead, users of a strategy are in
danger of pinning a lot of the buffers in the ring, or even more than
the ring size. For some strategies, that means "escaping" from the
ring, and in others it means forcing dirty data to disk very frequently
with associated WAL flushing. Since external code has no insight into
any of that, allow individual strategy types to expose a clamp that
should be applied when deciding how many buffers to pin at once.
Discussion: https://postgr.es/m/CAAKRu_aJXnqsyZt6HwFLnxYEBgE17oypkxbKbT1t1geE_wvH2Q%40mail.gmail.com
---
src/backend/storage/aio/read_stream.c | 5 ++++
src/backend/storage/buffer/freelist.c | 35 +++++++++++++++++++++++++++
src/include/storage/bufmgr.h | 1 +
3 files changed, 41 insertions(+)
diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 4f21262ff5..eab87f6f73 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -419,6 +419,7 @@ read_stream_begin_relation(int flags,
size_t size;
int16 queue_size;
int16 max_ios;
+ int strategy_pin_limit;
uint32 max_pinned_buffers;
Oid tablespace_id;
SMgrRelation smgr;
@@ -460,6 +461,10 @@ read_stream_begin_relation(int flags,
max_pinned_buffers = Min(max_pinned_buffers,
PG_INT16_MAX - io_combine_limit - 1);
+ /* Give the strategy a chance to limit the number of buffers we pin. */
+ strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
+ max_pinned_buffers = Min(strategy_pin_limit, max_pinned_buffers);
+
/* Don't allow this backend to pin more than its share of buffers. */
if (SmgrIsTemp(smgr))
LimitAdditionalLocalPins(&max_pinned_buffers);
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index 3611357fa3..c69590d6d8 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -629,6 +629,41 @@ GetAccessStrategyBufferCount(BufferAccessStrategy strategy)
return strategy->nbuffers;
}
+/*
+ * GetAccessStrategyPinLimit -- get cap of number of buffers that can be pinned
+ *
+ * Strategies can specify the maximum number of buffers that a user should pin
+ * at once when performing look-ahead. Callers should combine this number with
+ * other relevant limits and take the minimum.
+ */
+int
+GetAccessStrategyPinLimit(BufferAccessStrategy strategy)
+{
+ if (strategy == NULL)
+ return NBuffers;
+
+ switch (strategy->btype)
+ {
+ case BAS_BULKREAD:
+
+ /*
+ * Since BAS_BULKREAD uses StrategyRejectBuffer(), dirty buffers
+ * shouldn't be a problem and the caller is free to pin up to the
+ * entire ring at once.
+ */
+ return strategy->nbuffers;
+
+ default:
+
+ /*
+ * Tell call not to pin more than half the buffers in the ring.
+ * This is a trade-off between look ahead distance and deferring
+ * writeback and associated WAL traffic.
+ */
+ return strategy->nbuffers / 2;
+ }
+}
+
/*
* FreeAccessStrategy -- release a BufferAccessStrategy object
*
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index f380f9d9a6..07ba1a6050 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -318,6 +318,7 @@ extern BufferAccessStrategy GetAccessStrategy(BufferAccessStrategyType btype);
extern BufferAccessStrategy GetAccessStrategyWithSize(BufferAccessStrategyType btype,
int ring_size_kb);
extern int GetAccessStrategyBufferCount(BufferAccessStrategy strategy);
+extern int GetAccessStrategyPinLimit(BufferAccessStrategy strategy);
extern void FreeAccessStrategy(BufferAccessStrategy strategy);
--
2.39.3 (Apple Git-146)
[text/plain] v2-0002-Increase-default-vacuum_buffer_usage_limit-to-2MB.txt (4.1K, ../../CA+hUKG+jv4AVmxPEBD2bA=D4M+0idRmU8U0g+vYVRrEUL-6Vvw@mail.gmail.com/3-v2-0002-Increase-default-vacuum_buffer_usage_limit-to-2MB.txt)
download | inline diff:
From e610bc78a2e3ecee50bd897e35084469d00bbac5 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Thu, 4 Apr 2024 21:11:06 +1300
Subject: [PATCH v2 2/2] Increase default vacuum_buffer_usage_limit to 2MB.
The BAS_VACUUM ring size has been 256kB since commit d526575f. Commit
1cbbee03 made it configurable but retained the traditional default.
The correct default size has been debated for years, but 256kB is
certainly very small. VACUUM soon needs to write back data it dirtied
only 32 blocks ago, which usually requires flushing the WAL. New
experiments in prefetching pages for VACUUM exacerbated the problem by
crashing into dirty data even sooner. Let's make the default 2MB.
That's 1.5% of the default toy buffer pool size, and 0.2% of 1GB, which
would be a considered a small shared_buffers setting for a real system
these days. Users are still free to set the GUC to a different value.
Discussion: https://postgr.es/m/20240403221257.md4gfki3z75cdyf6%40awork3.anarazel.de
Discussion: https://postgre.es/m/CA%2BhUKGLY4Q4ZY4f1rvnFtv6%2BPkjNf8MejdPkcju3Qii9DYqqcQ%40mail.gmail.com
---
doc/src/sgml/config.sgml | 2 +-
src/backend/storage/buffer/freelist.c | 2 +-
src/backend/utils/init/globals.c | 2 +-
src/backend/utils/misc/guc_tables.c | 2 +-
src/backend/utils/misc/postgresql.conf.sample | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 624518e0b0..d8e1282e12 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1961,7 +1961,7 @@ include_dir 'conf.d'
valid sizes range from <literal>128 kB</literal> to
<literal>16 GB</literal>. If the specified size would exceed 1/8 the
size of <varname>shared_buffers</varname>, the size is silently capped
- to that value. The default value is <literal>256 kB</literal>. If
+ to that value. The default value is <literal>2MB</literal>. If
this value is specified without units, it is taken as kilobytes. This
parameter can be set at any time. It can be overridden for
<xref linkend="sql-vacuum"/> and <xref linkend="sql-analyze"/>
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index c69590d6d8..65a6b3b357 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -561,7 +561,7 @@ GetAccessStrategy(BufferAccessStrategyType btype)
ring_size_kb = 16 * 1024;
break;
case BAS_VACUUM:
- ring_size_kb = 256;
+ ring_size_kb = 2048;
break;
default:
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 3e38bb1311..cc61937eef 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -143,7 +143,7 @@ int max_parallel_workers = 8;
int MaxBackends = 0;
/* GUC parameters for vacuum */
-int VacuumBufferUsageLimit = 256;
+int VacuumBufferUsageLimit = 2048;
int VacuumCostPageHit = 1;
int VacuumCostPageMiss = 2;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c12784cbec..7d4e4387cf 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2275,7 +2275,7 @@ struct config_int ConfigureNamesInt[] =
GUC_UNIT_KB
},
&VacuumBufferUsageLimit,
- 256, 0, MAX_BAS_VAC_RING_SIZE_KB,
+ 2048, 0, MAX_BAS_VAC_RING_SIZE_KB,
check_vacuum_buffer_usage_limit, NULL, NULL
},
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index baecde2841..2166ea4a87 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -166,7 +166,7 @@
# mmap
# (change requires restart)
#min_dynamic_shared_memory = 0MB # (change requires restart)
-#vacuum_buffer_usage_limit = 256kB # size of vacuum and analyze buffer access strategy ring;
+#vacuum_buffer_usage_limit = 2MB # size of vacuum and analyze buffer access strategy ring;
# 0 to disable vacuum buffer access strategy;
# range 128kB to 16GB
--
2.39.3 (Apple Git-146)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Streaming read-ready sequential scan code
2024-04-04 09:37 Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]>
@ 2024-04-04 16:39 ` Andres Freund <[email protected]>
2024-04-05 15:31 ` Re: Streaming read-ready sequential scan code Melanie Plageman <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2024-04-04 16:39 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Melanie Plageman <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; David Rowley <[email protected]>
Hi,
On 2024-04-04 22:37:39 +1300, Thomas Munro wrote:
> On Thu, Apr 4, 2024 at 10:31 PM Thomas Munro <[email protected]> wrote:
> > Alright what about this?
I think it's probably worth adding a bit more of the commit message to the
function comment. Yes, there's a bit in one of the return branches, but that's
not what you're going to look at when just checking what the function does.
> From e610bc78a2e3ecee50bd897e35084469d00bbac5 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <[email protected]>
> Date: Thu, 4 Apr 2024 21:11:06 +1300
> Subject: [PATCH v2 2/2] Increase default vacuum_buffer_usage_limit to 2MB.
>
> The BAS_VACUUM ring size has been 256kB since commit d526575f. Commit
> 1cbbee03 made it configurable but retained the traditional default.
> The correct default size has been debated for years, but 256kB is
> certainly very small. VACUUM soon needs to write back data it dirtied
> only 32 blocks ago, which usually requires flushing the WAL. New
> experiments in prefetching pages for VACUUM exacerbated the problem by
> crashing into dirty data even sooner. Let's make the default 2MB.
> That's 1.5% of the default toy buffer pool size, and 0.2% of 1GB, which
> would be a considered a small shared_buffers setting for a real system
> these days. Users are still free to set the GUC to a different value.
+1. Independent of any other changes, this improves the default vacuum
performance substantially. We might want to dynamically size the default at
some point - but we probably should overhaul the infrastructure first...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Streaming read-ready sequential scan code
2024-04-04 09:37 Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]>
2024-04-04 16:39 ` Re: Streaming read-ready sequential scan code Andres Freund <[email protected]>
@ 2024-04-05 15:31 ` Melanie Plageman <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Melanie Plageman @ 2024-04-05 15:31 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; David Rowley <[email protected]>
On Thu, Apr 4, 2024 at 12:39 PM Andres Freund <[email protected]> wrote:
>
> On 2024-04-04 22:37:39 +1300, Thomas Munro wrote:
> > On Thu, Apr 4, 2024 at 10:31 PM Thomas Munro <[email protected]> wrote:
> > > Alright what about this?
>
> I think it's probably worth adding a bit more of the commit message to the
> function comment. Yes, there's a bit in one of the return branches, but that's
> not what you're going to look at when just checking what the function does.
Agreed about the comment. I kept thinking that BAS_BULKREAD should
maybe return nbuffers - 1, but I couldn't convince myself why.
Otherwise v2-0001-Allow-BufferAccessStrategy-to-limit-pin-count LGTM.
- Melanie
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2024-04-05 15:31 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-09-10 14:26 [PATCH 1/5] Fix GiST and SP-GiST ordering by distance for NULLs Nikita Glukhov <[email protected]>
2019-09-10 14:26 [PATCH 1/4] Fix GiST and SP-GiST ordering by distance for NULLs and NaNs Nikita Glukhov <[email protected]>
2019-09-10 14:26 [PATCH 1/4] Fix GiST and SP-GiST ordering by distance for NULLs Nikita Glukhov <[email protected]>
2021-12-30 20:01 Why is src/test/modules/committs/t/002_standby.pl flaky? Thomas Munro <[email protected]>
2021-12-30 20:35 ` Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Andrew Dunstan <[email protected]>
2024-01-22 04:26 Re: Why is src/test/modules/committs/t/002_standby.pl flaky? Peter Smith <[email protected]>
2024-04-04 09:37 Re: Streaming read-ready sequential scan code Thomas Munro <[email protected]>
2024-04-04 16:39 ` Re: Streaming read-ready sequential scan code Andres Freund <[email protected]>
2024-04-05 15:31 ` Re: Streaming read-ready sequential scan code Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox