public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/6] Move IS [NOT] NULL handling from BRIN support functions 3+ messages / 3 participants [nested] [flat]
* [PATCH 2/6] 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 8a1a9da78f..55851376d8 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 @@ -178,7 +181,6 @@ brininsert(Relation idxRel, Datum *values, bool *nulls, OffsetNumber off; BrinTuple *brtup; BrinMemTuple *dtup; - int keyno; CHECK_FOR_INTERRUPTS(); @@ -242,31 +244,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) { @@ -389,8 +367,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; @@ -415,10 +395,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. @@ -439,23 +422,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); @@ -463,9 +447,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 */ @@ -543,15 +541,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 @@ -694,7 +734,6 @@ brinbuildCallback(Relation index, { BrinBuildState *state = (BrinBuildState *) brstate; BlockNumber thisblock; - int i; thisblock = ItemPointerGetBlockNumber(tid); @@ -723,25 +762,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); } /* @@ -1520,6 +1542,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); @@ -1573,3 +1628,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 85c612e490..7b79a52536 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 --------------D21F32E0F9E7408E9BE1AF2E Content-Type: text/x-patch; charset=UTF-8; name="0003-Optimize-allocations-in-bringetbitmap-20210114.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0003-Optimize-allocations-in-bringetbitmap-20210114.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add macros for ReorderBufferTXN toptxn @ 2023-03-14 23:54 Peter Smith <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Peter Smith @ 2023-03-14 23:54 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Mar 14, 2023 at 10:43 PM Amit Kapila <[email protected]> wrote: > > On Tue, Mar 14, 2023 at 12:37 PM Peter Smith <[email protected]> wrote: > > > > Thanks for the review! > > > > On Mon, Mar 13, 2023 at 6:19 PM vignesh C <[email protected]> wrote: > > ... > > > > > 4) We check if txn->toptxn is not null twice here both in if condition > > > and in the assignment, we could retain the assignment operation as > > > earlier to remove the 2nd check: > > > - if (txn->toptxn) > > > - txn = txn->toptxn; > > > + if (isa_subtxn(txn)) > > > + txn = get_toptxn(txn); > > > > > > We could avoid one check again by: > > > + if (isa_subtxn(txn)) > > > + txn = txn->toptxn; > > > > > > > Yeah, that is true, but I chose not to keep the original assignment in > > this case mainly because then it is consistent with the other changed > > code --- e.g. Every other direct member assignment/access of the > > 'toptxn' member now hides behind the macros so I did not want this > > single place to be the odd one out. TBH, I don't think 1 extra check > > is of any significance, but it is not a problem to change like you > > suggested if other people also want it done that way. > > > > Can't we directly use rbtxn_get_toptxn() for this case? I think that > way code will look neat. I see that it is not exactly matching the > existing check so you might be worried but I feel the new code will > achieve the same purpose and will be easy to follow. > OK. Done as suggested. PSA v3. ------ Kind Regards, Peter Smith. Fujitsu Australia Attachments: [application/octet-stream] v3-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch (6.4K, ../../CAHut+PtrD4xU4OPUB64ZK+DPDhfKn3zph=nDpEWUFFzUvMKo2w@mail.gmail.com/2-v3-0001-Add-macros-for-ReorderBufferTXN-toptxn.patch) download | inline diff: From 9b660e136afa6f820af3c3d27841ece4e021121e Mon Sep 17 00:00:00 2001 From: Peter Smith <[email protected]> Date: Wed, 15 Mar 2023 09:37:20 +1100 Subject: [PATCH v3] Add macros for ReorderBufferTXN toptxn. Make toptxn related code more readable by introducing some simple macros. --- contrib/test_decoding/test_decoding.c | 4 +-- src/backend/replication/logical/reorderbuffer.c | 40 ++++++++++--------------- src/backend/replication/pgoutput/pgoutput.c | 6 ++-- src/include/replication/reorderbuffer.h | 18 +++++++++++ 4 files changed, 38 insertions(+), 30 deletions(-) diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c index b7e6048..628c6a2 100644 --- a/contrib/test_decoding/test_decoding.c +++ b/contrib/test_decoding/test_decoding.c @@ -815,11 +815,11 @@ pg_decode_stream_abort(LogicalDecodingContext *ctx, * maintain the output_plugin_private only under the toptxn so if this is * not the toptxn then fetch the toptxn. */ - ReorderBufferTXN *toptxn = txn->toptxn ? txn->toptxn : txn; + ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn); TestDecodingTxnData *txndata = toptxn->output_plugin_private; bool xact_wrote_changes = txndata->xact_wrote_changes; - if (txn->toptxn == NULL) + if (rbtxn_is_toptxn(txn)) { Assert(txn->output_plugin_private != NULL); pfree(txndata); diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 2d17c55..0f063b0 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -717,10 +717,7 @@ ReorderBufferProcessPartialChange(ReorderBuffer *rb, ReorderBufferTXN *txn, return; /* Get the top transaction. */ - if (txn->toptxn != NULL) - toptxn = txn->toptxn; - else - toptxn = txn; + toptxn = rbtxn_get_toptxn(txn); /* * Indicate a partial change for toast inserts. The change will be @@ -809,13 +806,7 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn, change->action == REORDER_BUFFER_CHANGE_TRUNCATE || change->action == REORDER_BUFFER_CHANGE_MESSAGE) { - ReorderBufferTXN *toptxn; - - /* get the top transaction */ - if (txn->toptxn != NULL) - toptxn = txn->toptxn; - else - toptxn = txn; + ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn); toptxn->txn_flags |= RBTXN_HAS_STREAMABLE_CHANGE; } @@ -1667,7 +1658,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep * about the toplevel xact (we send the XID in all messages), but we never * stream XIDs of empty subxacts. */ - if ((!txn_prepared) && ((!txn->toptxn) || (txn->nentries_mem != 0))) + if ((!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0))) txn->txn_flags |= RBTXN_IS_STREAMED; if (txn_prepared) @@ -3207,10 +3198,7 @@ ReorderBufferChangeMemoryUpdate(ReorderBuffer *rb, * Update the total size in top level as well. This is later used to * compute the decoding stats. */ - if (txn->toptxn != NULL) - toptxn = txn->toptxn; - else - toptxn = txn; + toptxn = rbtxn_get_toptxn(txn); if (addition) { @@ -3295,8 +3283,7 @@ ReorderBufferAddInvalidations(ReorderBuffer *rb, TransactionId xid, * so that we can execute them all together. See comments atop this * function. */ - if (txn->toptxn) - txn = txn->toptxn; + txn = rbtxn_get_toptxn(txn); Assert(nmsgs > 0); @@ -3354,7 +3341,6 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn) { ReorderBufferTXN *txn; - ReorderBufferTXN *toptxn; txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true); @@ -3370,11 +3356,15 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer *rb, TransactionId xid, * conveniently check just top-level transaction and decide whether to * build the hash table or not. */ - toptxn = txn->toptxn; - if (toptxn != NULL && !rbtxn_has_catalog_changes(toptxn)) + if (rbtxn_is_subtxn(txn)) { - toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES; - dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node); + ReorderBufferTXN *toptxn = rbtxn_get_toptxn(txn); + + if (!rbtxn_has_catalog_changes(toptxn)) + { + toptxn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES; + dclist_push_tail(&rb->catchange_txns, &toptxn->catchange_node); + } } } @@ -3619,7 +3609,7 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb) (txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL) { /* we know there has to be one, because the size is not zero */ - Assert(txn && !txn->toptxn); + Assert(txn && rbtxn_is_toptxn(txn)); Assert(txn->total_size > 0); Assert(rb->size >= txn->total_size); @@ -4007,7 +3997,7 @@ ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn) bool txn_is_streamed; /* We can never reach here for a subtransaction. */ - Assert(txn->toptxn == NULL); + Assert(rbtxn_is_toptxn(txn)); /* * We can't make any assumptions about base snapshot here, similar to what diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 00a2d73..3a2d2e3 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -694,8 +694,8 @@ maybe_send_schema(LogicalDecodingContext *ctx, if (in_streaming) xid = change->txn->xid; - if (change->txn->toptxn) - topxid = change->txn->toptxn->xid; + if (rbtxn_is_subtxn(change->txn)) + topxid = rbtxn_get_toptxn(change->txn)->xid; else topxid = xid; @@ -1879,7 +1879,7 @@ pgoutput_stream_abort(struct LogicalDecodingContext *ctx, Assert(!in_streaming); /* determine the toplevel transaction */ - toptxn = (txn->toptxn) ? txn->toptxn : txn; + toptxn = rbtxn_get_toptxn(txn); Assert(rbtxn_is_streamed(toptxn)); diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h index 215d149..08dc9ee 100644 --- a/src/include/replication/reorderbuffer.h +++ b/src/include/replication/reorderbuffer.h @@ -249,6 +249,24 @@ typedef struct ReorderBufferChange ((txn)->txn_flags & RBTXN_SKIPPED_PREPARE) != 0 \ ) +/* Is this a top-level transaction? */ +#define rbtxn_is_toptxn(txn)\ +(\ + (txn)->toptxn == NULL\ +) + +/* Is this a subtransaction? */ +#define rbtxn_is_subtxn(txn)\ +(\ + (txn)->toptxn != NULL\ +) + +/* Get the top-level transaction of this (sub)transaction. */ +#define rbtxn_get_toptxn(txn)\ +(\ + rbtxn_is_subtxn(txn) ? (txn)->toptxn : (txn)\ +) + typedef struct ReorderBufferTXN { /* See above */ -- 1.8.3.1 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add macros for ReorderBufferTXN toptxn @ 2023-03-15 05:54 Masahiko Sawada <[email protected]> parent: Peter Smith <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Masahiko Sawada @ 2023-03-15 05:54 UTC (permalink / raw) To: Peter Smith <[email protected]>; +Cc: Amit Kapila <[email protected]>; vignesh C <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On Wed, Mar 15, 2023 at 8:55 AM Peter Smith <[email protected]> wrote: > > On Tue, Mar 14, 2023 at 10:43 PM Amit Kapila <[email protected]> wrote: > > > > On Tue, Mar 14, 2023 at 12:37 PM Peter Smith <[email protected]> wrote: > > > > > > Thanks for the review! > > > > > > On Mon, Mar 13, 2023 at 6:19 PM vignesh C <[email protected]> wrote: > > > ... > > > > > > > 4) We check if txn->toptxn is not null twice here both in if condition > > > > and in the assignment, we could retain the assignment operation as > > > > earlier to remove the 2nd check: > > > > - if (txn->toptxn) > > > > - txn = txn->toptxn; > > > > + if (isa_subtxn(txn)) > > > > + txn = get_toptxn(txn); > > > > > > > > We could avoid one check again by: > > > > + if (isa_subtxn(txn)) > > > > + txn = txn->toptxn; > > > > > > > > > > Yeah, that is true, but I chose not to keep the original assignment in > > > this case mainly because then it is consistent with the other changed > > > code --- e.g. Every other direct member assignment/access of the > > > 'toptxn' member now hides behind the macros so I did not want this > > > single place to be the odd one out. TBH, I don't think 1 extra check > > > is of any significance, but it is not a problem to change like you > > > suggested if other people also want it done that way. > > > > > > > Can't we directly use rbtxn_get_toptxn() for this case? I think that > > way code will look neat. I see that it is not exactly matching the > > existing check so you might be worried but I feel the new code will > > achieve the same purpose and will be easy to follow. > > > > OK. Done as suggested. > +1 to the idea. Here are some minor comments: @@ -1667,7 +1658,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep * about the toplevel xact (we send the XID in all messages), but we never * stream XIDs of empty subxacts. */ - if ((!txn_prepared) && ((!txn->toptxn) || (txn->nentries_mem != 0))) + if ((!txn_prepared) && (rbtxn_is_toptxn(txn) || (txn->nentries_mem != 0))) txn->txn_flags |= RBTXN_IS_STREAMED; Probably the following comment of the above lines also needs to be updated? * The toplevel transaction, identified by (toptxn==NULL), is marked as * streamed always, --- +/* Is this a top-level transaction? */ +#define rbtxn_is_toptxn(txn)\ +(\ + (txn)->toptxn == NULL\ +) + +/* Is this a subtransaction? */ +#define rbtxn_is_subtxn(txn)\ +(\ + (txn)->toptxn != NULL\ +) + +/* Get the top-level transaction of this (sub)transaction. */ +#define rbtxn_get_toptxn(txn)\ +(\ + rbtxn_is_subtxn(txn) ? (txn)->toptxn : (txn)\ +) We need a whitespace before backslashes. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-03-15 05:54 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/6] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]> 2023-03-14 23:54 Re: Add macros for ReorderBufferTXN toptxn Peter Smith <[email protected]> 2023-03-15 05:54 ` Re: Add macros for ReorderBufferTXN toptxn Masahiko Sawada <[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