public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 3/8] Move IS [NOT] NULL handling from BRIN support functions 10+ messages / 4 participants [nested] [flat]
* [PATCH 3/8] Move IS [NOT] NULL handling from BRIN support functions @ 2021-03-02 18:27 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Tomas Vondra @ 2021-03-02 18:27 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 | 96 +------- src/backend/access/brin/brin_minmax.c | 93 +------ src/include/access/brin_internal.h | 3 + 4 files changed, 244 insertions(+), 241 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 743f2a925a..ce0f525c21 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; @@ -419,13 +399,18 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * consistent support procedure. We allocate space for all attributes, so * that we don't have to bother determining which attributes are used. * + * We keep null and regular keys separate, so that we can pass just the + * regular keys to the consistent function easily. + * * XXX The widest table can have ~1600 attributes, so this may allocate a * couple kilobytes of memory). We could invent a more compact approach * (with just space for used attributes) but that would make the matching * more complicated, so it may not be a win. */ 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. @@ -445,23 +430,23 @@ 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); @@ -469,9 +454,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 */ @@ -550,15 +549,58 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) Datum add; Oid collation; - /* skip attributes without any scan 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 @@ -665,7 +707,6 @@ brinbuildCallback(Relation index, { BrinBuildState *state = (BrinBuildState *) brstate; BlockNumber thisblock; - int i; thisblock = ItemPointerGetBlockNumber(tid); @@ -694,25 +735,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); } /* @@ -1491,6 +1515,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); @@ -1544,3 +1601,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 a260074c91..b17077703c 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); @@ -268,52 +258,9 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) int nkeys = PG_GETARG_INT32(3); Oid colloid = PG_GET_COLLATION(); int keyno; - bool has_regular_keys = false; - - /* Handle IS NULL/IS NOT NULL tests */ - for (keyno = 0; keyno < nkeys; keyno++) - { - ScanKey key = keys[keyno]; - Assert(key->sk_attno == column->bv_attno); - - /* Skip regular scan keys (and remember that we have some). */ - if ((!key->sk_flags & SK_ISNULL)) - { - has_regular_keys = true; - continue; - } - - 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); - } - - /* If there are no regular keys, the page range is considered consistent. */ - if (!has_regular_keys) - PG_RETURN_BOOL(true); + /* make sure we got some scan keys */ + Assert((nkeys > 0) && (keys != NULL)); /* * If is all nulls, it cannot possibly be consistent (at this point we @@ -331,9 +278,8 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) { ScanKey key = keys[keyno]; - /* Skip IS NULL/IS NOT NULL keys (already 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 criteria for @@ -574,37 +520,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 e116084a02..330bed0487 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); @@ -156,52 +146,9 @@ brin_minmax_consistent(PG_FUNCTION_ARGS) int nkeys = PG_GETARG_INT32(3); Oid colloid = PG_GET_COLLATION(); int keyno; - bool has_regular_keys = false; - - /* handle IS NULL/IS NOT NULL tests */ - for (keyno = 0; keyno < nkeys; keyno++) - { - ScanKey key = keys[keyno]; - - Assert(key->sk_attno == column->bv_attno); - - /* Skip regular scan keys (and remember that we have some). */ - if ((!key->sk_flags & SK_ISNULL)) - { - has_regular_keys = true; - continue; - } - 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); - } - - /* If there are no regular keys, the page range is considered consistent. */ - if (!has_regular_keys) - PG_RETURN_BOOL(true); + /* make sure we got some scan keys */ + Assert((nkeys > 0) && (keys != NULL)); /* * If is all nulls, it cannot possibly be consistent (at this point we @@ -215,9 +162,8 @@ brin_minmax_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 criteria for @@ -307,34 +253,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 --------------D524CD37E77B36A2883A7617 Content-Type: text/x-patch; charset=UTF-8; name="0004-Optimize-allocations-in-bringetbitmap-20210311.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0004-Optimize-allocations-in-bringetbitmap-20210311.patch" ^ permalink raw reply [nested|flat] 10+ messages in thread
* Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-25 07:54 Laurenz Albe <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Laurenz Albe @ 2023-09-25 07:54 UTC (permalink / raw) To: [email protected] In v16 and later, the following fails: CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); COPY boom FROM STDIN; ERROR: value too long for type character varying(5) In PostgreSQL v15 and earlier, the COPY statement succeeds. The error is thrown in BeginCopyFrom in line 1578 (HEAD) defexpr = expression_planner(defexpr); Bisecting shows that the regression was introduced by commit 9f8377f7a2, which introduced DEFAULT values for COPY FROM. The table definition is clearly silly, so I am not sure if that regression is worth fixing. On the other hand, it is not cool if something that worked without an error in v15 starts to fail later on. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-25 08:59 Laurenz Albe <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Laurenz Albe @ 2023-09-25 08:59 UTC (permalink / raw) To: [email protected] On Mon, 2023-09-25 at 09:54 +0200, Laurenz Albe wrote: > In v16 and later, the following fails: > > CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); > > COPY boom FROM STDIN; > ERROR: value too long for type character varying(5) > > In PostgreSQL v15 and earlier, the COPY statement succeeds. > > The error is thrown in BeginCopyFrom in line 1578 (HEAD) > > defexpr = expression_planner(defexpr); > > Bisecting shows that the regression was introduced by commit 9f8377f7a2, > which introduced DEFAULT values for COPY FROM. I suggest the attached fix, which evaluates default values only if the DEFAULT option was specified or if the column does not appear in the column list of COPY. Yours, Laurenz Albe Attachments: [text/x-patch] 0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.patch (1.7K, ../../[email protected]/2-0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.patch) download | inline diff: From 4af982c56df57a1a0f04340d394c297559fbabb5 Mon Sep 17 00:00:00 2001 From: Laurenz Albe <[email protected]> Date: Mon, 25 Sep 2023 10:56:15 +0200 Subject: [PATCH] Evaluate defaults in COPY FROM only if necessary Since commit 9f8377f7a2, we evaluate the column default values in COPY FROM for all columns except generated ones, because they could be needed if the input value matches the DEFAULT option. This can cause a surprising regression: CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); COPY boom FROM STDIN; ERROR: value too long for type character varying(5) This worked before 9f8377f7a2, since default values were only evaluated for columns that were not specified in the column list. To fix, fetch the default values only if the DEFAULT option was specified or for columns not specified in the column list. --- src/backend/commands/copyfrom.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 70871ed819..320b764aa9 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -1567,7 +1567,14 @@ BeginCopyFrom(ParseState *pstate, /* Get default info if available */ defexprs[attnum - 1] = NULL; - if (!att->attgenerated) + /* + * We need the default values only for columns that do not appear in the + * column list or if the DEFAULT option was given. We also don't need + * it for generated columns. + */ + if ((!list_member_int(cstate->attnumlist, attnum) || + cstate->opts.default_print != NULL) && + !att->attgenerated) { Expr *defexpr = (Expr *) build_column_default(cstate->rel, attnum); -- 2.41.0 ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-25 15:06 Andrew Dunstan <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Andrew Dunstan @ 2023-09-25 15:06 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; [email protected] On 2023-09-25 Mo 04:59, Laurenz Albe wrote: > On Mon, 2023-09-25 at 09:54 +0200, Laurenz Albe wrote: >> In v16 and later, the following fails: >> >> CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); >> >> COPY boom FROM STDIN; >> ERROR: value too long for type character varying(5) >> >> In PostgreSQL v15 and earlier, the COPY statement succeeds. >> >> The error is thrown in BeginCopyFrom in line 1578 (HEAD) >> >> defexpr = expression_planner(defexpr); >> >> Bisecting shows that the regression was introduced by commit 9f8377f7a2, >> which introduced DEFAULT values for COPY FROM. Oops :-( > I suggest the attached fix, which evaluates default values only if > the DEFAULT option was specified or if the column does not appear in > the column list of COPY. > Patch looks reasonable, haven't tested yet. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-25 20:52 Andrew Dunstan <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Andrew Dunstan @ 2023-09-25 20:52 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; [email protected] On 2023-09-25 Mo 11:06, Andrew Dunstan wrote: > > > On 2023-09-25 Mo 04:59, Laurenz Albe wrote: >> On Mon, 2023-09-25 at 09:54 +0200, Laurenz Albe wrote: >>> In v16 and later, the following fails: >>> >>> CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); >>> >>> COPY boom FROM STDIN; >>> ERROR: value too long for type character varying(5) >>> >>> In PostgreSQL v15 and earlier, the COPY statement succeeds. >>> >>> The error is thrown in BeginCopyFrom in line 1578 (HEAD) >>> >>> defexpr = expression_planner(defexpr); >>> >>> Bisecting shows that the regression was introduced by commit 9f8377f7a2, >>> which introduced DEFAULT values for COPY FROM. > > Thinking about this a little more, wouldn't it be better if we checked at the time we set the default that the value is actually valid for the given column? This is only one manifestation of a problem you could run into given this table definition. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-25 21:49 Tom Lane <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2023-09-25 21:49 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: Laurenz Albe <[email protected]>; [email protected] Andrew Dunstan <[email protected]> writes: > On 2023-09-25 Mo 11:06, Andrew Dunstan wrote: >> On 2023-09-25 Mo 04:59, Laurenz Albe wrote: >>> CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); > Thinking about this a little more, wouldn't it be better if we checked > at the time we set the default that the value is actually valid for the > given column? This is only one manifestation of a problem you could run > into given this table definition. I dunno, it seems at least possible that someone would do this deliberately as a means of preventing the column from being defaulted. In any case, the current behavior has stood for a very long time and no one has complained that an error should be thrown sooner. regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-26 06:48 Laurenz Albe <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Laurenz Albe @ 2023-09-26 06:48 UTC (permalink / raw) To: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; +Cc: [email protected] On Mon, 2023-09-25 at 17:49 -0400, Tom Lane wrote: > Andrew Dunstan <[email protected]> writes: > > On 2023-09-25 Mo 11:06, Andrew Dunstan wrote: > > > On 2023-09-25 Mo 04:59, Laurenz Albe wrote: > > > > CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); > > > Thinking about this a little more, wouldn't it be better if we checked > > at the time we set the default that the value is actually valid for the > > given column? This is only one manifestation of a problem you could run > > into given this table definition. > > I dunno, it seems at least possible that someone would do this > deliberately as a means of preventing the column from being defaulted. > In any case, the current behavior has stood for a very long time and > no one has complained that an error should be thrown sooner. Moreover, this makes restoring a pg_dump from v15 to v16 fail, which should never happen. This is how I got that bug report. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-09-26 08:11 Laurenz Albe <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Laurenz Albe @ 2023-09-26 08:11 UTC (permalink / raw) To: Tom Lane <[email protected]>; Andrew Dunstan <[email protected]>; +Cc: [email protected] Here is an improved version of the patch with regression tests. Yours, Laurenz Albe Attachments: [text/x-patch] 0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.V2.patch (3.5K, ../../[email protected]/2-0001-Evaluate-defaults-in-COPY-FROM-only-if-necessary.V2.patch) download | inline diff: From 71744ada1e2c8cfdbb57e03018572a1af623b09e Mon Sep 17 00:00:00 2001 From: Laurenz Albe <[email protected]> Date: Tue, 26 Sep 2023 10:09:49 +0200 Subject: [PATCH] Evaluate defaults in COPY FROM only if necessary Since commit 9f8377f7a2, we evaluate the column default values in COPY FROM for all columns except generated ones, because they could be needed if the input value matches the DEFAULT option. This can cause a surprising regression: CREATE TABLE boom (t character varying(5) DEFAULT 'a long string'); COPY boom FROM STDIN; ERROR: value too long for type character varying(5) This worked before 9f8377f7a2, since default values were only evaluated for columns that were not specified in the column list. To fix, fetch the default values only if the DEFAULT option was specified or for columns not specified in the column list. --- src/backend/commands/copyfrom.c | 9 ++++++++- src/test/regress/expected/copy.out | 17 +++++++++++++++++ src/test/regress/sql/copy.sql | 15 +++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 70871ed819..3f3e631dee 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -1567,7 +1567,14 @@ BeginCopyFrom(ParseState *pstate, /* Get default info if available */ defexprs[attnum - 1] = NULL; - if (!att->attgenerated) + /* + * We need the default values only for columns that do not appear in the + * column list. But if the DEFAULT option was given, we may need all + * column default values. We never need defaults for generated columns. + */ + if ((cstate->opts.default_print != NULL || + !list_member_int(cstate->attnumlist, attnum)) && + !att->attgenerated) { Expr *defexpr = (Expr *) build_column_default(cstate->rel, attnum); diff --git a/src/test/regress/expected/copy.out b/src/test/regress/expected/copy.out index 8a8bf43fde..a5912c13a8 100644 --- a/src/test/regress/expected/copy.out +++ b/src/test/regress/expected/copy.out @@ -240,3 +240,20 @@ SELECT * FROM header_copytest ORDER BY a; (5 rows) drop table header_copytest; +-- test COPY with overlong column defaults +create temp table oversized_column_default ( + col1 varchar(5) DEFAULT 'more than 5 chars', + col2 varchar(5)); +-- normal COPY should work +copy oversized_column_default from stdin; +-- error if the column is excluded +copy oversized_column_default (col2) from stdin; +ERROR: value too long for type character varying(5) +\. +invalid command \. +-- error if the DEFAULT option is given +copy oversized_column_default from stdin (default ''); +ERROR: value too long for type character varying(5) +\. +invalid command \. +drop table oversized_column_default; diff --git a/src/test/regress/sql/copy.sql b/src/test/regress/sql/copy.sql index f9da7b1508..7fdb26d14f 100644 --- a/src/test/regress/sql/copy.sql +++ b/src/test/regress/sql/copy.sql @@ -268,3 +268,18 @@ a c b SELECT * FROM header_copytest ORDER BY a; drop table header_copytest; + +-- test COPY with overlong column defaults +create temp table oversized_column_default ( + col1 varchar(5) DEFAULT 'more than 5 chars', + col2 varchar(5)); +-- normal COPY should work +copy oversized_column_default from stdin; +\. +-- error if the column is excluded +copy oversized_column_default (col2) from stdin; +\. +-- error if the DEFAULT option is given +copy oversized_column_default from stdin (default ''); +\. +drop table oversized_column_default; -- 2.41.0 ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-10-01 14:55 Andrew Dunstan <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Andrew Dunstan @ 2023-10-01 14:55 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; Tom Lane <[email protected]>; +Cc: [email protected] On 2023-09-26 Tu 04:11, Laurenz Albe wrote: > Here is an improved version of the patch with regression tests. > Thanks, pushed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Regression in COPY FROM caused by 9f8377f7a2 @ 2023-10-02 04:10 Laurenz Albe <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Laurenz Albe @ 2023-10-02 04:10 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; Tom Lane <[email protected]>; +Cc: [email protected] On Sun, 2023-10-01 at 10:55 -0400, Andrew Dunstan wrote: > Thanks, pushed. Thanks for taking care of that. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2023-10-02 04:10 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-02 18:27 [PATCH 3/8] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]> 2023-09-25 07:54 Regression in COPY FROM caused by 9f8377f7a2 Laurenz Albe <[email protected]> 2023-09-25 08:59 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Laurenz Albe <[email protected]> 2023-09-25 15:06 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Andrew Dunstan <[email protected]> 2023-09-25 20:52 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Andrew Dunstan <[email protected]> 2023-09-25 21:49 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Tom Lane <[email protected]> 2023-09-26 06:48 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Laurenz Albe <[email protected]> 2023-09-26 08:11 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Laurenz Albe <[email protected]> 2023-10-01 14:55 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Andrew Dunstan <[email protected]> 2023-10-02 04:10 ` Re: Regression in COPY FROM caused by 9f8377f7a2 Laurenz Albe <[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