public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions 12+ messages / 5 participants [nested] [flat]
* [PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions @ 2020-09-17 15:26 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 12+ 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 --------------CF71AF65F7C337C37C24B045 Content-Type: text/x-patch; charset=UTF-8; name="0003-Optimize-allocations-in-bringetbitmap-20210112.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0003-Optimize-allocations-in-bringetbitmap-20210112.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re:Possible corruption by CreateRestartPoint at promotion @ 2022-04-27 04:36 =?ISO-8859-1?B?UnVpIFpoYW8=?= <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: =?ISO-8859-1?B?UnVpIFpoYW8=?= @ 2022-04-27 04:36 UTC (permalink / raw) To: =?ISO-8859-1?B?S3lvdGFybyBIb3JpZ3VjaGk=?= <[email protected]>; +Cc: pgsql-hackers Kyotaro's patch seems good to me and fixes the test case in my patch. Do you have interest in adding a test like one in my patch? > + LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); > + > /* > * Remember the prior checkpoint's redo ptr for > * UpdateCheckPointDistanceEstimate() > */ > PriorRedoPtr = ControlFile->checkPointCopy.redo; > > + Assert (PriorRedoPtr < RedoRecPtr);Maybe PriorRedoPtr does not need to be under LWLockAcquire? regards. -- Zhao Rui Alibaba Cloud: https://www.aliyun.com/ ------------------ Original ------------------ From: "Kyotaro Horiguchi" <[email protected]>; Date: Wed, Mar 16, 2022 09:24 AM To: "pgsql-hackers"<[email protected]>; Cc: "masao.fujii"<[email protected]>; Subject: Possible corruption by CreateRestartPoint at promotion Hello, (Cc:ed Fujii-san) This is a diverged topic from [1], which is summarized as $SUBJECT. To recap: While discussing on additional LSNs in checkpoint log message, Fujii-san pointed out [2] that there is a case where CreateRestartPoint leaves unrecoverable database when concurrent promotion happens. That corruption is "fixed" by the next checkpoint so it is not a severe corruption. AFAICS since 9.5, no check(/restart)pionts won't run concurrently with restartpoint [3]. So I propose to remove the code path as attached. regards. [1] https://www.postgresql.org/message-id/20220316.091913.806120467943749797.horikyota.ntt%40gmail.com [2] https://www.postgresql.org/message-id/7bfad665-db9c-0c2a-2604-9f54763c5f9e%40oss.nttdata.com [3] https://www.postgresql.org/message-id/20220222.174401.765586897814316743.horikyota.ntt%40gmail.com -- Kyotaro Horiguchi NTT Open Source Software Center Attachments: [application/octet-stream] 0001-Test-of-this-problem-v14.patch (3.9K, ../../[email protected]/3-0001-Test-of-this-problem-v14.patch) download | inline diff: From 12afd1644d7413340fde57a57b69ebfbb6f1511b Mon Sep 17 00:00:00 2001 From: Zhao Rui <[email protected]> Date: Wed, 27 Apr 2022 11:38:05 +0800 Subject: [PATCH] Test of primary crash continually with invalid checkpoint after promote --- .../t/027_invalid_checkpoint_after_promote.pl | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 src/test/recovery/t/027_invalid_checkpoint_after_promote.pl diff --git a/src/test/recovery/t/027_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/027_invalid_checkpoint_after_promote.pl new file mode 100644 index 0000000000..9cf6db5c4c --- /dev/null +++ b/src/test/recovery/t/027_invalid_checkpoint_after_promote.pl @@ -0,0 +1,113 @@ +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Time::HiRes qw(usleep nanosleep); +use Test::More tests => 5; + +# initialize primary node +my $node_primary = get_new_node('master'); +$node_primary->init(allows_streaming => 1); +$node_primary->append_conf( + 'postgresql.conf', q[ +checkpoint_timeout = 30s +max_wal_size = 16GB +log_checkpoints = on +restart_after_crash = on +]); +$node_primary->start; +my $backup_name = 'my_backup'; +$node_primary->backup($backup_name); + +# setup a standby +my $node_standby = get_new_node('standby1'); +$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1); +$node_standby->start; + +# dummy table for the upcoming tests. +$node_primary->safe_psql('postgres', 'checkpoint'); +$node_primary->safe_psql('postgres', 'create table test (a int, b varchar(255))'); + +# use background psql to insert batch, just to make checkpoint a little slow. +my $psql_timeout = IPC::Run::timer(3600); +my ($stdin, $stdout, $stderr) = ('', '', ''); +my $psql_primary = IPC::Run::start( + [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_primary->connstr('postgres') ], + '<', + \$stdin, + '>', + \$stdout, + '2>', + \$stderr, + $psql_timeout); +$stdin .= q[ +INSERT INTO test SELECT i, 'aaaaaaaaaaaaaaaaaaaaaaa' from generate_series(1, 100000000) as i; +]; +$psql_primary->pump_nb(); + +# wait until restartpoint on standy +my $logstart = -s $node_standby->logfile; +my $checkpoint_start = 0; +for (my $i = 0; $i < 3000; $i++) +{ + my $log = TestLib::slurp_file($node_standby->logfile, $logstart); + if ($log =~ m/restartpoint starting: time/) + { + $checkpoint_start = 1; + last; + } + usleep(100_000); +} +is($checkpoint_start, 1, 'restartpoint has started'); + +# promote during restartpoint +$node_primary->stop; +$node_standby->promote; + +# wait until checkpoint on new primary +$logstart = -s $node_standby->logfile; +$checkpoint_start = 0; +for (my $i = 0; $i < 3000; $i++) +{ + my $log = TestLib::slurp_file($node_standby->logfile, $logstart); + if ($log =~ m/restartpoint complete/) + { + $checkpoint_start = 1; + last; + } + usleep(100_000); +} +is($checkpoint_start, 1, 'checkpoint has started'); + +# kill SIGKILL a backend, and all backend will restart. Note that previous checkpoint has not completed. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/; +my $pid = $killme_stdout; +chomp($pid); +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# after recovery, the server will not start, and log PANIC: could not locate a valid checkpoint record +for (my $i = 0; $i < 30; $i++) +{ + ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1'); + last if $ret == 0; + sleep(1); +} +is($ret, 0, "psql connect success"); +is($stdout, 1, "psql select 1"); + +$psql_primary->finish; +$killme->finish; \ No newline at end of file -- 2.32.0.3.g01195cf9f ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-04-27 05:16 Michael Paquier <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Michael Paquier @ 2022-04-27 05:16 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected] On Tue, Apr 26, 2022 at 08:26:09PM -0700, Nathan Bossart wrote: > On Wed, Apr 27, 2022 at 10:43:53AM +0900, Kyotaro Horiguchi wrote: >> At Tue, 26 Apr 2022 11:33:49 -0700, Nathan Bossart <[email protected]> wrote in >>> I suspect we'll start seeing this problem more often once end-of-recovery >>> checkpoints are removed [0]. Would you mind creating a commitfest entry >>> for this thread? I didn't see one. >> >> I'm not sure the patch makes any change here, because restart points >> don't run while crash recovery, since no checkpoint records seen >> during a crash recovery. Anyway the patch doesn't apply anymore so >> rebased, but only the one for master for the lack of time for now. > > Thanks for the new patch! Yeah, it wouldn't affect crash recovery, but > IIUC Robert's patch also applies to archive recovery. > >> + /* Update pg_control */ >> + LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); >> + >> /* >> * Remember the prior checkpoint's redo ptr for >> * UpdateCheckPointDistanceEstimate() >> */ >> PriorRedoPtr = ControlFile->checkPointCopy.redo; > > nitpick: Why move the LWLockAcquire() all the way up here? Yeah, that should not be necessary. InitWalRecovery() is the only place outside the checkpointer that would touch this field, but that happens far too early in the startup sequence to matter with the checkpointer. >> + Assert (PriorRedoPtr < RedoRecPtr); > > I think this could use a short explanation. That's just to make sure that the current redo LSN is always older than the one prior that. It does not seem really necessary to me to add that. >> + /* >> + * Aarchive recovery has ended. Crash recovery ever after should >> + * always recover to the end of WAL >> + */ s/Aarchive/Archive/. >> + ControlFile->minRecoveryPoint = InvalidXLogRecPtr; >> + ControlFile->minRecoveryPointTLI = 0; >> + >> + /* also update local copy */ >> + LocalMinRecoveryPoint = InvalidXLogRecPtr; >> + LocalMinRecoveryPointTLI = 0; > > Should this be handled by the code that changes the control file state to > DB_IN_PRODUCTION instead? It looks like this is ordinarily done in the > next checkpoint. It's not clear to me why it is done this way. Anyway, that would be the work of the end-of-recovery checkpoint requested at the end of StartupXLOG() once a promotion happens or of the checkpoint requested by PerformRecoveryXLogAction() in the second case, no? So, I don't quite see why we need to update minRecoveryPoint and minRecoveryPointTLI in the control file here, as much as this does not have to be part of the end-of-recovery code that switches the control file to DB_IN_PRODUCTION. - if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY && - ControlFile->checkPointCopy.redo < lastCheckPoint.redo) - { 7ff23c6 has removed the last call to CreateCheckpoint() outside the checkpointer, meaning that there is one less concurrent race to worry about, but I have to admit that this change, to update the control file's checkPoint and checkPointCopy even if we don't check after ControlFile->checkPointCopy.redo < lastCheckPoint.redo would make the code less robust in ~14. So I am questioning whether a backpatch is actually worth the risk here. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-04-27 05:27 Michael Paquier <[email protected]> parent: =?ISO-8859-1?B?UnVpIFpoYW8=?= <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Michael Paquier @ 2022-04-27 05:27 UTC (permalink / raw) To: Rui Zhao <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; pgsql-hackers On Wed, Apr 27, 2022 at 12:36:10PM +0800, Rui Zhao wrote: > Do you have interest in adding a test like one in my patch? I have studied the test case you are proposing, and I am afraid that it is too expensive as designed. And it is actually racy as you expect the restart point to take longer than the promotion with a timing based on an arbitrary (and large!) amount of data inserted into the primary. Well, the promotion should be shorter than the restart point in any case, but such tests should be designed so as they would work reliably on slow machines while being able to complete quickly on fast machines. It would much better if the test is designed so as the restart point is stopped at an arbitrary step rather than throttled, moving on when the promotion of the standby is done. A well-known method, that would not work on Windows, is to rely on SIGSTOP that could be used on the checkpointer for such things. Anyway, we don't have any mean to reliably stop a restart point while in the middle of its processing, do we? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-04-28 00:12 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Michael Paquier @ 2022-04-28 00:12 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected] On Wed, Apr 27, 2022 at 11:09:45AM -0700, Nathan Bossart wrote: > On Wed, Apr 27, 2022 at 02:16:01PM +0900, Michael Paquier wrote: >> - if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY && >> - ControlFile->checkPointCopy.redo < lastCheckPoint.redo) >> - { >> 7ff23c6 has removed the last call to CreateCheckpoint() outside the >> checkpointer, meaning that there is one less concurrent race to worry >> about, but I have to admit that this change, to update the control >> file's checkPoint and checkPointCopy even if we don't check after >> ControlFile->checkPointCopy.redo < lastCheckPoint.redo would make the >> code less robust in ~14. So I am questioning whether a backpatch >> is actually worth the risk here. > > IMO we should still check this before updating ControlFile to be safe. Sure. Fine by me to play it safe. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-04-28 02:43 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Kyotaro Horiguchi @ 2022-04-28 02:43 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected] At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <[email protected]> wrote in > On Wed, Apr 27, 2022 at 11:09:45AM -0700, Nathan Bossart wrote: > > On Wed, Apr 27, 2022 at 02:16:01PM +0900, Michael Paquier wrote: > >> - if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY && > >> - ControlFile->checkPointCopy.redo < lastCheckPoint.redo) > >> - { > >> 7ff23c6 has removed the last call to CreateCheckpoint() outside the > >> checkpointer, meaning that there is one less concurrent race to worry > >> about, but I have to admit that this change, to update the control > >> file's checkPoint and checkPointCopy even if we don't check after > >> ControlFile->checkPointCopy.redo < lastCheckPoint.redo would make the > >> code less robust in ~14. So I am questioning whether a backpatch > >> is actually worth the risk here. > > > > IMO we should still check this before updating ControlFile to be safe. > > Sure. Fine by me to play it safe. Why do we consider concurrent check/restart points here while we don't consider the same for ControlFile->checkPointCopy? regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-04-28 06:49 Michael Paquier <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 12+ messages in thread From: Michael Paquier @ 2022-04-28 06:49 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected] On Thu, Apr 28, 2022 at 11:43:57AM +0900, Kyotaro Horiguchi wrote: > At Thu, 28 Apr 2022 09:12:13 +0900, Michael Paquier <[email protected]> wrote in >> On Wed, Apr 27, 2022 at 11:09:45AM -0700, Nathan Bossart wrote: >>> On Wed, Apr 27, 2022 at 02:16:01PM +0900, Michael Paquier wrote: >>>> - if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY && >>>> - ControlFile->checkPointCopy.redo < lastCheckPoint.redo) >>>> - { >>>> 7ff23c6 has removed the last call to CreateCheckpoint() outside the >>>> checkpointer, meaning that there is one less concurrent race to worry >>>> about, but I have to admit that this change, to update the control >>>> file's checkPoint and checkPointCopy even if we don't check after >>>> ControlFile->checkPointCopy.redo < lastCheckPoint.redo would make the >>>> code less robust in ~14. So I am questioning whether a backpatch >>>> is actually worth the risk here. >>> >>> IMO we should still check this before updating ControlFile to be safe. >> >> Sure. Fine by me to play it safe. > > Why do we consider concurrent check/restart points here while we don't > consider the same for ControlFile->checkPointCopy? I am not sure what you mean here. FWIW, I am translating the suggestion of Nathan to split the existing check in CreateRestartPoint() that we are discussing here into two if blocks, instead of just one: - Move the update of checkPoint and checkPointCopy into its own if block, controlled only by the check on (ControlFile->checkPointCopy.redo < lastCheckPoint.redo) - Keep the code updating minRecoveryPoint and minRecoveryPointTLI mostly as-is, but just do the update when the control file state is DB_IN_ARCHIVE_RECOVERY. Of course we need to keep the check on (minRecoveryPoint < lastCheckPointEndPtr). v5 is mostly doing that. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-05-06 10:58 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 12+ messages in thread From: Michael Paquier @ 2022-05-06 10:58 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected] On Thu, Apr 28, 2022 at 03:49:42PM +0900, Michael Paquier wrote: > I am not sure what you mean here. FWIW, I am translating the > suggestion of Nathan to split the existing check in > CreateRestartPoint() that we are discussing here into two if blocks, > instead of just one: > - Move the update of checkPoint and checkPointCopy into its own if > block, controlled only by the check on > (ControlFile->checkPointCopy.redo < lastCheckPoint.redo) > - Keep the code updating minRecoveryPoint and minRecoveryPointTLI > mostly as-is, but just do the update when the control file state is > DB_IN_ARCHIVE_RECOVERY. Of course we need to keep the check on > (minRecoveryPoint < lastCheckPointEndPtr). And I have spent a bit of this stuff to finish with the attached. It will be a plus to get that done on HEAD for beta1, so I'll try to deal with it on Monday. I am still a bit stressed about the back branches as concurrent checkpoints are possible via CreateCheckPoint() from the startup process (not the case of HEAD), but the stable branches will have a new point release very soon so let's revisit this choice there later. v6 attached includes a TAP test, but I don't intend to include it as it is expensive. -- Michael Attachments: [text/x-diff] v6-0001-Correctly-update-control-file-at-promotion.patch (7.9K, ../../YnT%[email protected]/2-v6-0001-Correctly-update-control-file-at-promotion.patch) download | inline diff: From ffc1469819b28ff76e3290cd695e60511d94305a Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Fri, 6 May 2022 19:57:54 +0900 Subject: [PATCH v6] Correctly update control file at promotion If the cluster ends recovery on a promotion (aka the control file shows a state different than DB_IN_ARCHIVE_RECOVERY) while CreateRestartPoint() is still processing, this function could miss the update the control file but still do the recycling/removal of the past WAL segments, causing a follow-up restart doing recovery to fail on a missing checkpoint record (aka PANIC) because the redo LSN referred in the control file was located in a segment already gone. This commit fixes the control file update so the redo LSN is updated to reflect what gets recycled, with the minimum recovery point changed only if the cluster is still doing archive recovery. --- src/backend/access/transam/xlog.c | 48 ++++---- .../t/027_invalid_checkpoint_after_promote.pl | 113 ++++++++++++++++++ 2 files changed, 141 insertions(+), 20 deletions(-) create mode 100644 src/test/recovery/t/027_invalid_checkpoint_after_promote.pl diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 61cda56c6f..c9c0b3f90a 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6921,6 +6921,9 @@ CreateRestartPoint(int flags) XLogSegNo _logSegNo; TimestampTz xtime; + /* Concurrent checkpoint/restartpoint cannot happen */ + Assert (!IsUnderPostmaster || MyBackendType == B_CHECKPOINTER); + /* Get a local copy of the last safe checkpoint record. */ SpinLockAcquire(&XLogCtl->info_lck); lastCheckPointRecPtr = XLogCtl->lastCheckPointRecPtr; @@ -7013,30 +7016,33 @@ CreateRestartPoint(int flags) */ PriorRedoPtr = ControlFile->checkPointCopy.redo; - /* - * Update pg_control, using current time. Check that it still shows - * DB_IN_ARCHIVE_RECOVERY state and an older checkpoint, else do nothing; - * this is a quick hack to make sure nothing really bad happens if somehow - * we get here after the end-of-recovery checkpoint. - */ + /* Update pg_control, using current time */ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); - if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY && - ControlFile->checkPointCopy.redo < lastCheckPoint.redo) + + /* + * Check if the control file still shows an older checkpoint, and update + * its information. + */ + if (ControlFile->checkPointCopy.redo < lastCheckPoint.redo) { ControlFile->checkPoint = lastCheckPointRecPtr; ControlFile->checkPointCopy = lastCheckPoint; + } - /* - * Ensure minRecoveryPoint is past the checkpoint record. Normally, - * this will have happened already while writing out dirty buffers, - * but not necessarily - e.g. because no buffers were dirtied. We do - * this because a backup performed in recovery uses minRecoveryPoint to - * determine which WAL files must be included in the backup, and the - * file (or files) containing the checkpoint record must be included, - * at a minimum. Note that for an ordinary restart of recovery there's - * no value in having the minimum recovery point any earlier than this - * anyway, because redo will begin just after the checkpoint record. - */ + /* + * Ensure minRecoveryPoint is past the checkpoint record while archive + * recovery is still ongoing. Normally, this will have happened already + * while writing out dirty buffers, but not necessarily - e.g. because no + * buffers were dirtied. We do this because a base backup uses + * minRecoveryPoint to determine which WAL files must be included in the + * backup, and the file (or files) containing the checkpoint record must + * be included, at a minimum. Note that for an ordinary restart of + * recovery there's no value in having the minimum recovery point any + * earlier than this anyway, because redo will begin just after the + * checkpoint record. + */ + if (ControlFile->state == DB_IN_ARCHIVE_RECOVERY) + { if (ControlFile->minRecoveryPoint < lastCheckPointEndPtr) { ControlFile->minRecoveryPoint = lastCheckPointEndPtr; @@ -7046,10 +7052,12 @@ CreateRestartPoint(int flags) LocalMinRecoveryPoint = ControlFile->minRecoveryPoint; LocalMinRecoveryPointTLI = ControlFile->minRecoveryPointTLI; } + if (flags & CHECKPOINT_IS_SHUTDOWN) ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY; - UpdateControlFile(); } + + UpdateControlFile(); LWLockRelease(ControlFileLock); /* diff --git a/src/test/recovery/t/027_invalid_checkpoint_after_promote.pl b/src/test/recovery/t/027_invalid_checkpoint_after_promote.pl new file mode 100644 index 0000000000..15c8d0a01a --- /dev/null +++ b/src/test/recovery/t/027_invalid_checkpoint_after_promote.pl @@ -0,0 +1,113 @@ +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Time::HiRes qw(usleep nanosleep); +use Test::More tests => 5; + +# initialize primary node +my $node_primary = PostgreSQL::Test::Cluster->new('master'); +$node_primary->init(allows_streaming => 1); +$node_primary->append_conf( + 'postgresql.conf', q[ +checkpoint_timeout = 30s +max_wal_size = 16GB +log_checkpoints = on +restart_after_crash = on +]); +$node_primary->start; +my $backup_name = 'my_backup'; +$node_primary->backup($backup_name); + +# setup a standby +my $node_standby = PostgreSQL::Test::Cluster->new('standby1'); +$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1); +$node_standby->start; + +# dummy table for the upcoming tests. +$node_primary->safe_psql('postgres', 'checkpoint'); +$node_primary->safe_psql('postgres', 'create table test (a int, b varchar(255))'); + +# use background psql to insert batch, just to make checkpoint a little slow. +my $psql_timeout = IPC::Run::timer(3600); +my ($stdin, $stdout, $stderr) = ('', '', ''); +my $psql_primary = IPC::Run::start( + [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_primary->connstr('postgres') ], + '<', + \$stdin, + '>', + \$stdout, + '2>', + \$stderr, + $psql_timeout); +$stdin .= q[ +INSERT INTO test SELECT i, 'aaaaaaaaaaaaaaaaaaaaaaa' from generate_series(1, 100000000) as i; +]; +$psql_primary->pump_nb(); + +# wait until restartpoint on standy +my $logstart = -s $node_standby->logfile; +my $checkpoint_start = 0; +for (my $i = 0; $i < 3000; $i++) +{ + my $log = slurp_file($node_standby->logfile, $logstart); + if ($log =~ m/restartpoint starting: time/) + { + $checkpoint_start = 1; + last; + } + usleep(100_000); +} +is($checkpoint_start, 1, 'restartpoint has started'); + +# promote during restartpoint +$node_primary->stop; +$node_standby->promote; + +# wait until checkpoint on new primary +$logstart = -s $node_standby->logfile; +$checkpoint_start = 0; +for (my $i = 0; $i < 3000; $i++) +{ + my $log = slurp_file($node_standby->logfile, $logstart); + if ($log =~ m/restartpoint complete/) + { + $checkpoint_start = 1; + last; + } + usleep(100_000); +} +is($checkpoint_start, 1, 'checkpoint has started'); + +# kill SIGKILL a backend, and all backend will restart. Note that previous checkpoint has not completed. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/; +my $pid = $killme_stdout; +chomp($pid); +my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# after recovery, the server will not start, and log PANIC: could not locate a valid checkpoint record +for (my $i = 0; $i < 30; $i++) +{ + ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1'); + last if $ret == 0; + sleep(1); +} +is($ret, 0, "psql connect success"); +is($stdout, 1, "psql select 1"); + +$psql_primary->finish; +$killme->finish; -- 2.36.0 [application/pgp-signature] signature.asc (833B, ../../YnT%[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-05-06 15:52 Nathan Bossart <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Nathan Bossart @ 2022-05-06 15:52 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected] On Fri, May 06, 2022 at 07:58:43PM +0900, Michael Paquier wrote: > And I have spent a bit of this stuff to finish with the attached. It > will be a plus to get that done on HEAD for beta1, so I'll try to deal > with it on Monday. I am still a bit stressed about the back branches > as concurrent checkpoints are possible via CreateCheckPoint() from the > startup process (not the case of HEAD), but the stable branches will > have a new point release very soon so let's revisit this choice there > later. v6 attached includes a TAP test, but I don't intend to include > it as it is expensive. I was looking at other changes in this area (e.g., 3c64dcb), and now I'm wondering if we actually should invalidate the minRecoveryPoint when the control file no longer indicates archive recovery. Specifically, what happens if a base backup of a newly promoted standby is used for a point-in-time restore? If the checkpoint location is updated and all previous segments have been recycled/removed, it seems like the minRecoveryPoint might point to a missing segment. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-05-06 23:43 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Michael Paquier @ 2022-05-06 23:43 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected] On Fri, May 06, 2022 at 08:52:45AM -0700, Nathan Bossart wrote: > I was looking at other changes in this area (e.g., 3c64dcb), and now I'm > wondering if we actually should invalidate the minRecoveryPoint when the > control file no longer indicates archive recovery. Specifically, what > happens if a base backup of a newly promoted standby is used for a > point-in-time restore? If the checkpoint location is updated and all > previous segments have been recycled/removed, it seems like the > minRecoveryPoint might point to a missing segment. A new checkpoint is enforced at the beginning of the backup which would update minRecoveryPoint and minRecoveryPointTLI, while we don't a allow a backup to finish if it began on a standby has just promoted in-between. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-05-09 00:24 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 1 sibling, 1 reply; 12+ messages in thread From: Michael Paquier @ 2022-05-09 00:24 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected] On Fri, May 06, 2022 at 07:58:43PM +0900, Michael Paquier wrote: > And I have spent a bit of this stuff to finish with the attached. It > will be a plus to get that done on HEAD for beta1, so I'll try to deal > with it on Monday. I am still a bit stressed about the back branches > as concurrent checkpoints are possible via CreateCheckPoint() from the > startup process (not the case of HEAD), but the stable branches will > have a new point release very soon so let's revisit this choice there > later. v6 attached includes a TAP test, but I don't intend to include > it as it is expensive. Okay, applied this one on HEAD after going back-and-forth on it for the last couple of days. I have found myself shaping the patch in what looks like its simplest form, by applying the check based on an older checkpoint to all the fields updated in the control file, with the check on DB_IN_ARCHIVE_RECOVERY applying to the addition of DB_SHUTDOWNED_IN_RECOVERY (got initialially surprised that this was having side effects on pg_rewind) and the minRecoveryPoint calculations. Now, it would be nice to get a test for this stuff, and we are going to need something cheaper than what's been proposed upthread. This comes down to the point of being able to put a deterministic stop in a restart point while it is processing, meaning that we need to interact with one of the internal routines of CheckPointGuts(). One fancy way to do so would be to forcibly take a LWLock to stuck the restart point until it is released. Using a SQL function for that would be possible, if not artistic. Perhaps we don't need such a function though, if we could stuck arbitrarily the internals of a checkpoint? Any ideas? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Possible corruption by CreateRestartPoint at promotion @ 2022-05-16 03:47 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Michael Paquier @ 2022-05-16 03:47 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected] On Mon, May 09, 2022 at 09:24:06AM +0900, Michael Paquier wrote: > Okay, applied this one on HEAD after going back-and-forth on it for > the last couple of days. I have found myself shaping the patch in > what looks like its simplest form, by applying the check based on an > older checkpoint to all the fields updated in the control file, with > the check on DB_IN_ARCHIVE_RECOVERY applying to the addition of > DB_SHUTDOWNED_IN_RECOVERY (got initialially surprised that this was > having side effects on pg_rewind) and the minRecoveryPoint > calculations. It took me some time to make sure that this would be safe, but done now for all the stable branches. > Now, it would be nice to get a test for this stuff, and we are going > to need something cheaper than what's been proposed upthread. This > comes down to the point of being able to put a deterministic stop > in a restart point while it is processing, meaning that we need to > interact with one of the internal routines of CheckPointGuts(). One > fancy way to do so would be to forcibly take a LWLock to stuck the > restart point until it is released. Using a SQL function for that > would be possible, if not artistic. Perhaps we don't need such a > function though, if we could stuck arbitrarily the internals of a > checkpoint? Any ideas? One thing that we could do here is to resurrect the patch that adds support for stop points in the code.. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../YoHJUSht3ulTO%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2022-05-16 03:47 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-17 15:26 [PATCH 2/8] Move IS [NOT] NULL handling from BRIN support functions Tomas Vondra <[email protected]> 2022-04-27 04:36 Re:Possible corruption by CreateRestartPoint at promotion =?ISO-8859-1?B?UnVpIFpoYW8=?= <[email protected]> 2022-04-27 05:27 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-04-27 05:16 Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-04-28 00:12 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-04-28 02:43 ` Re: Possible corruption by CreateRestartPoint at promotion Kyotaro Horiguchi <[email protected]> 2022-04-28 06:49 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-05-06 10:58 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-05-06 15:52 ` Re: Possible corruption by CreateRestartPoint at promotion Nathan Bossart <[email protected]> 2022-05-06 23:43 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-05-09 00:24 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[email protected]> 2022-05-16 03:47 ` Re: Possible corruption by CreateRestartPoint at promotion Michael Paquier <[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