public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/9] Move IS [NOT] NULL handling from BRIN support functions 3+ 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; 3+ 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] 3+ messages in thread
* Teach predtest about IS [NOT] <boolean> proofs @ 2023-12-11 19:59 James Coleman <[email protected]> 2023-12-13 18:36 ` Re: Teach predtest about IS [NOT] <boolean> proofs Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: James Coleman @ 2023-12-11 19:59 UTC (permalink / raw) To: pgsql-hackers Hello, I recently encountered a case where partial indexes were surprisingly not being used. The issue is that predtest doesn't understand how boolean values and IS <boolean> expressions relate. For example if I have: create table foo(i int, bar boolean); create index on foo(i) where bar is true; then this query: select * from foo where i = 1 and bar; doesn't use the partial index. Attached is a patch that solves that issue. It also teaches predtest about quite a few more cases involving BooleanTest expressions (e.g., how they relate to NullTest expressions). One thing I could imagine being an objection is that not all of these warrant cycles in planning. If that turns out to be the case there's not a particularly clear line in my mind about where to draw that line. As noted in a TODO in the patch itself, I think it may be worth refactoring the test_predtest module to run the "x, y" case as well as the "y, x" case with a single call so as to eliminate a lot of repetition in clause/expression test cases. If reviewers agree that's desirable, then I could do that as a precursor. Regards, James Coleman Attachments: [application/octet-stream] v1-0001-Teach-predtest-about-IS-NOT-bool-proofs.patch (26.1K, ../../CAAaqYe8Bo4bf_i6qKj8KBsmHMYXhe3Xt6vOe3OBQnOaf3_XBWg@mail.gmail.com/3-v1-0001-Teach-predtest-about-IS-NOT-bool-proofs.patch) download | inline diff: From 316c234154f0aefa98cf0730f66a381b1e07b95a Mon Sep 17 00:00:00 2001 From: jcoleman <[email protected]> Date: Thu, 21 Sep 2023 14:34:15 -0400 Subject: [PATCH v1] Teach predtest about IS [NOT] <bool> proofs This is particularly helpful for ensuring partial indexes are used. For example: ``` create table foo(i int, bar boolean); create index on foo(i) where bar is true; ``` this change allows the following query to use the index: ``` select * from foo where i = 1 and bar; ``` --- src/backend/optimizer/util/predtest.c | 266 ++++++++- .../test_predtest/expected/test_predtest.out | 539 +++++++++++++++++- .../test_predtest/sql/test_predtest.sql | 196 +++++++ 3 files changed, 990 insertions(+), 11 deletions(-) diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c index fe83e45311..afd0925cd9 100644 --- a/src/backend/optimizer/util/predtest.c +++ b/src/backend/optimizer/util/predtest.c @@ -1162,27 +1162,160 @@ predicate_implied_by_simple_clause(Expr *predicate, Node *clause, } } + /* Similarly check for boolean IS clauses */ + if (IsA(clause, BooleanTest)) + { + BooleanTest *test = (BooleanTest *) clause; + if (test->booltesttype == IS_TRUE) + { + /* X is true implies X */ + if (equal(predicate, test->arg)) + return true; + } + else if (test->booltesttype == IS_FALSE) + { + /* X is false implies NOT X */ + if (is_notclause(predicate) && + equal(get_notclausearg(predicate), test->arg)) + return true; + } + else if (test->booltesttype == IS_NOT_TRUE) + { + /* + * X is not true implies NOT X + * + * We can only prove weak implication here since null is not true + * evaluates to true rather than null. + */ + if (weak && is_notclause(predicate) && + equal(get_notclausearg(predicate), test->arg)) + return true; + } + else if (test->booltesttype == IS_NOT_FALSE) + { + /* + * X is not false implies X + * + * We can only prove weak implication here since null is not false + * evaluates to true rather than null. + */ + if (weak && equal(predicate, test->arg)) + return true; + } + } + + /* + * As well as boolean IS predicates + */ + if (IsA(predicate, BooleanTest)) + { + BooleanTest *test = (BooleanTest *) predicate; + if (test->booltesttype == IS_TRUE) + { + /* + * X implies X is true + * + * We can only prove strong implication here since `null is true` + * is false rather than null. + */ + if (!weak && equal(clause, test->arg)) + return true; + } + else if (test->booltesttype == IS_FALSE) + { + /* + * NOT X implies X is false + * + * We can only prove strong implication here since `not null` is + * null rather than true. + */ + if (!weak && is_notclause(clause) && + equal(get_notclausearg(clause), test->arg)) + return true; + } + else if (test->booltesttype == IS_NOT_TRUE) + { + /* NOT X implies X is not true */ + if (is_notclause(clause) && + equal(get_notclausearg(clause), test->arg)) + return true; + } + else if (test->booltesttype == IS_NOT_FALSE) + { + /* X implies X is not false*/ + if (equal(clause, test->arg)) + return true; + } + else if (test->booltesttype == IS_UNKNOWN) + { + if (IsA(clause, NullTest)) + { + NullTest *ntest = (NullTest *) clause; + + /* X IS NULL implies X is unknown */ + if (ntest->nulltesttype == IS_NULL && + equal(ntest->arg, test->arg)) + return true; + } + } + + if (test->booltesttype == IS_NOT_TRUE + || test->booltesttype == IS_NOT_FALSE) + { + + if (IsA(clause, BooleanTest)) + { + BooleanTest *testclause = (BooleanTest *) clause; + + /* + * X is unknown weakly implies X is not true + * X is unknown weakly implies X is not false + */ + if (weak && testclause->booltesttype == IS_UNKNOWN && + equal(testclause->arg, test->arg)) + return true; + } + } + + } + /* * We could likewise check whether the predicate is boolean equality to a * constant; but there are no known use-cases for that at the moment, * assuming that the predicate has been through constant-folding. */ - /* Next try the IS NOT NULL case */ - if (!weak && - predicate && IsA(predicate, NullTest)) + /* Next try the IS [NOT] NULL cases */ + if (IsA(predicate, NullTest)) { NullTest *ntest = (NullTest *) predicate; - /* row IS NOT NULL does not act in the simple way we have in mind */ - if (ntest->nulltesttype == IS_NOT_NULL && - !ntest->argisrow) + if (IsA(clause, BooleanTest)) { - /* strictness of clause for foo implies foo IS NOT NULL */ - if (clause_is_strict_for(clause, (Node *) ntest->arg, true)) - return true; + BooleanTest* btest = (BooleanTest *) clause; + + if ((ntest->nulltesttype == IS_NOT_NULL && + btest->booltesttype == IS_NOT_UNKNOWN) || + (ntest->nulltesttype == IS_NULL && + btest->booltesttype == IS_UNKNOWN)) + { + if (equal(ntest->arg, btest->arg)) + return true; + } + } + + if (!weak) + { + /* row IS NOT NULL does not act in the simple way we have in mind */ + if (ntest->nulltesttype == IS_NOT_NULL && + !ntest->argisrow) + { + /* strictness of clause for foo implies foo IS NOT NULL */ + if (clause_is_strict_for(clause, (Node *) ntest->arg, true)) + return true; + } + return false; /* we can't succeed below... */ } - return false; /* we can't succeed below... */ } /* Else try operator-related knowledge */ @@ -1272,6 +1405,18 @@ predicate_refuted_by_simple_clause(Expr *predicate, Node *clause, equal(((NullTest *) predicate)->arg, isnullarg)) return true; + if (IsA(predicate, BooleanTest)) + { + BooleanTest *testpredicate = (BooleanTest *) predicate; + + /* foo IS NULL refutes foo IS FALSE */ + /* foo IS NULL refutes foo IS TRUE*/ + if ((testpredicate->booltesttype == IS_FALSE || + testpredicate->booltesttype == IS_TRUE) && + equal(testpredicate->arg, isnullarg)) + return true; + } + /* foo IS NULL weakly refutes any predicate that is strict for foo */ if (weak && clause_is_strict_for((Node *) predicate, (Node *) isnullarg, true)) @@ -1280,6 +1425,98 @@ predicate_refuted_by_simple_clause(Expr *predicate, Node *clause, return false; /* we can't succeed below... */ } + /* Try the clause-IS-NOT-NULL case */ + if (clause && IsA(clause, NullTest) && + ((NullTest *) clause)->nulltesttype == IS_NOT_NULL) + { + Expr *isnotnullarg = ((NullTest *) clause)->arg; + + if (IsA(predicate, BooleanTest)) + { + BooleanTest *testpredicate = (BooleanTest *) predicate; + + /* foo IS NOT NULL refutes foo IS UNKNOWN */ + if (testpredicate->booltesttype == IS_UNKNOWN && + equal(testpredicate->arg, isnotnullarg)) + return true; + } + } + + if (IsA(predicate, BooleanTest)) + { + BooleanTest *test = (BooleanTest *) predicate; + + if (test->booltesttype == IS_UNKNOWN) + { + /* + * foo IS NOT UNKNOWN refutes foo IS UNKNOWN is covered by the + * clause strictness check below + */ + + /* strictness of clause for foo refutes foo IS UNKNOWN */ + if (clause_is_strict_for(clause, (Node *) test->arg, true)) + return true; + } + + if (test->booltesttype == IS_TRUE) + { + if (IsA(clause, BooleanTest)) + { + BooleanTest *testclause = (BooleanTest *) clause; + + /* foo IS NOT TRUE refutes foo IS TRUE */ + /* foo IS UNKNOWN refutes foo IS TRUE */ + if ((testclause->booltesttype == IS_NOT_TRUE || + testclause->booltesttype == IS_UNKNOWN) && + equal(test->arg, testclause->arg)) + return true; + } + } + + } + + if (IsA(clause, BooleanTest)) + { + BooleanTest *test = (BooleanTest *) clause; + + if (test->booltesttype == IS_UNKNOWN) + { + if (IsA(predicate, BooleanTest)) + { + BooleanTest *testpredicate= (BooleanTest *) predicate; + + /* foo IS UNKNOWN refutes foo IS TRUE */ + /* foo IS UNKNOWN refutes foo IS NOT UNKNOWN */ + if ((testpredicate->booltesttype == IS_FALSE || + testpredicate->booltesttype == IS_NOT_UNKNOWN) && + equal(testpredicate->arg, test->arg)) + return true; + } + + /* foo IS UNKNOWN weakly refutes any predicate that is strict for foo */ + if (weak && + clause_is_strict_for((Node *) predicate, (Node *) test->arg, true)) + return true; + } + + if (test->booltesttype == IS_TRUE) + { + /* foo IS TRUE refutes NOT foo */ + if (is_notclause(predicate) && + equal(get_notclausearg(predicate), test->arg)) + return true; + } + else if (test->booltesttype == IS_NOT_TRUE) + { + /* foo IS NOT TRUE weakly refutes foo */ + if (weak && equal(predicate, test->arg)) + return true; + } + else if (test->booltesttype == IS_FALSE) + { + } + } + /* Else try operator-related knowledge */ return operator_predicate_proof(predicate, clause, true, weak); } @@ -1500,6 +1737,15 @@ clause_is_strict_for(Node *clause, Node *subexpr, bool allow_false) return clause_is_strict_for(arraynode, subexpr, false); } + if (IsA(clause, BooleanTest)) + { + BooleanTest *test = (BooleanTest *) clause; + + if (test->booltesttype == IS_TRUE || test->booltesttype == IS_FALSE || + test->booltesttype == IS_NOT_UNKNOWN) + return true; + } + /* * When recursing into an expression, we might find a NULL constant. * That's certainly NULL, whether it matches subexpr or not. diff --git a/src/test/modules/test_predtest/expected/test_predtest.out b/src/test/modules/test_predtest/expected/test_predtest.out index 6d21bcd603..ea1272e2cc 100644 --- a/src/test/modules/test_predtest/expected/test_predtest.out +++ b/src/test/modules/test_predtest/expected/test_predtest.out @@ -143,12 +143,194 @@ $$); strong_implied_by | f weak_implied_by | f strong_refuted_by | f +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | f +w_r_holds | t + +select * from test_predtest($$ +select x is not true, not x +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select not x, x is not true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | f +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is not true, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is unknown, x is not true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | f weak_refuted_by | f s_i_holds | f w_i_holds | f s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is true, x +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | f +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select not x, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is true, not x +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is not true, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is false, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is unknown, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t w_r_holds | t +select * from test_predtest($$ +select x is false, not x +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | f +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select not x, x is false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + select * from test_predtest($$ select x is false, x from booleans @@ -177,6 +359,62 @@ w_i_holds | f s_r_holds | t w_r_holds | t +select * from test_predtest($$ +select x is not false, x +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x, x is not false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | f +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is not false, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is unknown, x is not false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | f +w_i_holds | f +s_r_holds | f +w_r_holds | f + select * from test_predtest($$ select x is unknown, x from booleans @@ -199,12 +437,96 @@ $$); strong_implied_by | f weak_implied_by | f strong_refuted_by | f -weak_refuted_by | f +weak_refuted_by | t s_i_holds | f w_i_holds | t s_r_holds | f w_r_holds | t +select * from test_predtest($$ +select x is not unknown, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is unknown, x is not unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is null, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is not null, x is not unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is not null, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is not null, x is false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + -- Assorted not-so-trivial refutation rules select * from test_predtest($$ select x is null, x @@ -234,6 +556,34 @@ w_i_holds | t s_r_holds | f w_r_holds | t +select * from test_predtest($$ +select x is null, x is not unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is not unknown, x is null +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + select * from test_predtest($$ select strictf(x,y), x is null from booleans @@ -1094,3 +1444,190 @@ w_i_holds | t s_r_holds | f w_r_holds | t +-- More BooleanTest proofs +-- TODO: some of these are duplicative, but it's easier to see them all in one +-- spot. I'm wondering if we should standardize a location for all of them, +-- and potentially updated test_predtest to run both directions rather than +-- needing to duplicate queries (with the two clauses swapped). +select * from test_predtest($$ +select x, x is false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | t +s_i_holds | f +w_i_holds | t +s_r_holds | f +w_r_holds | t + +select * from test_predtest($$ +select not x, x is true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x, x is not true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | f +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | f +w_r_holds | t + +select * from test_predtest($$ +select x is true, x is not true +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is true, x is false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is true, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is true, x is null +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is false, x is unknown +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is false, x is null +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is unknown, x is false +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + +select * from test_predtest($$ +select x is unknown, x is null +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | t +weak_implied_by | t +strong_refuted_by | f +weak_refuted_by | f +s_i_holds | t +w_i_holds | t +s_r_holds | f +w_r_holds | f + +select * from test_predtest($$ +select x is unknown, x is not null +from booleans +$$); +-[ RECORD 1 ]-----+-- +strong_implied_by | f +weak_implied_by | f +strong_refuted_by | t +weak_refuted_by | t +s_i_holds | f +w_i_holds | f +s_r_holds | t +w_r_holds | t + diff --git a/src/test/modules/test_predtest/sql/test_predtest.sql b/src/test/modules/test_predtest/sql/test_predtest.sql index 072eb5b0d5..d2eb4bdb5f 100644 --- a/src/test/modules/test_predtest/sql/test_predtest.sql +++ b/src/test/modules/test_predtest/sql/test_predtest.sql @@ -76,6 +76,71 @@ select x, x is not true from booleans $$); +select * from test_predtest($$ +select x is not true, not x +from booleans +$$); + +select * from test_predtest($$ +select not x, x is not true +from booleans +$$); + +select * from test_predtest($$ +select x is not true, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is not true +from booleans +$$); + +select * from test_predtest($$ +select x is true, x +from booleans +$$); + +select * from test_predtest($$ +select x, x is true +from booleans +$$); + +select * from test_predtest($$ +select not x, x is true +from booleans +$$); + +select * from test_predtest($$ +select x is true, not x +from booleans +$$); + +select * from test_predtest($$ +select x is not true, x is true +from booleans +$$); + +select * from test_predtest($$ +select x is false, x is true +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is true +from booleans +$$); + +select * from test_predtest($$ +select x is false, not x +from booleans +$$); + +select * from test_predtest($$ +select not x, x is false +from booleans +$$); + select * from test_predtest($$ select x is false, x from booleans @@ -86,6 +151,26 @@ select x, x is false from booleans $$); +select * from test_predtest($$ +select x is not false, x +from booleans +$$); + +select * from test_predtest($$ +select x, x is not false +from booleans +$$); + +select * from test_predtest($$ +select x is not false, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is not false +from booleans +$$); + select * from test_predtest($$ select x is unknown, x from booleans @@ -96,6 +181,36 @@ select x, x is unknown from booleans $$); +select * from test_predtest($$ +select x is not unknown, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is not unknown +from booleans +$$); + +select * from test_predtest($$ +select x is null, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select x is not null, x is not unknown +from booleans +$$); + +select * from test_predtest($$ +select x is not null, x is true +from booleans +$$); + +select * from test_predtest($$ +select x is not null, x is false +from booleans +$$); + -- Assorted not-so-trivial refutation rules select * from test_predtest($$ @@ -108,6 +223,16 @@ select x, x is null from booleans $$); +select * from test_predtest($$ +select x is null, x is not unknown +from booleans +$$); + +select * from test_predtest($$ +select x is not unknown, x is null +from booleans +$$); + select * from test_predtest($$ select strictf(x,y), x is null from booleans @@ -440,3 +565,74 @@ select * from test_predtest($$ select x = all(opaque_array(array[1])), x is null from integers $$); + +-- More BooleanTest proofs +-- TODO: some of these are duplicative, but it's easier to see them all in one +-- spot. I'm wondering if we should standardize a location for all of them, +-- and potentially updated test_predtest to run both directions rather than +-- needing to duplicate queries (with the two clauses swapped). + +select * from test_predtest($$ +select x, x is false +from booleans +$$); + +select * from test_predtest($$ +select x, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select not x, x is true +from booleans +$$); + +select * from test_predtest($$ +select x, x is not true +from booleans +$$); + +select * from test_predtest($$ +select x is true, x is not true +from booleans +$$); + +select * from test_predtest($$ +select x is true, x is false +from booleans +$$); + +select * from test_predtest($$ +select x is true, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select x is true, x is null +from booleans +$$); + +select * from test_predtest($$ +select x is false, x is unknown +from booleans +$$); + +select * from test_predtest($$ +select x is false, x is null +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is false +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is null +from booleans +$$); + +select * from test_predtest($$ +select x is unknown, x is not null +from booleans +$$); -- 2.39.3 (Apple Git-145) ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Teach predtest about IS [NOT] <boolean> proofs 2023-12-11 19:59 Teach predtest about IS [NOT] <boolean> proofs James Coleman <[email protected]> @ 2023-12-13 18:36 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tom Lane @ 2023-12-13 18:36 UTC (permalink / raw) To: James Coleman <[email protected]>; +Cc: pgsql-hackers James Coleman <[email protected]> writes: > Attached is a patch that solves that issue. It also teaches predtest about > quite a few more cases involving BooleanTest expressions (e.g., how they > relate to NullTest expressions). One thing I could imagine being an > objection is that not all of these warrant cycles in planning. If that > turns out to be the case there's not a particularly clear line in my mind > about where to draw that line. I don't have an objection in principle to adding more smarts to predtest.c. However, we should be wary of slowing down cases where no BooleanTests are present to be optimized. I wonder if it could help to use a switch on nodeTag rather than a series of if(IsA()) tests. (I'd be inclined to rewrite the inner if-then-else chains as switches too, really. You get some benefit from the compiler noticing whether you've covered all the enum values.) I note you've actively broken the function's ability to cope with NULL input pointers. Maybe we don't need it to, but I'm not going to accept a patch that just side-swipes that case without any justification. Another way in which the patch needs more effort is that you've not bothered to update the large comment block atop the function. Perhaps, rather than hoping people will notice comments that are potentially offscreen from what they're modifying, we should relocate those comment paras to be adjacent to the relevant parts of the function? I've not gone through the patch in detail to see whether I believe the proposed proof rules. It would help to have more comments justifying them. > As noted in a TODO in the patch itself, I think it may be worth refactoring > the test_predtest module to run the "x, y" case as well as the "y, x" case > with a single call so as to eliminate a lot of repetition in > clause/expression test cases. If reviewers agree that's desirable, then I > could do that as a precursor. I think that's actively undesirable. It is not typically the case that a proof rule for A => B also works in the other direction, so this would encourage wasting cycles in the tests. I fear it might also cause confusion about which direction a proof rule is supposed to work in. regards, tom lane ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-12-13 18:36 UTC | newest] Thread overview: 3+ 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]> 2023-12-11 19:59 Teach predtest about IS [NOT] <boolean> proofs James Coleman <[email protected]> 2023-12-13 18:36 ` Re: Teach predtest about IS [NOT] <boolean> proofs Tom Lane <[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