public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/9] Move IS [NOT] NULL handling from BRIN support functions
5+ messages / 3 participants
[nested] [flat]
* [PATCH 2/9] Move IS [NOT] NULL handling from BRIN support functions
@ 2020-09-17 15:26 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Vondra @ 2020-09-17 15:26 UTC (permalink / raw)
The handling of IS [NOT] NULL clauses is independent of an opclass, and
most of the code was exactly the same in both minmax and inclusion. So
instead move the code from support procedures to the AM methods etc.
This simplifies the code quite a bit - especially the support procedures
quite a bit, as they don't need to care about NULL values and flags at
all. It also means the IS [NOT] NULL clauses can be evaluated without
invoking the support procedure at all.
Author: Tomas Vondra <[email protected]>
Author: Nikita Glukhov <[email protected]>
Reviewed-by: Alvaro Herrera <[email protected]>
Reviewed-by: Mark Dilger <[email protected]>
Reviewed-by: Alexander Korotkov <[email protected]>
Reviewed-by: Masahiko Sawada <[email protected]>
Reviewed-by: John Naylor <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/brin/brin.c | 293 +++++++++++++++++------
src/backend/access/brin/brin_inclusion.c | 106 +-------
src/backend/access/brin/brin_minmax.c | 103 +-------
src/include/access/brin_internal.h | 3 +
4 files changed, 239 insertions(+), 266 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index dc187153aa..14da9ed17f 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -35,6 +35,7 @@
#include "storage/freespace.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/datum.h"
#include "utils/index_selfuncs.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -77,7 +78,9 @@ static void form_and_insert_tuple(BrinBuildState *state);
static void union_tuples(BrinDesc *bdesc, BrinMemTuple *a,
BrinTuple *b);
static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy);
-
+static bool add_values_to_range(Relation idxRel, BrinDesc *bdesc,
+ BrinMemTuple *dtup, Datum *values, bool *nulls);
+static bool check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys);
/*
* BRIN handler function: return IndexAmRoutine with access method parameters
@@ -179,7 +182,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
OffsetNumber off;
BrinTuple *brtup;
BrinMemTuple *dtup;
- int keyno;
CHECK_FOR_INTERRUPTS();
@@ -243,31 +245,7 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
dtup = brin_deform_tuple(bdesc, brtup, NULL);
- /*
- * Compare the key values of the new tuple to the stored index values;
- * our deformed tuple will get updated if the new tuple doesn't fit
- * the original range (note this means we can't break out of the loop
- * early). Make a note of whether this happens, so that we know to
- * insert the modified tuple later.
- */
- for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
- {
- Datum result;
- BrinValues *bval;
- FmgrInfo *addValue;
-
- bval = &dtup->bt_columns[keyno];
- addValue = index_getprocinfo(idxRel, keyno + 1,
- BRIN_PROCNUM_ADDVALUE);
- result = FunctionCall4Coll(addValue,
- idxRel->rd_indcollation[keyno],
- PointerGetDatum(bdesc),
- PointerGetDatum(bval),
- values[keyno],
- nulls[keyno]);
- /* if that returned true, we need to insert the updated tuple */
- need_insert |= DatumGetBool(result);
- }
+ need_insert = add_values_to_range(idxRel, bdesc, dtup, values, nulls);
if (!need_insert)
{
@@ -390,8 +368,10 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
BrinMemTuple *dtup;
BrinTuple *btup = NULL;
Size btupsz = 0;
- ScanKey **keys;
- int *nkeys;
+ ScanKey **keys,
+ **nullkeys;
+ int *nkeys,
+ *nnullkeys;
int keyno;
opaque = (BrinOpaque *) scan->opaque;
@@ -416,10 +396,13 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
/*
* Make room for per-attribute lists of scan keys that we'll pass to the
- * consistent support procedure.
+ * consistent support procedure. We keep null and regular keys separate,
+ * so that we can easily pass regular keys to the consistent function.
*/
keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
+ nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts);
nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
+ nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts);
/*
* Preprocess the scan keys - split them into per-attribute arrays.
@@ -440,23 +423,24 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
TupleDescAttr(bdesc->bd_tupdesc,
keyattno - 1)->attcollation));
- /* First time we see this index attribute, so init as needed. */
- if (!keys[keyattno-1])
+ /*
+ * First time we see this index attribute, so init as needed.
+ *
+ * This is a bit of an overkill - we don't know how many scan
+ * keys are there for a given attribute, so we simply allocate
+ * the largest number possible (as if all scan keys belonged to
+ * the same attribute). This may waste a bit of memory, but we
+ * only expect small number of scan keys in general, so this
+ * should be negligible, and it's probably cheaper than having
+ * to repalloc repeatedly.
+ */
+ if (consistentFn[keyattno - 1].fn_oid == InvalidOid)
{
FmgrInfo *tmp;
- /*
- * This is a bit of an overkill - we don't know how many
- * scan keys are there for this attribute, so we simply
- * allocate the largest number possible. This may waste
- * a bit of memory, but we only expect small number of
- * scan keys in general, so this should be negligible,
- * and it's cheaper than having to repalloc repeatedly.
- */
- keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
-
- /* First time this column, so look up consistent function */
- Assert(consistentFn[keyattno - 1].fn_oid == InvalidOid);
+ /* No key/null arrays for this attribute. */
+ Assert((keys[keyattno - 1] == NULL) && (nkeys[keyattno - 1] == 0));
+ Assert((nullkeys[keyattno - 1] == NULL) && (nnullkeys[keyattno - 1] == 0));
tmp = index_getprocinfo(idxRel, keyattno,
BRIN_PROCNUM_CONSISTENT);
@@ -464,9 +448,23 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
CurrentMemoryContext);
}
- /* Add key to the per-attribute array. */
- keys[keyattno - 1][nkeys[keyattno - 1]] = key;
- nkeys[keyattno - 1]++;
+ /* Add key to the proper per-attribute array. */
+ if (key->sk_flags & SK_ISNULL)
+ {
+ if (!nullkeys[keyattno - 1])
+ nullkeys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+ nullkeys[keyattno - 1][nnullkeys[keyattno - 1]] = key;
+ nnullkeys[keyattno - 1]++;
+ }
+ else
+ {
+ if (!keys[keyattno - 1])
+ keys[keyattno - 1] = palloc0(sizeof(ScanKey) * scan->numberOfKeys);
+
+ keys[keyattno - 1][nkeys[keyattno - 1]] = key;
+ nkeys[keyattno - 1]++;
+ }
}
/* allocate an initial in-memory tuple, out of the per-range memcxt */
@@ -544,15 +542,57 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
BrinValues *bval;
Datum add;
- /* skip attributes without any san keys */
- if (nkeys[attno - 1] == 0)
+ /*
+ * skip attributes without any scan keys (both regular
+ * and IS [NOT] NULL)
+ */
+ if (nkeys[attno - 1] == 0 && nnullkeys[attno - 1] == 0)
continue;
bval = &dtup->bt_columns[attno - 1];
+ /*
+ * First check if there are any IS [NOT] NULL scan keys,
+ * and if we're violating them. In that case we can
+ * terminate early, without invoking the support function.
+ *
+ * As there may be more keys, we can only detemine mismatch
+ * within this loop.
+ */
+ if (bdesc->bd_info[attno - 1]->oi_regular_nulls &&
+ !check_null_keys(bval, nullkeys[attno - 1],
+ nnullkeys[attno - 1]))
+ {
+ /*
+ * If any of the IS [NOT] NULL keys failed, the page
+ * range as a whole can't pass. So terminate the loop.
+ */
+ addrange = false;
+ break;
+ }
+
+ /*
+ * So either there are no IS [NOT] NULL keys, or all passed.
+ * If there are no regular scan keys, we're done - the page
+ * range matches. If there are regular keys, but the page
+ * range is marked as 'all nulls' it can't possibly pass
+ * (we're assuming the operators are strict).
+ */
+
+ /* No regular scan keys - page range as a whole passes. */
+ if (!nkeys[attno - 1])
+ continue;
+
Assert((nkeys[attno - 1] > 0) &&
(nkeys[attno - 1] <= scan->numberOfKeys));
+ /* If it is all nulls, it cannot possibly be consistent. */
+ if (bval->bv_allnulls)
+ {
+ addrange = false;
+ break;
+ }
+
/*
* Check whether the scan key is consistent with the page
* range values; if so, have the pages in the range added
@@ -695,7 +735,6 @@ brinbuildCallback(Relation index,
{
BrinBuildState *state = (BrinBuildState *) brstate;
BlockNumber thisblock;
- int i;
thisblock = ItemPointerGetBlockNumber(tid);
@@ -724,25 +763,8 @@ brinbuildCallback(Relation index,
}
/* Accumulate the current tuple into the running state */
- for (i = 0; i < state->bs_bdesc->bd_tupdesc->natts; i++)
- {
- FmgrInfo *addValue;
- BrinValues *col;
- Form_pg_attribute attr = TupleDescAttr(state->bs_bdesc->bd_tupdesc, i);
-
- col = &state->bs_dtuple->bt_columns[i];
- addValue = index_getprocinfo(index, i + 1,
- BRIN_PROCNUM_ADDVALUE);
-
- /*
- * Update dtuple state, if and as necessary.
- */
- FunctionCall4Coll(addValue,
- attr->attcollation,
- PointerGetDatum(state->bs_bdesc),
- PointerGetDatum(col),
- values[i], isnull[i]);
- }
+ (void) add_values_to_range(index, state->bs_bdesc, state->bs_dtuple,
+ values, isnull);
}
/*
@@ -1521,6 +1543,39 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b)
FmgrInfo *unionFn;
BrinValues *col_a = &a->bt_columns[keyno];
BrinValues *col_b = &db->bt_columns[keyno];
+ BrinOpcInfo *opcinfo = bdesc->bd_info[keyno];
+
+ if (opcinfo->oi_regular_nulls)
+ {
+ /* Adjust "hasnulls". */
+ if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
+ col_a->bv_hasnulls = true;
+
+ /* If there are no values in B, there's nothing left to do. */
+ if (col_b->bv_allnulls)
+ continue;
+
+ /*
+ * Adjust "allnulls". If A doesn't have values, just copy the
+ * values from B into A, and we're done. We cannot run the
+ * operators in this case, because values in A might contain
+ * garbage. Note we already established that B contains values.
+ */
+ if (col_a->bv_allnulls)
+ {
+ int i;
+
+ col_a->bv_allnulls = false;
+
+ for (i = 0; i < opcinfo->oi_nstored; i++)
+ col_a->bv_values[i] =
+ datumCopy(col_b->bv_values[i],
+ opcinfo->oi_typcache[i]->typbyval,
+ opcinfo->oi_typcache[i]->typlen);
+
+ continue;
+ }
+ }
unionFn = index_getprocinfo(bdesc->bd_index, keyno + 1,
BRIN_PROCNUM_UNION);
@@ -1574,3 +1629,103 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy)
*/
FreeSpaceMapVacuum(idxrel);
}
+
+static bool
+add_values_to_range(Relation idxRel, BrinDesc *bdesc, BrinMemTuple *dtup,
+ Datum *values, bool *nulls)
+{
+ int keyno;
+ bool modified = false;
+
+ /*
+ * Compare the key values of the new tuple to the stored index values;
+ * our deformed tuple will get updated if the new tuple doesn't fit
+ * the original range (note this means we can't break out of the loop
+ * early). Make a note of whether this happens, so that we know to
+ * insert the modified tuple later.
+ */
+ for (keyno = 0; keyno < bdesc->bd_tupdesc->natts; keyno++)
+ {
+ Datum result;
+ BrinValues *bval;
+ FmgrInfo *addValue;
+
+ bval = &dtup->bt_columns[keyno];
+
+ if (bdesc->bd_info[keyno]->oi_regular_nulls && nulls[keyno])
+ {
+ /*
+ * If the new value is null, we record that we saw it if it's
+ * the first one; otherwise, there's nothing to do.
+ */
+ if (!bval->bv_hasnulls)
+ {
+ bval->bv_hasnulls = true;
+ modified = true;
+ }
+
+ continue;
+ }
+
+ addValue = index_getprocinfo(idxRel, keyno + 1,
+ BRIN_PROCNUM_ADDVALUE);
+ result = FunctionCall4Coll(addValue,
+ idxRel->rd_indcollation[keyno],
+ PointerGetDatum(bdesc),
+ PointerGetDatum(bval),
+ values[keyno],
+ nulls[keyno]);
+ /* if that returned true, we need to insert the updated tuple */
+ modified |= DatumGetBool(result);
+ }
+
+ return modified;
+}
+
+static bool
+check_null_keys(BrinValues *bval, ScanKey *nullkeys, int nnullkeys)
+{
+ int keyno;
+
+ /*
+ * First check if there are any IS [NOT] NULL scan keys, and if we're
+ * violating them.
+ */
+ for (keyno = 0; keyno < nnullkeys; keyno++)
+ {
+ ScanKey key = nullkeys[keyno];
+
+ Assert(key->sk_attno == bval->bv_attno);
+
+ /* Handle only IS NULL/IS NOT NULL tests */
+ if (!(key->sk_flags & SK_ISNULL))
+ continue;
+
+ if (key->sk_flags & SK_SEARCHNULL)
+ {
+ /* IS NULL scan key, but range has no NULLs */
+ if (!bval->bv_allnulls && !bval->bv_hasnulls)
+ return false;
+ }
+ else if (key->sk_flags & SK_SEARCHNOTNULL)
+ {
+ /*
+ * For IS NOT NULL, we can only skip ranges that are known to
+ * have only nulls.
+ */
+ if (bval->bv_allnulls)
+ return false;
+ }
+ else
+ {
+ /*
+ * Neither IS NULL nor IS NOT NULL was used; assume all indexable
+ * operators are strict and thus return false with NULL value in
+ * the scan key.
+ */
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 215bc794d3..f4730be3b9 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -110,6 +110,7 @@ brin_inclusion_opcinfo(PG_FUNCTION_ARGS)
*/
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(3)) + sizeof(InclusionOpaque));
result->oi_nstored = 3;
+ result->oi_regular_nulls = true;
result->oi_opaque = (InclusionOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(3));
@@ -141,7 +142,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_BOOL(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_BOOL(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *finfo;
Datum result;
@@ -149,18 +150,7 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
AttrNumber attno;
Form_pg_attribute attr;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -264,63 +254,6 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
int nkeys = PG_GETARG_INT32(3);
Oid colloid = PG_GET_COLLATION();
int keyno;
- bool regular_keys = false;
-
- /*
- * First check if there are any IS NULL scan keys, and if we're
- * violating them. In that case we can terminate early, without
- * inspecting the ranges.
- */
- for (keyno = 0; keyno < nkeys; keyno++)
- {
- ScanKey key = keys[keyno];
-
- Assert(key->sk_attno == column->bv_attno);
-
- /* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
- {
- if (key->sk_flags & SK_SEARCHNULL)
- {
- if (column->bv_allnulls || column->bv_hasnulls)
- continue; /* this key is fine, continue */
-
- PG_RETURN_BOOL(false);
- }
-
- /*
- * For IS NOT NULL, we can only skip ranges that are known to have
- * only nulls.
- */
- if (key->sk_flags & SK_SEARCHNOTNULL)
- {
- if (column->bv_allnulls)
- PG_RETURN_BOOL(false);
-
- continue;
- }
-
- /*
- * Neither IS NULL nor IS NOT NULL was used; assume all indexable
- * operators are strict and return false.
- */
- PG_RETURN_BOOL(false);
- }
- else
- /* note we have regular (non-NULL) scan keys */
- regular_keys = true;
- }
-
- /*
- * If the page range is all nulls, it cannot possibly be consistent if
- * there are some regular scan keys.
- */
- if (column->bv_allnulls && regular_keys)
- PG_RETURN_BOOL(false);
-
- /* If there are no regular keys, the page range is considered consistent. */
- if (!regular_keys)
- PG_RETURN_BOOL(true);
/* It has to be checked, if it contains elements that are not mergeable. */
if (DatumGetBool(column->bv_values[INCLUSION_UNMERGEABLE]))
@@ -331,9 +264,8 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS)
{
ScanKey key = keys[keyno];
- /* ignore IS NULL/IS NOT NULL tests handled above */
- if (key->sk_flags & SK_ISNULL)
- continue;
+ /* NULL keys are handled and filtered-out in bringetbitmap */
+ Assert(!(key->sk_flags & SK_ISNULL));
/*
* When there are multiple scan keys, failure to meet the
@@ -572,37 +504,11 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
Datum result;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls". */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do. */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[INCLUSION_UNION] =
- datumCopy(col_b->bv_values[INCLUSION_UNION],
- attr->attbyval, attr->attlen);
- col_a->bv_values[INCLUSION_UNMERGEABLE] =
- col_b->bv_values[INCLUSION_UNMERGEABLE];
- col_a->bv_values[INCLUSION_CONTAINS_EMPTY] =
- col_b->bv_values[INCLUSION_CONTAINS_EMPTY];
- PG_RETURN_VOID();
- }
-
/* If B includes empty elements, mark A similarly, if needed. */
if (!DatumGetBool(col_a->bv_values[INCLUSION_CONTAINS_EMPTY]) &&
DatumGetBool(col_b->bv_values[INCLUSION_CONTAINS_EMPTY]))
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 12878ff3a0..6c8852d404 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -48,6 +48,7 @@ brin_minmax_opcinfo(PG_FUNCTION_ARGS)
result = palloc0(MAXALIGN(SizeofBrinOpcInfo(2)) +
sizeof(MinmaxOpaque));
result->oi_nstored = 2;
+ result->oi_regular_nulls = true;
result->oi_opaque = (MinmaxOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(2));
result->oi_typcache[0] = result->oi_typcache[1] =
@@ -69,7 +70,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0);
BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1);
Datum newval = PG_GETARG_DATUM(2);
- bool isnull = PG_GETARG_DATUM(3);
+ bool isnull PG_USED_FOR_ASSERTS_ONLY = PG_GETARG_DATUM(3);
Oid colloid = PG_GET_COLLATION();
FmgrInfo *cmpFn;
Datum compar;
@@ -77,18 +78,7 @@ brin_minmax_add_value(PG_FUNCTION_ARGS)
Form_pg_attribute attr;
AttrNumber attno;
- /*
- * If the new value is null, we record that we saw it if it's the first
- * one; otherwise, there's nothing to do.
- */
- if (isnull)
- {
- if (column->bv_hasnulls)
- PG_RETURN_BOOL(false);
-
- column->bv_hasnulls = true;
- PG_RETURN_BOOL(true);
- }
+ Assert(!isnull);
attno = column->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
@@ -152,72 +142,14 @@ brin_minmax_consistent(PG_FUNCTION_ARGS)
int nkeys = PG_GETARG_INT32(3);
Oid colloid = PG_GET_COLLATION();
int keyno;
- bool regular_keys = false;
-
- /*
- * First check if there are any IS NULL scan keys, and if we're
- * violating them. In that case we can terminate early, without
- * inspecting the ranges.
- */
- for (keyno = 0; keyno < nkeys; keyno++)
- {
- ScanKey key = keys[keyno];
-
- Assert(key->sk_attno == column->bv_attno);
-
- /* handle IS NULL/IS NOT NULL tests */
- if (key->sk_flags & SK_ISNULL)
- {
- if (key->sk_flags & SK_SEARCHNULL)
- {
- if (column->bv_allnulls || column->bv_hasnulls)
- continue; /* this key is fine, continue */
-
- PG_RETURN_BOOL(false);
- }
-
- /*
- * For IS NOT NULL, we can only skip ranges that are known to have
- * only nulls.
- */
- if (key->sk_flags & SK_SEARCHNOTNULL)
- {
- if (column->bv_allnulls)
- PG_RETURN_BOOL(false);
-
- continue;
- }
-
- /*
- * Neither IS NULL nor IS NOT NULL was used; assume all indexable
- * operators are strict and return false.
- */
- PG_RETURN_BOOL(false);
- }
- else
- /* note we have regular (non-NULL) scan keys */
- regular_keys = true;
- }
-
- /*
- * If the page range is all nulls, it cannot possibly be consistent if
- * there are some regular scan keys.
- */
- if (column->bv_allnulls && regular_keys)
- PG_RETURN_BOOL(false);
-
- /* If there are no regular keys, the page range is considered consistent. */
- if (!regular_keys)
- PG_RETURN_BOOL(true);
/* Check that the range is consistent with all scan keys. */
for (keyno = 0; keyno < nkeys; keyno++)
{
ScanKey key = keys[keyno];
- /* ignore IS NULL/IS NOT NULL tests handled above */
- if (key->sk_flags & SK_ISNULL)
- continue;
+ /* NULL keys are handled and filtered-out in bringetbitmap */
+ Assert(!(key->sk_flags & SK_ISNULL));
/*
* When there are multiple scan keys, failure to meet the
@@ -308,34 +240,11 @@ brin_minmax_union(PG_FUNCTION_ARGS)
bool needsadj;
Assert(col_a->bv_attno == col_b->bv_attno);
-
- /* Adjust "hasnulls" */
- if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
- col_a->bv_hasnulls = true;
-
- /* If there are no values in B, there's nothing left to do */
- if (col_b->bv_allnulls)
- PG_RETURN_VOID();
+ Assert(!col_a->bv_allnulls && !col_b->bv_allnulls);
attno = col_a->bv_attno;
attr = TupleDescAttr(bdesc->bd_tupdesc, attno - 1);
- /*
- * Adjust "allnulls". If A doesn't have values, just copy the values from
- * B into A, and we're done. We cannot run the operators in this case,
- * because values in A might contain garbage. Note we already established
- * that B contains values.
- */
- if (col_a->bv_allnulls)
- {
- col_a->bv_allnulls = false;
- col_a->bv_values[0] = datumCopy(col_b->bv_values[0],
- attr->attbyval, attr->attlen);
- col_a->bv_values[1] = datumCopy(col_b->bv_values[1],
- attr->attbyval, attr->attlen);
- PG_RETURN_VOID();
- }
-
/* Adjust minimum, if B's min is less than A's min */
finfo = minmax_get_strategy_procinfo(bdesc, attno, attr->atttypid,
BTLessStrategyNumber);
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 78c89a6961..79440ebe7b 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -27,6 +27,9 @@ typedef struct BrinOpcInfo
/* Number of columns stored in an index column of this opclass */
uint16 oi_nstored;
+ /* Regular processing of NULLs in BrinValues? */
+ bool oi_regular_nulls;
+
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
--
2.26.2
--------------0E72B707603BED22B4040825
Content-Type: text/x-patch; charset=UTF-8;
name="0001-Pass-all-scan-keys-to-BRIN-consistent-funct-20210211.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename*0="0001-Pass-all-scan-keys-to-BRIN-consistent-funct-20210211.pa";
filename*1="tch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: extended stats on partitioned tables
@ 2022-01-15 05:11 Justin Pryzby <[email protected]>
2022-01-15 18:35 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Justin Pryzby @ 2022-01-15 05:11 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Zhihong Yu <[email protected]>; Jaime Casanova <[email protected]>; pgsql-hackers
On Mon, Dec 13, 2021 at 09:40:09PM +0100, Tomas Vondra wrote:
> 1) If the table is a separate relation (not part of an inheritance
> tree), this should make no difference. -> OK
>
> 2) If the table is using "old" inheritance, this reverts back to
> pre-regression behavior. So people will keep using the old statistics
> until the ANALYZE, and we need to tell them to ANALYZE or something.
>
> 3) If the table is using partitioning, it's guaranteed to be empty and
> there are no stats at all. Again, we should tell people to run ANALYZE.
I think these can be mentioned in the commit message, which can end up in the
minor release notes as a recommendation to rerun ANALYZE.
Thanks for pushing 0001.
--
Justin
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: extended stats on partitioned tables
2022-01-15 05:11 Re: extended stats on partitioned tables Justin Pryzby <[email protected]>
@ 2022-01-15 18:35 ` Tomas Vondra <[email protected]>
2022-01-15 19:05 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2022-01-15 18:35 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Zhihong Yu <[email protected]>; Jaime Casanova <[email protected]>; pgsql-hackers
On 1/15/22 06:11, Justin Pryzby wrote:
> On Mon, Dec 13, 2021 at 09:40:09PM +0100, Tomas Vondra wrote:
>> 1) If the table is a separate relation (not part of an inheritance
>> tree), this should make no difference. -> OK
>>
>> 2) If the table is using "old" inheritance, this reverts back to
>> pre-regression behavior. So people will keep using the old statistics
>> until the ANALYZE, and we need to tell them to ANALYZE or something.
>>
>> 3) If the table is using partitioning, it's guaranteed to be empty and
>> there are no stats at all. Again, we should tell people to run ANALYZE.
>
> I think these can be mentioned in the commit message, which can end up in the
> minor release notes as a recommendation to rerun ANALYZE.
>
Good point. I pushed the 0002 part and added a short paragraph
suggesting ANALYZE might be necessary. I did not go into details about
the individual cases, because that'd be too much for a commit message.
> Thanks for pushing 0001.
>
Thanks for posting the patches!
I've pushed the second part - attached are the two remaining parts. I'll
wait a bit before pushing the rebased 0001 (which goes into master
branch only). Not sure about 0002 - I'm not convinced the refactored ACL
checks are an improvement, but I'll think about it.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] 0001-Add-stxdinherit-flag-to-pg_statistic_ext_da-20220115.patch (40.0K, ../../[email protected]/2-0001-Add-stxdinherit-flag-to-pg_statistic_ext_da-20220115.patch)
download | inline diff:
From e065e54d1961a4e295ebe77febbe6a3307d142b5 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Sun, 12 Dec 2021 00:07:27 +0100
Subject: [PATCH 1/2] Add stxdinherit flag to pg_statistic_ext_data
The support for extended statistics on inheritance trees was somewhat
problematic, because the catalog pg_statistic_ext_data did not have a
flag identifying whether the data was built with/without data for the
child relations.
For partitioned tables we've been able to work around this because the
non-leaf relations can't contain data, so there's really just one set of
statistics to store. But for regular inheritance trees we had to pick,
and there is no clearly better option - we need both.
This introduces the pg_statistic_ext_data.stxdinherit flag, analogous to
pg_statistic.stainherit, and modifies analyze to build statistics for
both cases.
This relaxes the relationship between the two catalogs - until now we've
created the pg_statistic_ext_data one when creating the statistics, and
then only updated it later. There was always 1:1 relationship between
rows in the two catalogs. With this change we no longer insert any data
rows while creating statistics, because we don't know which flag value
to create, and it seems wasteful to initialize both for every relation.
The there may be 0, 1 or 2 data rows for each statistics definition.
Patch by me, with extensive improvements and fixes by Justin Pryzby.
Author: Tomas Vondra, Justin Pryzby
Reviewed-by: Tomas Vondra, Justin Pryzby
Discussion: https://postgr.es/m/20210923212624.GI831%40telsasoft.com
---
doc/src/sgml/catalogs.sgml | 23 +++
src/backend/catalog/system_views.sql | 2 +
src/backend/commands/analyze.c | 28 +---
src/backend/commands/statscmds.c | 72 +++++-----
src/backend/optimizer/util/plancat.c | 147 ++++++++++++--------
src/backend/statistics/dependencies.c | 22 ++-
src/backend/statistics/extended_stats.c | 75 +++++-----
src/backend/statistics/mcv.c | 9 +-
src/backend/statistics/mvdistinct.c | 5 +-
src/backend/utils/adt/selfuncs.c | 37 +----
src/backend/utils/cache/syscache.c | 6 +-
src/include/catalog/pg_statistic_ext_data.h | 4 +-
src/include/commands/defrem.h | 1 +
src/include/nodes/pathnodes.h | 1 +
src/include/statistics/statistics.h | 11 +-
src/test/regress/expected/rules.out | 2 +
src/test/regress/expected/stats_ext.out | 31 +++--
src/test/regress/sql/stats_ext.sql | 13 +-
18 files changed, 254 insertions(+), 235 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 03e2537b07d..2aeb2ef346e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7521,6 +7521,19 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
created with <link linkend="sql-createstatistics"><command>CREATE STATISTICS</command></link>.
</para>
+ <para>
+ Normally there is one entry, with <structfield>stxdinherit</structfield> =
+ <literal>false</literal>, for each statistics object that has been analyzed.
+ If the table has inheritance children, a second entry with
+ <structfield>stxdinherit</structfield> = <literal>true</literal> is also created.
+ This row represents the statistics object over the inheritance tree, i.e.,
+ statistics for the data you'd see with
+ <literal>SELECT * FROM <replaceable>table</replaceable>*</literal>,
+ whereas the <structfield>stxdinherit</structfield> = <literal>false</literal> row
+ represents the results of
+ <literal>SELECT * FROM ONLY <replaceable>table</replaceable></literal>.
+ </para>
+
<para>
Like <link linkend="catalog-pg-statistic"><structname>pg_statistic</structname></link>,
<structname>pg_statistic_ext_data</structname> should not be
@@ -7560,6 +7573,16 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>stxdinherit</structfield> <type>bool</type>
+ </para>
+ <para>
+ If true, the stats include inheritance child columns, not just the
+ values in the specified relation
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>stxdndistinct</structfield> <type>pg_ndistinct</type>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 701ff38f761..3cb69b1f87b 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -266,6 +266,7 @@ CREATE VIEW pg_stats_ext WITH (security_barrier) AS
) AS attnames,
pg_get_statisticsobjdef_expressions(s.oid) as exprs,
s.stxkind AS kinds,
+ sd.stxdinherit AS inherited,
sd.stxdndistinct AS n_distinct,
sd.stxddependencies AS dependencies,
m.most_common_vals,
@@ -298,6 +299,7 @@ CREATE VIEW pg_stats_ext_exprs WITH (security_barrier) AS
s.stxname AS statistics_name,
pg_get_userbyid(s.stxowner) AS statistics_owner,
stat.expr,
+ sd.stxdinherit AS inherited,
(stat.a).stanullfrac AS null_frac,
(stat.a).stawidth AS avg_width,
(stat.a).stadistinct AS n_distinct,
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index d447bc9b436..a0da998c2ea 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -549,7 +549,6 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
{
MemoryContext col_context,
old_context;
- bool build_ext_stats;
pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
PROGRESS_ANALYZE_PHASE_COMPUTE_STATS);
@@ -613,30 +612,9 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
thisdata->attr_cnt, thisdata->vacattrstats);
}
- /*
- * Should we build extended statistics for this relation?
- *
- * The extended statistics catalog does not include an inheritance
- * flag, so we can't store statistics built both with and without
- * data from child relations. We can store just one set of statistics
- * per relation. For plain relations that's fine, but for inheritance
- * trees we have to pick whether to store statistics for just the
- * one relation or the whole tree. For plain inheritance we store
- * the (!inh) version, mostly for backwards compatibility reasons.
- * For partitioned tables that's pointless (the non-leaf tables are
- * always empty), so we store stats representing the whole tree.
- */
- build_ext_stats = (onerel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) ? inh : (!inh);
-
- /*
- * Build extended statistics (if there are any).
- *
- * For now we only build extended statistics on individual relations,
- * not for relations representing inheritance trees.
- */
- if (build_ext_stats)
- BuildRelationExtStatistics(onerel, totalrows, numrows, rows,
- attr_cnt, vacattrstats);
+ /* Build extended statistics (if there are any). */
+ BuildRelationExtStatistics(onerel, inh, totalrows, numrows, rows,
+ attr_cnt, vacattrstats);
}
pgstat_progress_update_param(PROGRESS_ANALYZE_PHASE,
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index f9d3f9c7b88..f7419b8f562 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -75,13 +75,10 @@ CreateStatistics(CreateStatsStmt *stmt)
HeapTuple htup;
Datum values[Natts_pg_statistic_ext];
bool nulls[Natts_pg_statistic_ext];
- Datum datavalues[Natts_pg_statistic_ext_data];
- bool datanulls[Natts_pg_statistic_ext_data];
int2vector *stxkeys;
List *stxexprs = NIL;
Datum exprsDatum;
Relation statrel;
- Relation datarel;
Relation rel = NULL;
Oid relid;
ObjectAddress parentobject,
@@ -514,28 +511,10 @@ CreateStatistics(CreateStatsStmt *stmt)
relation_close(statrel, RowExclusiveLock);
/*
- * Also build the pg_statistic_ext_data tuple, to hold the actual
- * statistics data.
+ * We used to create the pg_statistic_ext_data tuple too, but it's not clear
+ * what value should the stxdinherit flag have (it depends on whether the rel
+ * is partitioned, contains data, etc.)
*/
- datarel = table_open(StatisticExtDataRelationId, RowExclusiveLock);
-
- memset(datavalues, 0, sizeof(datavalues));
- memset(datanulls, false, sizeof(datanulls));
-
- datavalues[Anum_pg_statistic_ext_data_stxoid - 1] = ObjectIdGetDatum(statoid);
-
- /* no statistics built yet */
- datanulls[Anum_pg_statistic_ext_data_stxdndistinct - 1] = true;
- datanulls[Anum_pg_statistic_ext_data_stxddependencies - 1] = true;
- datanulls[Anum_pg_statistic_ext_data_stxdmcv - 1] = true;
- datanulls[Anum_pg_statistic_ext_data_stxdexpr - 1] = true;
-
- /* insert it into pg_statistic_ext_data */
- htup = heap_form_tuple(datarel->rd_att, datavalues, datanulls);
- CatalogTupleInsert(datarel, htup);
- heap_freetuple(htup);
-
- relation_close(datarel, RowExclusiveLock);
InvokeObjectPostCreateHook(StatisticExtRelationId, statoid, 0);
@@ -717,32 +696,49 @@ AlterStatistics(AlterStatsStmt *stmt)
}
/*
- * Guts of statistics object deletion.
+ * Delete entry in pg_statistic_ext_data catalog. We don't know if the row
+ * exists, so don't error out.
*/
void
-RemoveStatisticsById(Oid statsOid)
+RemoveStatisticsDataById(Oid statsOid, bool inh)
{
Relation relation;
HeapTuple tup;
- Form_pg_statistic_ext statext;
- Oid relid;
- /*
- * First delete the pg_statistic_ext_data tuple holding the actual
- * statistical data.
- */
relation = table_open(StatisticExtDataRelationId, RowExclusiveLock);
- tup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid));
-
- if (!HeapTupleIsValid(tup)) /* should not happen */
- elog(ERROR, "cache lookup failed for statistics data %u", statsOid);
+ tup = SearchSysCache2(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid),
+ BoolGetDatum(inh));
- CatalogTupleDelete(relation, &tup->t_self);
+ /* We don't know if the data row for inh value exists. */
+ if (HeapTupleIsValid(tup))
+ {
+ CatalogTupleDelete(relation, &tup->t_self);
- ReleaseSysCache(tup);
+ ReleaseSysCache(tup);
+ }
table_close(relation, RowExclusiveLock);
+}
+
+/*
+ * Guts of statistics object deletion.
+ */
+void
+RemoveStatisticsById(Oid statsOid)
+{
+ Relation relation;
+ HeapTuple tup;
+ Form_pg_statistic_ext statext;
+ Oid relid;
+
+ /*
+ * First delete the pg_statistic_ext_data tuples holding the actual
+ * statistical data. There might be data with/without inheritance, so
+ * attempt deleting both.
+ */
+ RemoveStatisticsDataById(statsOid, true);
+ RemoveStatisticsDataById(statsOid, false);
/*
* Delete the pg_statistic_ext tuple. Also send out a cache inval on the
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 535fa041ada..bd8c5e20f4f 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -30,6 +30,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_statistic_ext.h"
+#include "catalog/pg_statistic_ext_data.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
@@ -1276,6 +1277,87 @@ get_relation_constraints(PlannerInfo *root,
return result;
}
+/*
+ * Try loading data for the statistics object.
+ *
+ * We don't know if the data (specified by statOid and inh value) exist.
+ * The result is stored in stainfos list.
+ */
+static void
+get_relation_statistics_worker(List **stainfos, RelOptInfo *rel,
+ Oid statOid, bool inh,
+ Bitmapset *keys, List *exprs)
+{
+ Form_pg_statistic_ext_data dataForm;
+ HeapTuple dtup;
+
+ dtup = SearchSysCache2(STATEXTDATASTXOID,
+ ObjectIdGetDatum(statOid), BoolGetDatum(inh));
+ if (!HeapTupleIsValid(dtup))
+ return;
+
+ dataForm = (Form_pg_statistic_ext_data) GETSTRUCT(dtup);
+
+ /* add one StatisticExtInfo for each kind built */
+ if (statext_is_kind_built(dtup, STATS_EXT_NDISTINCT))
+ {
+ StatisticExtInfo *info = makeNode(StatisticExtInfo);
+
+ info->statOid = statOid;
+ info->inherit = dataForm->stxdinherit;
+ info->rel = rel;
+ info->kind = STATS_EXT_NDISTINCT;
+ info->keys = bms_copy(keys);
+ info->exprs = exprs;
+
+ *stainfos = lappend(*stainfos, info);
+ }
+
+ if (statext_is_kind_built(dtup, STATS_EXT_DEPENDENCIES))
+ {
+ StatisticExtInfo *info = makeNode(StatisticExtInfo);
+
+ info->statOid = statOid;
+ info->inherit = dataForm->stxdinherit;
+ info->rel = rel;
+ info->kind = STATS_EXT_DEPENDENCIES;
+ info->keys = bms_copy(keys);
+ info->exprs = exprs;
+
+ *stainfos = lappend(*stainfos, info);
+ }
+
+ if (statext_is_kind_built(dtup, STATS_EXT_MCV))
+ {
+ StatisticExtInfo *info = makeNode(StatisticExtInfo);
+
+ info->statOid = statOid;
+ info->inherit = dataForm->stxdinherit;
+ info->rel = rel;
+ info->kind = STATS_EXT_MCV;
+ info->keys = bms_copy(keys);
+ info->exprs = exprs;
+
+ *stainfos = lappend(*stainfos, info);
+ }
+
+ if (statext_is_kind_built(dtup, STATS_EXT_EXPRESSIONS))
+ {
+ StatisticExtInfo *info = makeNode(StatisticExtInfo);
+
+ info->statOid = statOid;
+ info->inherit = dataForm->stxdinherit;
+ info->rel = rel;
+ info->kind = STATS_EXT_EXPRESSIONS;
+ info->keys = bms_copy(keys);
+ info->exprs = exprs;
+
+ *stainfos = lappend(*stainfos, info);
+ }
+
+ ReleaseSysCache(dtup);
+}
+
/*
* get_relation_statistics
* Retrieve extended statistics defined on the table.
@@ -1299,7 +1381,6 @@ get_relation_statistics(RelOptInfo *rel, Relation relation)
Oid statOid = lfirst_oid(l);
Form_pg_statistic_ext staForm;
HeapTuple htup;
- HeapTuple dtup;
Bitmapset *keys = NULL;
List *exprs = NIL;
int i;
@@ -1309,10 +1390,6 @@ get_relation_statistics(RelOptInfo *rel, Relation relation)
elog(ERROR, "cache lookup failed for statistics object %u", statOid);
staForm = (Form_pg_statistic_ext) GETSTRUCT(htup);
- dtup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statOid));
- if (!HeapTupleIsValid(dtup))
- elog(ERROR, "cache lookup failed for statistics object %u", statOid);
-
/*
* First, build the array of columns covered. This is ultimately
* wasted if no stats within the object have actually been built, but
@@ -1324,6 +1401,11 @@ get_relation_statistics(RelOptInfo *rel, Relation relation)
/*
* Preprocess expressions (if any). We read the expressions, run them
* through eval_const_expressions, and fix the varnos.
+ *
+ * XXX We don't know yet if there are any data for this stats object,
+ * with either stxdinherit value. But it's reasonable to assume there
+ * is at least one of those, possibly both. So it's better to process
+ * keys and expressions here.
*/
{
bool isnull;
@@ -1364,61 +1446,14 @@ get_relation_statistics(RelOptInfo *rel, Relation relation)
}
}
- /* add one StatisticExtInfo for each kind built */
- if (statext_is_kind_built(dtup, STATS_EXT_NDISTINCT))
- {
- StatisticExtInfo *info = makeNode(StatisticExtInfo);
-
- info->statOid = statOid;
- info->rel = rel;
- info->kind = STATS_EXT_NDISTINCT;
- info->keys = bms_copy(keys);
- info->exprs = exprs;
-
- stainfos = lappend(stainfos, info);
- }
-
- if (statext_is_kind_built(dtup, STATS_EXT_DEPENDENCIES))
- {
- StatisticExtInfo *info = makeNode(StatisticExtInfo);
-
- info->statOid = statOid;
- info->rel = rel;
- info->kind = STATS_EXT_DEPENDENCIES;
- info->keys = bms_copy(keys);
- info->exprs = exprs;
-
- stainfos = lappend(stainfos, info);
- }
-
- if (statext_is_kind_built(dtup, STATS_EXT_MCV))
- {
- StatisticExtInfo *info = makeNode(StatisticExtInfo);
-
- info->statOid = statOid;
- info->rel = rel;
- info->kind = STATS_EXT_MCV;
- info->keys = bms_copy(keys);
- info->exprs = exprs;
-
- stainfos = lappend(stainfos, info);
- }
-
- if (statext_is_kind_built(dtup, STATS_EXT_EXPRESSIONS))
- {
- StatisticExtInfo *info = makeNode(StatisticExtInfo);
+ /* extract statistics for possible values of stxdinherit flag */
- info->statOid = statOid;
- info->rel = rel;
- info->kind = STATS_EXT_EXPRESSIONS;
- info->keys = bms_copy(keys);
- info->exprs = exprs;
+ get_relation_statistics_worker(&stainfos, rel, statOid, true, keys, exprs);
- stainfos = lappend(stainfos, info);
- }
+ get_relation_statistics_worker(&stainfos, rel, statOid, false, keys, exprs);
ReleaseSysCache(htup);
- ReleaseSysCache(dtup);
+
bms_free(keys);
}
diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c
index bbc29b67117..34326d55619 100644
--- a/src/backend/statistics/dependencies.c
+++ b/src/backend/statistics/dependencies.c
@@ -619,14 +619,16 @@ dependency_is_fully_matched(MVDependency *dependency, Bitmapset *attnums)
* Load the functional dependencies for the indicated pg_statistic_ext tuple
*/
MVDependencies *
-statext_dependencies_load(Oid mvoid)
+statext_dependencies_load(Oid mvoid, bool inh)
{
MVDependencies *result;
bool isnull;
Datum deps;
HeapTuple htup;
- htup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(mvoid));
+ htup = SearchSysCache2(STATEXTDATASTXOID,
+ ObjectIdGetDatum(mvoid),
+ BoolGetDatum(inh));
if (!HeapTupleIsValid(htup))
elog(ERROR, "cache lookup failed for statistics object %u", mvoid);
@@ -1417,16 +1419,6 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
Node **unique_exprs;
int unique_exprs_cnt;
- /*
- * When dealing with regular inheritance trees, ignore extended stats
- * (which were built without data from child rels, and thus do not
- * represent them). For partitioned tables data there's no data in the
- * non-leaf relations, so we build stats only for the inheritance tree.
- * So for partitioned tables we do consider extended stats.
- */
- if (rte->inh && rte->relkind != RELKIND_PARTITIONED_TABLE)
- return 1.0;
-
/* check if there's any stats that might be useful for us. */
if (!has_stats_of_kind(rel->statlist, STATS_EXT_DEPENDENCIES))
return 1.0;
@@ -1610,6 +1602,10 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
if (stat->kind != STATS_EXT_DEPENDENCIES)
continue;
+ /* skip statistics with mismatching stxdinherit value */
+ if (stat->inherit != rte->inh)
+ continue;
+
/*
* Count matching attributes - we have to undo the attnum offsets. The
* input attribute numbers are not offset (expressions are not
@@ -1656,7 +1652,7 @@ dependencies_clauselist_selectivity(PlannerInfo *root,
if (nmatched + nexprs < 2)
continue;
- deps = statext_dependencies_load(stat->statOid);
+ deps = statext_dependencies_load(stat->statOid, rte->inh);
/*
* The expressions may be represented by different attnums in the
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 5762621673b..87fe82ed114 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -25,6 +25,7 @@
#include "catalog/pg_statistic_ext.h"
#include "catalog/pg_statistic_ext_data.h"
#include "executor/executor.h"
+#include "commands/defrem.h"
#include "commands/progress.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
@@ -78,7 +79,7 @@ typedef struct StatExtEntry
static List *fetch_statentries_for_relation(Relation pg_statext, Oid relid);
static VacAttrStats **lookup_var_attr_stats(Relation rel, Bitmapset *attrs, List *exprs,
int nvacatts, VacAttrStats **vacatts);
-static void statext_store(Oid statOid,
+static void statext_store(Oid statOid, bool inh,
MVNDistinct *ndistinct, MVDependencies *dependencies,
MCVList *mcv, Datum exprs, VacAttrStats **stats);
static int statext_compute_stattarget(int stattarget,
@@ -111,7 +112,7 @@ static StatsBuildData *make_build_data(Relation onerel, StatExtEntry *stat,
* requested stats, and serializes them back into the catalog.
*/
void
-BuildRelationExtStatistics(Relation onerel, double totalrows,
+BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
int numrows, HeapTuple *rows,
int natts, VacAttrStats **vacattrstats)
{
@@ -231,7 +232,8 @@ BuildRelationExtStatistics(Relation onerel, double totalrows,
}
/* store the statistics in the catalog */
- statext_store(stat->statOid, ndistinct, dependencies, mcv, exprstats, stats);
+ statext_store(stat->statOid, inh,
+ ndistinct, dependencies, mcv, exprstats, stats);
/* for reporting progress */
pgstat_progress_update_param(PROGRESS_ANALYZE_EXT_STATS_COMPUTED,
@@ -782,23 +784,27 @@ lookup_var_attr_stats(Relation rel, Bitmapset *attrs, List *exprs,
* tuple.
*/
static void
-statext_store(Oid statOid,
+statext_store(Oid statOid, bool inh,
MVNDistinct *ndistinct, MVDependencies *dependencies,
MCVList *mcv, Datum exprs, VacAttrStats **stats)
{
Relation pg_stextdata;
- HeapTuple stup,
- oldtup;
+ HeapTuple stup;
Datum values[Natts_pg_statistic_ext_data];
bool nulls[Natts_pg_statistic_ext_data];
- bool replaces[Natts_pg_statistic_ext_data];
pg_stextdata = table_open(StatisticExtDataRelationId, RowExclusiveLock);
memset(nulls, true, sizeof(nulls));
- memset(replaces, false, sizeof(replaces));
memset(values, 0, sizeof(values));
+ /* basic info */
+ values[Anum_pg_statistic_ext_data_stxoid - 1] = ObjectIdGetDatum(statOid);
+ nulls[Anum_pg_statistic_ext_data_stxoid - 1] = false;
+
+ values[Anum_pg_statistic_ext_data_stxdinherit - 1] = BoolGetDatum(inh);
+ nulls[Anum_pg_statistic_ext_data_stxdinherit - 1] = false;
+
/*
* Construct a new pg_statistic_ext_data tuple, replacing the calculated
* stats.
@@ -831,25 +837,15 @@ statext_store(Oid statOid,
values[Anum_pg_statistic_ext_data_stxdexpr - 1] = exprs;
}
- /* always replace the value (either by bytea or NULL) */
- replaces[Anum_pg_statistic_ext_data_stxdndistinct - 1] = true;
- replaces[Anum_pg_statistic_ext_data_stxddependencies - 1] = true;
- replaces[Anum_pg_statistic_ext_data_stxdmcv - 1] = true;
- replaces[Anum_pg_statistic_ext_data_stxdexpr - 1] = true;
-
- /* there should already be a pg_statistic_ext_data tuple */
- oldtup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(statOid));
- if (!HeapTupleIsValid(oldtup))
- elog(ERROR, "cache lookup failed for statistics object %u", statOid);
-
- /* replace it */
- stup = heap_modify_tuple(oldtup,
- RelationGetDescr(pg_stextdata),
- values,
- nulls,
- replaces);
- ReleaseSysCache(oldtup);
- CatalogTupleUpdate(pg_stextdata, &stup->t_self, stup);
+ /*
+ * Delete the old tuple if it exists, and insert a new one. It's easier
+ * than trying to update or insert, based on various conditions.
+ */
+ RemoveStatisticsDataById(statOid, inh);
+
+ /* form and insert a new tuple */
+ stup = heap_form_tuple(RelationGetDescr(pg_stextdata), values, nulls);
+ CatalogTupleInsert(pg_stextdata, stup);
heap_freetuple(stup);
@@ -1235,7 +1231,7 @@ stat_covers_expressions(StatisticExtInfo *stat, List *exprs,
* further tiebreakers are needed.
*/
StatisticExtInfo *
-choose_best_statistics(List *stats, char requiredkind,
+choose_best_statistics(List *stats, char requiredkind, bool inh,
Bitmapset **clause_attnums, List **clause_exprs,
int nclauses)
{
@@ -1257,6 +1253,10 @@ choose_best_statistics(List *stats, char requiredkind,
if (info->kind != requiredkind)
continue;
+ /* skip statistics with mismatching inheritance flag */
+ if (info->inherit != inh)
+ continue;
+
/*
* Collect attributes and expressions in remaining (unestimated)
* clauses fully covered by this statistic object.
@@ -1697,16 +1697,6 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
Selectivity sel = (is_or) ? 0.0 : 1.0;
RangeTblEntry *rte = planner_rt_fetch(rel->relid, root);
- /*
- * When dealing with regular inheritance trees, ignore extended stats
- * (which were built without data from child rels, and thus do not
- * represent them). For partitioned tables data there's no data in the
- * non-leaf relations, so we build stats only for the inheritance tree.
- * So for partitioned tables we do consider extended stats.
- */
- if (rte->inh && rte->relkind != RELKIND_PARTITIONED_TABLE)
- return sel;
-
/* check if there's any stats that might be useful for us. */
if (!has_stats_of_kind(rel->statlist, STATS_EXT_MCV))
return sel;
@@ -1758,7 +1748,7 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
Bitmapset *simple_clauses;
/* find the best suited statistics object for these attnums */
- stat = choose_best_statistics(rel->statlist, STATS_EXT_MCV,
+ stat = choose_best_statistics(rel->statlist, STATS_EXT_MCV, rte->inh,
list_attnums, list_exprs,
list_length(clauses));
@@ -1847,7 +1837,7 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli
MCVList *mcv_list;
/* Load the MCV list stored in the statistics object */
- mcv_list = statext_mcv_load(stat->statOid);
+ mcv_list = statext_mcv_load(stat->statOid, rte->inh);
/*
* Compute the selectivity of the ORed list of clauses covered by
@@ -2408,7 +2398,7 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs)
* identified by the supplied index.
*/
HeapTuple
-statext_expressions_load(Oid stxoid, int idx)
+statext_expressions_load(Oid stxoid, bool inh, int idx)
{
bool isnull;
Datum value;
@@ -2418,7 +2408,8 @@ statext_expressions_load(Oid stxoid, int idx)
HeapTupleData tmptup;
HeapTuple tup;
- htup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(stxoid));
+ htup = SearchSysCache2(STATEXTDATASTXOID,
+ ObjectIdGetDatum(stxoid), BoolGetDatum(inh));
if (!HeapTupleIsValid(htup))
elog(ERROR, "cache lookup failed for statistics object %u", stxoid);
diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c
index 65fa87b1c79..2f6dfc02d80 100644
--- a/src/backend/statistics/mcv.c
+++ b/src/backend/statistics/mcv.c
@@ -559,12 +559,13 @@ build_column_frequencies(SortItem *groups, int ngroups,
* Load the MCV list for the indicated pg_statistic_ext tuple.
*/
MCVList *
-statext_mcv_load(Oid mvoid)
+statext_mcv_load(Oid mvoid, bool inh)
{
MCVList *result;
bool isnull;
Datum mcvlist;
- HeapTuple htup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(mvoid));
+ HeapTuple htup = SearchSysCache2(STATEXTDATASTXOID,
+ ObjectIdGetDatum(mvoid), BoolGetDatum(inh));
if (!HeapTupleIsValid(htup))
elog(ERROR, "cache lookup failed for statistics object %u", mvoid);
@@ -2039,11 +2040,13 @@ mcv_clauselist_selectivity(PlannerInfo *root, StatisticExtInfo *stat,
MCVList *mcv;
Selectivity s = 0.0;
+ RangeTblEntry *rte = root->simple_rte_array[rel->relid];
+
/* match/mismatch bitmap for each MCV item */
bool *matches = NULL;
/* load the MCV list stored in the statistics object */
- mcv = statext_mcv_load(stat->statOid);
+ mcv = statext_mcv_load(stat->statOid, rte->inh);
/* build a match bitmap for the clauses */
matches = mcv_get_match_bitmap(root, clauses, stat->keys, stat->exprs,
diff --git a/src/backend/statistics/mvdistinct.c b/src/backend/statistics/mvdistinct.c
index 55b831d4f50..6ade5eff78c 100644
--- a/src/backend/statistics/mvdistinct.c
+++ b/src/backend/statistics/mvdistinct.c
@@ -146,14 +146,15 @@ statext_ndistinct_build(double totalrows, StatsBuildData *data)
* Load the ndistinct value for the indicated pg_statistic_ext tuple
*/
MVNDistinct *
-statext_ndistinct_load(Oid mvoid)
+statext_ndistinct_load(Oid mvoid, bool inh)
{
MVNDistinct *result;
bool isnull;
Datum ndist;
HeapTuple htup;
- htup = SearchSysCache1(STATEXTDATASTXOID, ObjectIdGetDatum(mvoid));
+ htup = SearchSysCache2(STATEXTDATASTXOID,
+ ObjectIdGetDatum(mvoid), BoolGetDatum(inh));
if (!HeapTupleIsValid(htup))
elog(ERROR, "cache lookup failed for statistics object %u", mvoid);
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 5d56748f0a7..b00ee6a3bef 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3919,17 +3919,6 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
if (!rel->statlist)
return false;
- /*
- * When dealing with regular inheritance trees, ignore extended stats
- * (which were built without data from child rels, and thus do not
- * represent them). For partitioned tables data there's no data in the
- * non-leaf relations, so we build stats only for the inheritance tree.
- * So for partitioned tables we do consider extended stats.
- */
- rte = planner_rt_fetch(rel->relid, root);
- if (rte->inh && rte->relkind != RELKIND_PARTITIONED_TABLE)
- return false;
-
/* look for the ndistinct statistics object matching the most vars */
nmatches_vars = 0; /* we require at least two matches */
nmatches_exprs = 0;
@@ -4015,7 +4004,8 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
Assert(nmatches_vars + nmatches_exprs > 1);
- stats = statext_ndistinct_load(statOid);
+ rte = planner_rt_fetch(rel->relid, root);
+ stats = statext_ndistinct_load(statOid, rte->inh);
/*
* If we have a match, search it for the specific item that matches (there
@@ -5245,17 +5235,6 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
if (vardata->statsTuple)
break;
- /*
- * When dealing with regular inheritance trees, ignore extended
- * stats (which were built without data from child rels, and thus
- * do not represent them). For partitioned tables data there's no
- * data in the non-leaf relations, so we build stats only for the
- * inheritance tree. So for partitioned tables we do consider
- * extended stats.
- */
- if (rte->inh && rte->relkind != RELKIND_PARTITIONED_TABLE)
- break;
-
/* skip stats without per-expression stats */
if (info->kind != STATS_EXT_EXPRESSIONS)
continue;
@@ -5274,22 +5253,18 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
/* found a match, see if we can extract pg_statistic row */
if (equal(node, expr))
{
- HeapTuple t = statext_expressions_load(info->statOid, pos);
-
- /* Get statistics object's table for permission check */
- RangeTblEntry *rte;
Oid userid;
- vardata->statsTuple = t;
+ Assert(rte->rtekind == RTE_RELATION);
/*
* XXX Not sure if we should cache the tuple somewhere.
* Now we just create a new copy every time.
*/
- vardata->freefunc = ReleaseDummy;
+ vardata->statsTuple =
+ statext_expressions_load(info->statOid, rte->inh, pos);
- rte = planner_rt_fetch(onerel->relid, root);
- Assert(rte->rtekind == RTE_RELATION);
+ vardata->freefunc = ReleaseDummy;
/*
* Use checkAsUser if it's set, in case we're accessing
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index beeebfe59f1..f4e7819f1e2 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -763,11 +763,11 @@ static const struct cachedesc cacheinfo[] = {
32
},
{StatisticExtDataRelationId, /* STATEXTDATASTXOID */
- StatisticExtDataStxoidIndexId,
- 1,
+ StatisticExtDataStxoidInhIndexId,
+ 2,
{
Anum_pg_statistic_ext_data_stxoid,
- 0,
+ Anum_pg_statistic_ext_data_stxdinherit,
0,
0
},
diff --git a/src/include/catalog/pg_statistic_ext_data.h b/src/include/catalog/pg_statistic_ext_data.h
index 7c94d7b7cd7..b01e6205974 100644
--- a/src/include/catalog/pg_statistic_ext_data.h
+++ b/src/include/catalog/pg_statistic_ext_data.h
@@ -32,6 +32,7 @@ CATALOG(pg_statistic_ext_data,3429,StatisticExtDataRelationId)
{
Oid stxoid BKI_LOOKUP(pg_statistic_ext); /* statistics object
* this data is for */
+ bool stxdinherit; /* true if inheritance children are included */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
@@ -53,6 +54,7 @@ typedef FormData_pg_statistic_ext_data * Form_pg_statistic_ext_data;
DECLARE_TOAST(pg_statistic_ext_data, 3430, 3431);
-DECLARE_UNIQUE_INDEX_PKEY(pg_statistic_ext_data_stxoid_index, 3433, StatisticExtDataStxoidIndexId, on pg_statistic_ext_data using btree(stxoid oid_ops));
+DECLARE_UNIQUE_INDEX_PKEY(pg_statistic_ext_data_stxoid_inh_index, 3433, StatisticExtDataStxoidInhIndexId, on pg_statistic_ext_data using btree(stxoid oid_ops, stxdinherit bool_ops));
+
#endif /* PG_STATISTIC_EXT_DATA_H */
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 2d3cdddaf48..56d2bb66161 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -83,6 +83,7 @@ extern ObjectAddress AlterOperator(AlterOperatorStmt *stmt);
extern ObjectAddress CreateStatistics(CreateStatsStmt *stmt);
extern ObjectAddress AlterStatistics(AlterStatsStmt *stmt);
extern void RemoveStatisticsById(Oid statsOid);
+extern void RemoveStatisticsDataById(Oid statsOid, bool inh);
extern Oid StatisticsGetRelation(Oid statId, bool missing_ok);
/* commands/aggregatecmds.c */
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 1f33fe13c11..1f3845b3fec 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -934,6 +934,7 @@ typedef struct StatisticExtInfo
NodeTag type;
Oid statOid; /* OID of the statistics row */
+ bool inherit; /* includes child relations */
RelOptInfo *rel; /* back-link to statistic's table */
char kind; /* statistics kind of this entry */
Bitmapset *keys; /* attnums of the columns covered */
diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h
index 4a3676278de..bb7ef1240e0 100644
--- a/src/include/statistics/statistics.h
+++ b/src/include/statistics/statistics.h
@@ -94,11 +94,11 @@ typedef struct MCVList
MCVItem items[FLEXIBLE_ARRAY_MEMBER]; /* array of MCV items */
} MCVList;
-extern MVNDistinct *statext_ndistinct_load(Oid mvoid);
-extern MVDependencies *statext_dependencies_load(Oid mvoid);
-extern MCVList *statext_mcv_load(Oid mvoid);
+extern MVNDistinct *statext_ndistinct_load(Oid mvoid, bool inh);
+extern MVDependencies *statext_dependencies_load(Oid mvoid, bool inh);
+extern MCVList *statext_mcv_load(Oid mvoid, bool inh);
-extern void BuildRelationExtStatistics(Relation onerel, double totalrows,
+extern void BuildRelationExtStatistics(Relation onerel, bool inh, double totalrows,
int numrows, HeapTuple *rows,
int natts, VacAttrStats **vacattrstats);
extern int ComputeExtStatisticsRows(Relation onerel,
@@ -121,9 +121,10 @@ extern Selectivity statext_clauselist_selectivity(PlannerInfo *root,
bool is_or);
extern bool has_stats_of_kind(List *stats, char requiredkind);
extern StatisticExtInfo *choose_best_statistics(List *stats, char requiredkind,
+ bool inh,
Bitmapset **clause_attnums,
List **clause_exprs,
int nclauses);
-extern HeapTuple statext_expressions_load(Oid stxoid, int idx);
+extern HeapTuple statext_expressions_load(Oid stxoid, bool inh, int idx);
#endif /* STATISTICS_H */
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b58b062b10d..d652f7b5fb4 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2443,6 +2443,7 @@ pg_stats_ext| SELECT cn.nspname AS schemaname,
JOIN pg_attribute a ON (((a.attrelid = s.stxrelid) AND (a.attnum = k.k))))) AS attnames,
pg_get_statisticsobjdef_expressions(s.oid) AS exprs,
s.stxkind AS kinds,
+ sd.stxdinherit AS inherited,
sd.stxdndistinct AS n_distinct,
sd.stxddependencies AS dependencies,
m.most_common_vals,
@@ -2469,6 +2470,7 @@ pg_stats_ext_exprs| SELECT cn.nspname AS schemaname,
s.stxname AS statistics_name,
pg_get_userbyid(s.stxowner) AS statistics_owner,
stat.expr,
+ sd.stxdinherit AS inherited,
(stat.a).stanullfrac AS null_frac,
(stat.a).stawidth AS avg_width,
(stat.a).stadistinct AS n_distinct,
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index fd0cc54ea11..042316aeed8 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -140,13 +140,12 @@ Statistics objects:
"public.ab1_a_b_stats" ON a, b FROM ab1; STATISTICS 0
ANALYZE ab1;
-SELECT stxname, stxdndistinct, stxddependencies, stxdmcv
- FROM pg_statistic_ext s, pg_statistic_ext_data d
- WHERE s.stxname = 'ab1_a_b_stats'
- AND d.stxoid = s.oid;
- stxname | stxdndistinct | stxddependencies | stxdmcv
----------------+---------------+------------------+---------
- ab1_a_b_stats | | |
+SELECT stxname, stxdndistinct, stxddependencies, stxdmcv, stxdinherit
+ FROM pg_statistic_ext s LEFT JOIN pg_statistic_ext_data d ON (d.stxoid = s.oid)
+ WHERE s.stxname = 'ab1_a_b_stats';
+ stxname | stxdndistinct | stxddependencies | stxdmcv | stxdinherit
+---------------+---------------+------------------+---------+-------------
+ ab1_a_b_stats | | | |
(1 row)
ALTER STATISTICS ab1_a_b_stats SET STATISTICS -1;
@@ -200,12 +199,11 @@ SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* WHERE a = 0 AND b
CREATE STATISTICS stxdinh ON a, b FROM stxdinh;
VACUUM ANALYZE stxdinh, stxdinh1, stxdinh2;
--- Since the stats object does not include inherited stats, it should not
--- affect the estimates
+-- See if the extended stats affect the estimates
SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* GROUP BY 1, 2');
estimated | actual
-----------+--------
- 400 | 150
+ 150 | 150
(1 row)
-- Dependencies are applied at individual relations (within append), so
@@ -216,6 +214,19 @@ SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* WHERE a = 0 AND b
22 | 40
(1 row)
+-- Ensure correct (non-inherited) stats are applied to inherited query
+SELECT * FROM check_estimated_rows('SELECT a, b FROM ONLY stxdinh GROUP BY 1, 2');
+ estimated | actual
+-----------+--------
+ 100 | 100
+(1 row)
+
+SELECT * FROM check_estimated_rows('SELECT a, b FROM ONLY stxdinh WHERE a = 0 AND b = 0');
+ estimated | actual
+-----------+--------
+ 20 | 20
+(1 row)
+
DROP TABLE stxdinh, stxdinh1, stxdinh2;
-- Ensure inherited stats ARE applied to inherited query in partitioned table
CREATE TABLE stxdinp(i int, a int, b int) PARTITION BY RANGE (i);
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index d2c59b0a8ad..6b954c9e500 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -91,10 +91,9 @@ ALTER TABLE ab1 ALTER a SET STATISTICS -1;
ALTER STATISTICS ab1_a_b_stats SET STATISTICS 0;
\d ab1
ANALYZE ab1;
-SELECT stxname, stxdndistinct, stxddependencies, stxdmcv
- FROM pg_statistic_ext s, pg_statistic_ext_data d
- WHERE s.stxname = 'ab1_a_b_stats'
- AND d.stxoid = s.oid;
+SELECT stxname, stxdndistinct, stxddependencies, stxdmcv, stxdinherit
+ FROM pg_statistic_ext s LEFT JOIN pg_statistic_ext_data d ON (d.stxoid = s.oid)
+ WHERE s.stxname = 'ab1_a_b_stats';
ALTER STATISTICS ab1_a_b_stats SET STATISTICS -1;
\d+ ab1
-- partial analyze doesn't build stats either
@@ -126,12 +125,14 @@ SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* GROUP BY 1, 2');
SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* WHERE a = 0 AND b = 0');
CREATE STATISTICS stxdinh ON a, b FROM stxdinh;
VACUUM ANALYZE stxdinh, stxdinh1, stxdinh2;
--- Since the stats object does not include inherited stats, it should not
--- affect the estimates
+-- See if the extended stats affect the estimates
SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* GROUP BY 1, 2');
-- Dependencies are applied at individual relations (within append), so
-- this estimate changes a bit because we improve estimates for the parent
SELECT * FROM check_estimated_rows('SELECT a, b FROM stxdinh* WHERE a = 0 AND b = 0');
+-- Ensure correct (non-inherited) stats are applied to inherited query
+SELECT * FROM check_estimated_rows('SELECT a, b FROM ONLY stxdinh GROUP BY 1, 2');
+SELECT * FROM check_estimated_rows('SELECT a, b FROM ONLY stxdinh WHERE a = 0 AND b = 0');
DROP TABLE stxdinh, stxdinh1, stxdinh2;
-- Ensure inherited stats ARE applied to inherited query in partitioned table
--
2.31.1
[text/x-patch] 0002-Refactor-parent-ACL-check-20220115.patch (6.4K, ../../[email protected]/3-0002-Refactor-parent-ACL-check-20220115.patch)
download | inline diff:
From 95fc05ff7822e55c570c8fd6282dd3d6459bb594 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Sun, 12 Dec 2021 02:28:41 +0100
Subject: [PATCH 2/2] Refactor parent ACL check
selfuncs.c is 8k lines long, and this makes it 30 LOC shorter.
---
src/backend/utils/adt/selfuncs.c | 140 ++++++++++++-------------------
1 file changed, 52 insertions(+), 88 deletions(-)
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index b00ee6a3bef..b1ceb01775f 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -187,6 +187,8 @@ static char *convert_string_datum(Datum value, Oid typid, Oid collid,
bool *failure);
static double convert_timevalue_to_scalar(Datum value, Oid typid,
bool *failure);
+static void recheck_parent_acl(PlannerInfo *root, VariableStatData *vardata,
+ Oid relid);
static void examine_simple_variable(PlannerInfo *root, Var *var,
VariableStatData *vardata);
static bool get_variable_range(PlannerInfo *root, VariableStatData *vardata,
@@ -5153,51 +5155,7 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
(pg_class_aclcheck(rte->relid, userid,
ACL_SELECT) == ACLCHECK_OK);
- /*
- * If the user doesn't have permissions to
- * access an inheritance child relation, check
- * the permissions of the table actually
- * mentioned in the query, since most likely
- * the user does have that permission. Note
- * that whole-table select privilege on the
- * parent doesn't quite guarantee that the
- * user could read all columns of the child.
- * But in practice it's unlikely that any
- * interesting security violation could result
- * from allowing access to the expression
- * index's stats, so we allow it anyway. See
- * similar code in examine_simple_variable()
- * for additional comments.
- */
- if (!vardata->acl_ok &&
- root->append_rel_array != NULL)
- {
- AppendRelInfo *appinfo;
- Index varno = index->rel->relid;
-
- appinfo = root->append_rel_array[varno];
- while (appinfo &&
- planner_rt_fetch(appinfo->parent_relid,
- root)->rtekind == RTE_RELATION)
- {
- varno = appinfo->parent_relid;
- appinfo = root->append_rel_array[varno];
- }
- if (varno != index->rel->relid)
- {
- /* Repeat access check on this rel */
- rte = planner_rt_fetch(varno, root);
- Assert(rte->rtekind == RTE_RELATION);
-
- userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
-
- vardata->acl_ok =
- rte->securityQuals == NIL &&
- (pg_class_aclcheck(rte->relid,
- userid,
- ACL_SELECT) == ACLCHECK_OK);
- }
- }
+ recheck_parent_acl(root, vardata, index->rel->relid);
}
else
{
@@ -5285,49 +5243,7 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
(pg_class_aclcheck(rte->relid, userid,
ACL_SELECT) == ACLCHECK_OK);
- /*
- * If the user doesn't have permissions to access an
- * inheritance child relation, check the permissions of
- * the table actually mentioned in the query, since most
- * likely the user does have that permission. Note that
- * whole-table select privilege on the parent doesn't
- * quite guarantee that the user could read all columns of
- * the child. But in practice it's unlikely that any
- * interesting security violation could result from
- * allowing access to the expression stats, so we allow it
- * anyway. See similar code in examine_simple_variable()
- * for additional comments.
- */
- if (!vardata->acl_ok &&
- root->append_rel_array != NULL)
- {
- AppendRelInfo *appinfo;
- Index varno = onerel->relid;
-
- appinfo = root->append_rel_array[varno];
- while (appinfo &&
- planner_rt_fetch(appinfo->parent_relid,
- root)->rtekind == RTE_RELATION)
- {
- varno = appinfo->parent_relid;
- appinfo = root->append_rel_array[varno];
- }
- if (varno != onerel->relid)
- {
- /* Repeat access check on this rel */
- rte = planner_rt_fetch(varno, root);
- Assert(rte->rtekind == RTE_RELATION);
-
- userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
-
- vardata->acl_ok =
- rte->securityQuals == NIL &&
- (pg_class_aclcheck(rte->relid,
- userid,
- ACL_SELECT) == ACLCHECK_OK);
- }
- }
-
+ recheck_parent_acl(root, vardata, onerel->relid);
break;
}
@@ -5337,6 +5253,54 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
}
}
+/*
+ * If the user doesn't have permissions to access an inheritance child
+ * relation, check the permissions of the table actually mentioned in the
+ * query, since most likely the user does have that permission. Note that
+ * whole-table select privilege on the parent doesn't quite guarantee that the
+ * user could read all columns of the child. But in practice it's unlikely
+ * that any interesting security violation could result from allowing access to
+ * the expression stats, so we allow it anyway. See similar code in
+ * examine_simple_variable() for additional comments.
+ */
+static void
+recheck_parent_acl(PlannerInfo *root, VariableStatData *vardata, Oid relid)
+{
+ RangeTblEntry *rte;
+ Oid userid;
+
+ if (!vardata->acl_ok &&
+ root->append_rel_array != NULL)
+ {
+ AppendRelInfo *appinfo;
+ Index varno = relid;
+
+ appinfo = root->append_rel_array[varno];
+ while (appinfo &&
+ planner_rt_fetch(appinfo->parent_relid,
+ root)->rtekind == RTE_RELATION)
+ {
+ varno = appinfo->parent_relid;
+ appinfo = root->append_rel_array[varno];
+ }
+
+ if (varno != relid)
+ {
+ /* Repeat access check on this rel */
+ rte = planner_rt_fetch(varno, root);
+ Assert(rte->rtekind == RTE_RELATION);
+
+ userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+
+ vardata->acl_ok =
+ rte->securityQuals == NIL &&
+ (pg_class_aclcheck(rte->relid,
+ userid,
+ ACL_SELECT) == ACLCHECK_OK);
+ }
+ }
+}
+
/*
* examine_simple_variable
* Handle a simple Var for examine_variable
--
2.31.1
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: extended stats on partitioned tables
2022-01-15 05:11 Re: extended stats on partitioned tables Justin Pryzby <[email protected]>
2022-01-15 18:35 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
@ 2022-01-15 19:05 ` Tomas Vondra <[email protected]>
2022-01-16 23:44 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Vondra @ 2022-01-15 19:05 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Zhihong Yu <[email protected]>; Jaime Casanova <[email protected]>; pgsql-hackers
On 1/15/22 19:35, Tomas Vondra wrote:
> On 1/15/22 06:11, Justin Pryzby wrote:
>> On Mon, Dec 13, 2021 at 09:40:09PM +0100, Tomas Vondra wrote:
>>> 1) If the table is a separate relation (not part of an inheritance
>>> tree), this should make no difference. -> OK
>>>
>>> 2) If the table is using "old" inheritance, this reverts back to
>>> pre-regression behavior. So people will keep using the old statistics
>>> until the ANALYZE, and we need to tell them to ANALYZE or something.
>>>
>>> 3) If the table is using partitioning, it's guaranteed to be empty and
>>> there are no stats at all. Again, we should tell people to run ANALYZE.
>>
>> I think these can be mentioned in the commit message, which can end up
>> in the
>> minor release notes as a recommendation to rerun ANALYZE.
>>
>
> Good point. I pushed the 0002 part and added a short paragraph
> suggesting ANALYZE might be necessary. I did not go into details about
> the individual cases, because that'd be too much for a commit message.
>
>> Thanks for pushing 0001.
>>
>
> Thanks for posting the patches!
>
> I've pushed the second part - attached are the two remaining parts. I'll
> wait a bit before pushing the rebased 0001 (which goes into master
> branch only). Not sure about 0002 - I'm not convinced the refactored ACL
> checks are an improvement, but I'll think about it.
>
BTW when backpatching the first part, I had to decide what to do about
tests. The 10 & 11 branches did not have the check_estimated_rows()
function. I considered removing the tests, reworking the tests not to
need the function, or adding the function. Ultimately I added the
function, which seemed like the best option.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: extended stats on partitioned tables
2022-01-15 05:11 Re: extended stats on partitioned tables Justin Pryzby <[email protected]>
2022-01-15 18:35 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
2022-01-15 19:05 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
@ 2022-01-16 23:44 ` Tomas Vondra <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Tomas Vondra @ 2022-01-16 23:44 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Zhihong Yu <[email protected]>; Jaime Casanova <[email protected]>; pgsql-hackers
I've pushed the part adding stxdinherit flag to the catalog, so that on
master we build statistics both with and without data from the child
relations.
I'm not going to push the ACL refactoring. We have similar code on
various other places (not addressed by the proposed patch), and it'd
make backpatching harder. So I'm not sure it'd be an improvement.
In any case, I'm going to mark this as committed. Justin, if you think
we should reconsider the ACL refactoring, please submit it as a separate
patch. It seems mostly unrelated to the issue this thread was about.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-01-16 23:44 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 15:26 [PATCH 2/9] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]>
2022-01-15 05:11 Re: extended stats on partitioned tables Justin Pryzby <[email protected]>
2022-01-15 18:35 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
2022-01-15 19:05 ` Re: extended stats on partitioned tables Tomas Vondra <[email protected]>
2022-01-16 23:44 ` Re: extended stats on partitioned tables Tomas Vondra <[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