public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/8] Move IS [NOT] NULL handling from BRIN support functions 14+ messages / 6 participants [nested] [flat]
* [PATCH 4/8] Move IS [NOT] NULL handling from BRIN support functions @ 2021-03-02 18:27 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 14+ 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 f9a0476024..9f2656b8d9 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 */ @@ -549,15 +548,58 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) BrinValues *bval; Datum add; - /* 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 @@ -703,7 +745,6 @@ brinbuildCallback(Relation index, { BrinBuildState *state = (BrinBuildState *) brstate; BlockNumber thisblock; - int i; thisblock = ItemPointerGetBlockNumber(tid); @@ -732,25 +773,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); } /* @@ -1529,6 +1553,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); @@ -1582,3 +1639,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 --------------82D677E9F94AC7574E460205 Content-Type: text/x-patch; charset=UTF-8; name="0005-Optimize-allocations-in-bringetbitmap-20210308.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0005-Optimize-allocations-in-bringetbitmap-20210308.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 18:23 Tom Lane <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Tom Lane @ 2023-12-08 18:23 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > On 2023-12-08 10:05:09 -0500, Tom Lane wrote: >> ... there was already opinion upthread that this should be on by >> default, which I agree with. You shouldn't be hitting cases like >> this commonly (if so, they're bugs to fix or the errcode should be >> rethought), and the failure might be pretty hard to reproduce. > FWIW, I did some analysis on aggregated logs on a larger number of machines, > and it does look like that'd be a measurable increase in log volume. There are > a few voluminous internal errors in core, but the bigger issue is > extensions. They are typically much less disciplined about assigning error > codes than core PG is. Well, I don't see much wrong with making a push to assign error codes to more calls. We've had other discussions about doing that. Certainly these SSL failures are not "internal" errors. > could not accept SSL connection: %m - with zero errno > ... > I'm a bit confused about the huge number of "could not accept SSL connection: > %m" with a zero errno. I guess we must be clearing errno somehow, but I don't > immediately see where. Or perhaps we need to actually look at what > SSL_get_error() returns? Hmm, don't suppose you have a way to reproduce that? regards, tom lane ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 18:34 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 14+ messages in thread From: Andres Freund @ 2023-12-08 18:34 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2023-12-08 13:23:50 -0500, Tom Lane wrote: > Andres Freund <[email protected]> writes: > > On 2023-12-08 10:05:09 -0500, Tom Lane wrote: > >> ... there was already opinion upthread that this should be on by > >> default, which I agree with. You shouldn't be hitting cases like > >> this commonly (if so, they're bugs to fix or the errcode should be > >> rethought), and the failure might be pretty hard to reproduce. > > > FWIW, I did some analysis on aggregated logs on a larger number of machines, > > and it does look like that'd be a measurable increase in log volume. There are > > a few voluminous internal errors in core, but the bigger issue is > > extensions. They are typically much less disciplined about assigning error > > codes than core PG is. > > Well, I don't see much wrong with making a push to assign error codes > to more calls. Oh, very much agreed. But I suspect we won't quickly do the same for out-of-core extensions... > Certainly these SSL failures are not "internal" errors. > > > could not accept SSL connection: %m - with zero errno > > ... > > I'm a bit confused about the huge number of "could not accept SSL connection: > > %m" with a zero errno. I guess we must be clearing errno somehow, but I don't > > immediately see where. Or perhaps we need to actually look at what > > SSL_get_error() returns? > > Hmm, don't suppose you have a way to reproduce that? After a bit of trying, yes. I put an abort() into pgtls_open_client(), after initialize_SSL(). Connecting does result in: LOG: could not accept SSL connection: Success Greetings, Andres Freund ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 18:46 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Tom Lane @ 2023-12-08 18:46 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > On 2023-12-08 13:23:50 -0500, Tom Lane wrote: >> Hmm, don't suppose you have a way to reproduce that? > After a bit of trying, yes. I put an abort() into pgtls_open_client(), after > initialize_SSL(). Connecting does result in: > LOG: could not accept SSL connection: Success OK. I can dig into that, unless you're already on it? regards, tom lane ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 18:51 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 14+ messages in thread From: Andres Freund @ 2023-12-08 18:51 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2023-12-08 13:46:07 -0500, Tom Lane wrote: > Andres Freund <[email protected]> writes: > > On 2023-12-08 13:23:50 -0500, Tom Lane wrote: > >> Hmm, don't suppose you have a way to reproduce that? > > > After a bit of trying, yes. I put an abort() into pgtls_open_client(), after > > initialize_SSL(). Connecting does result in: > > LOG: could not accept SSL connection: Success > > OK. I can dig into that, unless you're already on it? I think I figured it it out. Looks like we need to translate a closed socket (recvfrom() returning 0) to ECONNRESET or such. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 19:33 Andres Freund <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 2 replies; 14+ messages in thread From: Andres Freund @ 2023-12-08 19:33 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2023-12-08 10:51:01 -0800, Andres Freund wrote: > On 2023-12-08 13:46:07 -0500, Tom Lane wrote: > > Andres Freund <[email protected]> writes: > > > On 2023-12-08 13:23:50 -0500, Tom Lane wrote: > > >> Hmm, don't suppose you have a way to reproduce that? > > > > > After a bit of trying, yes. I put an abort() into pgtls_open_client(), after > > > initialize_SSL(). Connecting does result in: > > > LOG: could not accept SSL connection: Success > > > > OK. I can dig into that, unless you're already on it? > > I think I figured it it out. Looks like we need to translate a closed socket > (recvfrom() returning 0) to ECONNRESET or such. I think we might just need to expand the existing branch for EOF: if (r < 0) ereport(COMMERROR, (errcode_for_socket_access(), errmsg("could not accept SSL connection: %m"))); else ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("could not accept SSL connection: EOF detected"))); The openssl docs say: The following return values can occur: 0 The TLS/SSL handshake was not successful but was shut down controlled and by the specifications of the TLS/SSL protocol. Call SSL_get_error() with the return value ret to find out the reason. 1 The TLS/SSL handshake was successfully completed, a TLS/SSL connection has been established. <0 The TLS/SSL handshake was not successful because a fatal error occurred either at the protocol level or a connection failure occurred. The shutdown was not clean. It can also occur if action is needed to continue the operation for nonblocking BIOs. Call SSL_get_error() with the return value ret to find out the reason. Which fits with my reproducer - due to the abort the connection was *not* shut down via SSL in a controlled manner, therefore r < 0. Hm, oddly enough, there's this tidbit in the SSL_get_error() manpage: On an unexpected EOF, versions before OpenSSL 3.0 returned SSL_ERROR_SYSCALL, nothing was added to the error stack, and errno was 0. Since OpenSSL 3.0 the returned error is SSL_ERROR_SSL with a meaningful error on the error stack. But I reproduced this with 3.1. Seems like we should just treat errno == 0 as a reason to emit the "EOF detected" message? I wonder if we should treat send/recv returning 0 different from an error message perspective during an established connection. Right now we produce could not receive data from client: Connection reset by peer because be_tls_read() sets errno to ECONNRESET - despite that not having been returned by the OS. But I guess that's a topic for another day. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 22:15 Andres Freund <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 14+ messages in thread From: Andres Freund @ 2023-12-08 22:15 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2023-12-08 11:33:16 -0800, Andres Freund wrote: > On 2023-12-08 10:51:01 -0800, Andres Freund wrote: > > On 2023-12-08 13:46:07 -0500, Tom Lane wrote: > > > Andres Freund <[email protected]> writes: > > > > On 2023-12-08 13:23:50 -0500, Tom Lane wrote: > > > >> Hmm, don't suppose you have a way to reproduce that? > > > > > > > After a bit of trying, yes. I put an abort() into pgtls_open_client(), after > > > > initialize_SSL(). Connecting does result in: > > > > LOG: could not accept SSL connection: Success > > > > > > OK. I can dig into that, unless you're already on it? > > [...] > > Seems like we should just treat errno == 0 as a reason to emit the "EOF > detected" message? I thought it'd be nice to have a test for this, particularly because it's not clear that the behaviour is consistent across openssl versions. I couldn't think of a way to do that with psql. But it's just a few lines of perl to gin up an "SSL" startup packet and then close the socket. I couldn't quite figure out when IO::Socket::INET was added, but I think it's likely been long enough, I see references from 1999. This worked easily on linux and freebsd, but not on windows and macos, where it seems to cause ECONNRESET. I thought that explicitly shutting down the socket might help, but that just additionally caused freebsd to fail. Windows uses an older openssl, so it could also be caused by the behaviour differing back then. To deal with that, I changed the test to instead check if "not accept SSL connection: Success" is not logged. I'm not sure that actually would be logged on windows, it does seem to have different strings for errors than other platforms. Greetings, Andres Freund Attachments: [text/x-diff] v1-0001-WIP-Fix-error-message-for-client-disconnects-duri.patch (3.1K, ../../[email protected]/2-v1-0001-WIP-Fix-error-message-for-client-disconnects-duri.patch) download | inline diff: From d5133a5783b743c0235e016dcb438d2dda82a07b Mon Sep 17 00:00:00 2001 From: Andres Freund <[email protected]> Date: Fri, 8 Dec 2023 13:09:38 -0800 Subject: [PATCH v1] WIP: Fix error message for client disconnects during SSL startup Author: Reviewed-by: Discussion: https://postgr.es/m/[email protected] Backpatch: --- src/backend/libpq/be-secure-openssl.c | 7 ++++- src/test/ssl/t/001_ssltests.pl | 42 +++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 6b8125695a3..3c182c2d3a4 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -496,7 +496,12 @@ aloop: WAIT_EVENT_SSL_OPEN_SERVER); goto aloop; case SSL_ERROR_SYSCALL: - if (r < 0) + /* + * If errno is 0, the client closed the socket without + * shutting down the SSL connection, e.g. because the client + * terminated. + */ + if (r < 0 && errno != 0) ereport(COMMERROR, (errcode_for_socket_access(), errmsg("could not accept SSL connection: %m"))); diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index d921f1dde9f..e23300b318e 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -213,6 +213,48 @@ $node->connect_fails( ? qr/server accepted connection without a valid SSL certificate/ : qr/sslcertmode value "require" is not supported/); + +# Check log message makes sense when initiating SSL connection establishment +# but then closing the socket immediately. In the past that sometimes was +# logged as "could not accept SSL connection: Success", which is misleading. +# Such disconnects happens regularly in real workloads, but can't easily be +# tested with psql. Therefore just establish the connection the hard way - +# it's easy enough here, because we just need to send a startup packet asking +# for SSL to be initiated. +{ + use IO::Socket::INET; + + my $sock = IO::Socket::INET->new( + PeerAddr => $SERVERHOSTADDR, + PeerPort => $node->port, + Proto => 'tcp'); + + my $log_location = -s $node->logfile; + + # ssl request is message length as 32 bit big endian, 4 byte magic "ssl" + # protocol version, terminated by a 0 byte. + my $message = pack "NNx", 4 + 4 + 1, (1234 << 16) | 5679; + + $sock->send($message); + $sock->close(); + + # On at least macos and windows closing the socket leads to a ECONNRESET + # on the server side. Therefore we don't check if the right message is + # logged, but that the "Success" message isn't. + # + # First wait for the connection attempt to be logged, so we can test for + # the absence of the "Success" message without a race. + ok( $node->wait_for_log( + qr/could not accept SSL connection:/, + $log_location), + 'aborted connection attempt is logged'); + + $node->log_check("aborted connection attempt is logged as failure", + $log_location, + (log_unlike => [qr/could not accept SSL connection: Success/])); +} + + # CRL tests # Invalid CRL filename is the same as no CRL, succeeds -- 2.38.0 ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 22:29 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 14+ messages in thread From: Tom Lane @ 2023-12-08 22:29 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: >> I think I figured it it out. Looks like we need to translate a closed socket >> (recvfrom() returning 0) to ECONNRESET or such. > Seems like we should just treat errno == 0 as a reason to emit the "EOF > detected" message? Agreed. I think we want to do that after the initial handshake, too, so maybe as attached. regards, tom lane Attachments: [text/x-diff] handle-openssl-returning-errno-zero.patch (958B, ../../[email protected]/2-handle-openssl-returning-errno-zero.patch) download | inline diff: diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 6b8125695a..f0b35f08c6 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -460,6 +460,7 @@ aloop: * per-thread error queue following another call to an OpenSSL I/O * routine. */ + errno = 0; ERR_clear_error(); r = SSL_accept(port->ssl); if (r <= 0) @@ -496,7 +497,7 @@ aloop: WAIT_EVENT_SSL_OPEN_SERVER); goto aloop; case SSL_ERROR_SYSCALL: - if (r < 0) + if (r < 0 && errno != 0) ereport(COMMERROR, (errcode_for_socket_access(), errmsg("could not accept SSL connection: %m"))); @@ -732,7 +733,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor) break; case SSL_ERROR_SYSCALL: /* leave it to caller to ereport the value of errno */ - if (n != -1) + if (n != -1 || errno == 0) { errno = ECONNRESET; n = -1; ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-08 22:35 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 14+ messages in thread From: Tom Lane @ 2023-12-08 22:35 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers Andres Freund <[email protected]> writes: > I thought it'd be nice to have a test for this, particularly because it's not > clear that the behaviour is consistent across openssl versions. Perhaps, but ... > To deal with that, I changed the test to instead check if "not accept SSL > connection: Success" is not logged. ... testing only that much seems entirely not worth the cycles, given the shape of the patches we both just made. If we can't rely on "errno != 0" to ensure we won't get "Success", there is one heck of a lot of other code that will be broken worse than this. regards, tom lane ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-19 16:11 Robert Haas <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 2 replies; 14+ messages in thread From: Robert Haas @ 2023-12-19 16:11 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Fri, Dec 8, 2023 at 1:34 PM Andres Freund <[email protected]> wrote: > Oh, very much agreed. But I suspect we won't quickly do the same for > out-of-core extensions... I feel like this is a problem that will sort itself out just fine. The rules about using ereport() and elog() could probably be better documented than they are, but doing that won't cause people to follow the practices any more rigorously than they have been. However, a change like this just might. If we make this policy change in core, then extension authors will probably get pressure from users to clean up any calls that are emitting excessively verbose log output, and that seems like a good thing. It's impossible to make an omelet without breaking some eggs, but the thing we're talking about here is, IMHO, extremely important. Users are forever hitting weird errors in production that aren't easy to reproduce on test systems, and because most elog() calls are written with the expectation that they won't be hit, they often contain minimal information, which IME makes it really difficult to understand what went wrong. A lot of these are things like - oh, this function expected a valid value of some sort, say a relkind, and it got some nonsense value, say a zero byte. But where did that nonsense value originate? That elog message can't tell you that, but a stack trace will. The last change we made in this area that, at least for me, massively improved debuggability was the change to log the current query string when a backend crashes. That's such a huge help; I can't imagine going back to the old way where you had basically no idea what made things go boom. I think doing something like this can have a similarly positive impact. It is going to take some work - from us and from extension authors - to tidy things up so that it doesn't produce a bunch of unwanted output, but the payoff will be the ability to actually find and fix the bugs instead of just saying to a customer "hey, sucks that you hit a bug, let us know if you find a reproducer." -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-19 16:29 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 2 replies; 14+ messages in thread From: Tom Lane @ 2023-12-19 16:29 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Robert Haas <[email protected]> writes: > The last change we made in this area that, at least for me, massively > improved debuggability was the change to log the current query string > when a backend crashes. That's such a huge help; I can't imagine going > back to the old way where you had basically no idea what made things > go boom. I think doing something like this can have a similarly > positive impact. It is going to take some work - from us and from > extension authors - to tidy things up so that it doesn't produce a > bunch of unwanted output, but the payoff will be the ability to > actually find and fix the bugs instead of just saying to a customer > "hey, sucks that you hit a bug, let us know if you find a reproducer." IMO, we aren't really going to get a massive payoff from this with the current backtrace output; it's just not detailed enough. It's better than nothing certainly, but to really move the goalposts we'd need something approaching gdb's "bt full" output. I wonder if it'd be sane to try to auto-invoke gdb. That's just blue sky for now, though. In the meantime, I agree with the proposal as it stands (that is, auto-backtrace on any XX000 error). We'll soon find out whether it's useless, or needs more detail to be really helpful, or is just right as it is. Once we have some practical experience with it, we can course-correct as needed. regards, tom lane ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-19 19:22 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 14+ messages in thread From: Robert Haas @ 2023-12-19 19:22 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Tue, Dec 19, 2023 at 11:29 AM Tom Lane <[email protected]> wrote: > IMO, we aren't really going to get a massive payoff from this with > the current backtrace output; it's just not detailed enough. It's > better than nothing certainly, but to really move the goalposts > we'd need something approaching gdb's "bt full" output. I wonder > if it'd be sane to try to auto-invoke gdb. That's just blue sky > for now, though. In the meantime, I agree with the proposal as it > stands (that is, auto-backtrace on any XX000 error). We'll soon find > out whether it's useless, or needs more detail to be really helpful, > or is just right as it is. Once we have some practical experience > with it, we can course-correct as needed. That all seems fair to me. I'm more optimistic than you are about getting something useful out of the current backtrace output, but (1) I could be wrong, (2) I'd still like to have something better, and (3) improving the backtrace output is a separate project from including backtraces more frequently. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-20 09:30 Jelte Fennema-Nio <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 14+ messages in thread From: Jelte Fennema-Nio @ 2023-12-20 09:30 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Tue, 19 Dec 2023 at 17:12, Robert Haas <[email protected]> wrote: > On Fri, Dec 8, 2023 at 1:34 PM Andres Freund <[email protected]> wrote: > > Oh, very much agreed. But I suspect we won't quickly do the same for > > out-of-core extensions... > > I feel like this is a problem that will sort itself out just fine. The > rules about using ereport() and elog() could probably be better > documented than they are, but doing that won't cause people to follow > the practices any more rigorously than they have been. However, a > change like this just might. If we make this policy change in core, > then extension authors will probably get pressure from users to clean > up any calls that are emitting excessively verbose log output, and > that seems like a good thing. As an extension author I wanted to make clear that Andres his concern is definitely not theoretical. Citus (as well as most other extensions me and our team at Microsoft maintains) use ereport without an error code very often. And while we try to use elog actually only for internal errors, there's definitely places where we haven't. We've had "adding error codes to all our errors" on our backlog for years though. I'm guessing this is mostly a combination of it being a boring task, it being a lot of work, and the impact not being particularly huge (i.e. now users can check error codes for all our errors wohoo!). If ereport without an errorcode would suddenly cause a log flood in the next postgres release then suddenly the impact of adding error codes would increase drastically. And as Robert said we'd basically be forced to adopt the pattern. Which I agree isn't necessarily a bad thing. But I'm not sure that smaller extensions that are not maintained by a team that's paid to do so would be happy about this change. Also I think we'd even change our extension to add errror codes to all ereport calls if the stack traces are useful enough, because then the impact of adding error codes suddenly increases a lot. So I think having a way for extensions to opt-in/opt-out of this change for their extension would be very much appreciated by those authors. ^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: backtrace_on_internal_error @ 2023-12-30 11:11 Peter Eisentraut <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 14+ messages in thread From: Peter Eisentraut @ 2023-12-30 11:11 UTC (permalink / raw) To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers On 19.12.23 17:29, Tom Lane wrote: > IMO, we aren't really going to get a massive payoff from this with > the current backtrace output; it's just not detailed enough. It's > better than nothing certainly, but to really move the goalposts > we'd need something approaching gdb's "bt full" output. I wonder > if it'd be sane to try to auto-invoke gdb. That's just blue sky > for now, though. In the meantime, I agree with the proposal as it > stands (that is, auto-backtrace on any XX000 error). We'll soon find > out whether it's useless, or needs more detail to be really helpful, > or is just right as it is. Once we have some practical experience > with it, we can course-correct as needed. Based on this, I have committed my original patch. ^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2023-12-30 11:11 UTC | newest] Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-02 18:27 [PATCH 4/8] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]> 2023-12-08 18:23 Re: backtrace_on_internal_error Tom Lane <[email protected]> 2023-12-08 18:34 ` Re: backtrace_on_internal_error Andres Freund <[email protected]> 2023-12-08 18:46 ` Re: backtrace_on_internal_error Tom Lane <[email protected]> 2023-12-08 18:51 ` Re: backtrace_on_internal_error Andres Freund <[email protected]> 2023-12-08 19:33 ` Re: backtrace_on_internal_error Andres Freund <[email protected]> 2023-12-08 22:15 ` Re: backtrace_on_internal_error Andres Freund <[email protected]> 2023-12-08 22:35 ` Re: backtrace_on_internal_error Tom Lane <[email protected]> 2023-12-08 22:29 ` Re: backtrace_on_internal_error Tom Lane <[email protected]> 2023-12-19 16:11 ` Re: backtrace_on_internal_error Robert Haas <[email protected]> 2023-12-19 16:29 ` Re: backtrace_on_internal_error Tom Lane <[email protected]> 2023-12-19 19:22 ` Re: backtrace_on_internal_error Robert Haas <[email protected]> 2023-12-30 11:11 ` Re: backtrace_on_internal_error Peter Eisentraut <[email protected]> 2023-12-20 09:30 ` Re: backtrace_on_internal_error Jelte Fennema-Nio <[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