public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/6] Pass all scan keys to BRIN consistent function at once 2+ messages / 2 participants [nested] [flat]
* [PATCH 1/6] Pass all scan keys to BRIN consistent function at once @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Tomas Vondra @ 2020-09-12 13:07 UTC (permalink / raw) Passing all scan keys to the BRIN consistent function at once may allow elimination of additional ranges, which would be impossible when only passing individual scan keys. The code continues to support both the original (one scan key at a time) and new (all scan keys at once) approaches, depending on whether the consistent function accepts three or four arguments. This modifies the existing BRIN opclasses (minmax, inclusion) although those don't really benefit from this change. The primary purpose of this is to allow more advanced opclases in the future. Author: Tomas Vondra <[email protected]> Reviewed-by: Alvaro Herrera <[email protected]> Reviewed-by: Mark Dilger <[email protected]> Reviewed-by: Alexander Korotkov <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 150 +++++++++++++++----- src/backend/access/brin/brin_inclusion.c | 170 +++++++++++++++-------- src/backend/access/brin/brin_minmax.c | 121 +++++++++++----- src/backend/access/brin/brin_validate.c | 4 +- src/include/catalog/pg_proc.dat | 4 +- 5 files changed, 324 insertions(+), 125 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 27ba596c6e..dc187153aa 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -390,6 +390,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) BrinMemTuple *dtup; BrinTuple *btup = NULL; Size btupsz = 0; + ScanKey **keys; + int *nkeys; + int keyno; opaque = (BrinOpaque *) scan->opaque; bdesc = opaque->bo_bdesc; @@ -411,6 +414,61 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) */ consistentFn = palloc0(sizeof(FmgrInfo) * bdesc->bd_tupdesc->natts); + /* + * Make room for per-attribute lists of scan keys that we'll pass to the + * consistent support procedure. + */ + keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); + nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts); + + /* + * Preprocess the scan keys - split them into per-attribute arrays. + */ + for (keyno = 0; keyno < scan->numberOfKeys; keyno++) + { + ScanKey key = &scan->keyData[keyno]; + AttrNumber keyattno = key->sk_attno; + + /* + * The collation of the scan key must match the collation + * used in the index column (but only if the search is not + * IS NULL/ IS NOT NULL). Otherwise we shouldn't be using + * this index ... + */ + Assert((key->sk_flags & SK_ISNULL) || + (key->sk_collation == + TupleDescAttr(bdesc->bd_tupdesc, + keyattno - 1)->attcollation)); + + /* First time we see this index attribute, so init as needed. */ + if (!keys[keyattno-1]) + { + 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); + + tmp = index_getprocinfo(idxRel, keyattno, + BRIN_PROCNUM_CONSISTENT); + fmgr_info_copy(&consistentFn[keyattno - 1], tmp, + CurrentMemoryContext); + } + + /* Add key to the per-attribute array. */ + keys[keyattno - 1][nkeys[keyattno - 1]] = key; + nkeys[keyattno - 1]++; + } + /* allocate an initial in-memory tuple, out of the per-range memcxt */ dtup = brin_new_memtuple(bdesc); @@ -471,7 +529,7 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) } else { - int keyno; + int attno; /* * Compare scan keys with summary values stored for the range. @@ -481,51 +539,75 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * no keys. */ addrange = true; - for (keyno = 0; keyno < scan->numberOfKeys; keyno++) + for (attno = 1; attno <= bdesc->bd_tupdesc->natts; attno++) { - ScanKey key = &scan->keyData[keyno]; - AttrNumber keyattno = key->sk_attno; - BrinValues *bval = &dtup->bt_columns[keyattno - 1]; + BrinValues *bval; Datum add; - /* - * The collation of the scan key must match the collation - * used in the index column (but only if the search is not - * IS NULL/ IS NOT NULL). Otherwise we shouldn't be using - * this index ... - */ - Assert((key->sk_flags & SK_ISNULL) || - (key->sk_collation == - TupleDescAttr(bdesc->bd_tupdesc, - keyattno - 1)->attcollation)); + /* skip attributes without any san keys */ + if (nkeys[attno - 1] == 0) + continue; - /* First time this column? look up consistent function */ - if (consistentFn[keyattno - 1].fn_oid == InvalidOid) - { - FmgrInfo *tmp; + bval = &dtup->bt_columns[attno - 1]; - tmp = index_getprocinfo(idxRel, keyattno, - BRIN_PROCNUM_CONSISTENT); - fmgr_info_copy(&consistentFn[keyattno - 1], tmp, - CurrentMemoryContext); - } + Assert((nkeys[attno - 1] > 0) && + (nkeys[attno - 1] <= scan->numberOfKeys)); /* * Check whether the scan key is consistent with the page * range values; if so, have the pages in the range added * to the output bitmap. * - * When there are multiple scan keys, failure to meet the - * criteria for a single one of them is enough to discard - * the range as a whole, so break out of the loop as soon - * as a false return value is obtained. + * The opclass may or may not support processing of multiple + * scan keys. We can determine that based on the number of + * arguments - functions with extra parameter (number of scan + * keys) do support this, otherwise we have to simply pass the + * scan keys one by one, */ - add = FunctionCall3Coll(&consistentFn[keyattno - 1], - key->sk_collation, - PointerGetDatum(bdesc), - PointerGetDatum(bval), - PointerGetDatum(key)); - addrange = DatumGetBool(add); + if (consistentFn[attno - 1].fn_nargs >= 4) + { + Oid collation; + + /* + * Collation from the first key (has to be the same for + * all keys for the same attribue). + */ + collation = keys[attno - 1][0]->sk_collation; + + /* Check all keys at once */ + add = FunctionCall4Coll(&consistentFn[attno - 1], + collation, + PointerGetDatum(bdesc), + PointerGetDatum(bval), + PointerGetDatum(keys[attno - 1]), + Int32GetDatum(nkeys[attno - 1])); + addrange = DatumGetBool(add); + } + else + { + /* + * Check keys one by one + * + * When there are multiple scan keys, failure to meet the + * criteria for a single one of them is enough to discard + * the range as a whole, so break out of the loop as soon + * as a false return value is obtained. + */ + int keyno; + + for (keyno = 0; keyno < nkeys[attno - 1]; keyno++) + { + add = FunctionCall3Coll(&consistentFn[attno - 1], + keys[attno - 1][keyno]->sk_collation, + PointerGetDatum(bdesc), + PointerGetDatum(bval), + PointerGetDatum(keys[attno - 1][keyno])); + addrange = DatumGetBool(add); + if (!addrange) + break; + } + } + if (!addrange) break; } diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c index 12e5bddd1f..215bc794d3 100644 --- a/src/backend/access/brin/brin_inclusion.c +++ b/src/backend/access/brin/brin_inclusion.c @@ -85,6 +85,8 @@ static FmgrInfo *inclusion_get_procinfo(BrinDesc *bdesc, uint16 attno, uint16 procnum); static FmgrInfo *inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype, uint16 strategynum); +static bool inclusion_consistent_key(BrinDesc *bdesc, BrinValues *column, + ScanKey key, Oid colloid); /* @@ -258,53 +260,109 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) { BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0); BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1); - ScanKey key = (ScanKey) PG_GETARG_POINTER(2); - Oid colloid = PG_GET_COLLATION(), - subtype; - Datum unionval; - AttrNumber attno; - Datum query; - FmgrInfo *finfo; - Datum result; - - Assert(key->sk_attno == column->bv_attno); + ScanKey *keys = (ScanKey *) PG_GETARG_POINTER(2); + int nkeys = PG_GETARG_INT32(3); + Oid colloid = PG_GET_COLLATION(); + int keyno; + bool regular_keys = false; - /* Handle IS NULL/IS NOT NULL tests. */ - if (key->sk_flags & SK_ISNULL) + /* + * 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++) { - if (key->sk_flags & SK_SEARCHNULL) + 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 (column->bv_allnulls || column->bv_hasnulls) - PG_RETURN_BOOL(true); - PG_RETURN_BOOL(false); - } + if (key->sk_flags & SK_SEARCHNULL) + { + if (column->bv_allnulls || column->bv_hasnulls) + continue; /* this key is fine, continue */ - /* - * For IS NOT NULL, we can only skip ranges that are known to have - * only nulls. - */ - if (key->sk_flags & SK_SEARCHNOTNULL) - PG_RETURN_BOOL(!column->bv_allnulls); + PG_RETURN_BOOL(false); + } - /* - * Neither IS NULL nor IS NOT NULL was used; assume all indexable - * operators are strict and return false. - */ - 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 it is all nulls, it cannot possibly be consistent. */ - if (column->bv_allnulls) + /* + * 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])) PG_RETURN_BOOL(true); - attno = key->sk_attno; - subtype = key->sk_subtype; - query = key->sk_argument; - unionval = column->bv_values[INCLUSION_UNION]; + /* 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; + + /* + * When there are multiple scan keys, failure to meet the + * criteria for a single one of them is enough to discard + * the range as a whole, so break out of the loop as soon + * as a false return value is obtained. + */ + if (!inclusion_consistent_key(bdesc, column, key, colloid)) + PG_RETURN_BOOL(false); + } + + PG_RETURN_BOOL(true); +} + +/* + * inclusion_consistent_key + * Determine if the range is consistent with a single scan key. + */ +static bool +inclusion_consistent_key(BrinDesc *bdesc, BrinValues *column, ScanKey key, + Oid colloid) +{ + FmgrInfo *finfo; + AttrNumber attno = key->sk_attno; + Oid subtype = key->sk_subtype; + Datum query = key->sk_argument; + Datum unionval = column->bv_values[INCLUSION_UNION]; + Datum result; + switch (key->sk_strategy) { /* @@ -324,49 +382,49 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTOverRightStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTOverLeftStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTRightStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTOverRightStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTLeftStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTRightStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTOverLeftStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTBelowStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTOverAboveStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTOverBelowStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTAboveStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTOverAboveStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTBelowStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); case RTAboveStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTOverBelowStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); /* * Overlap and contains strategies @@ -384,7 +442,7 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, key->sk_strategy); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_DATUM(result); + return DatumGetBool(result); /* * Contained by strategies @@ -404,9 +462,9 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) RTOverlapStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); if (DatumGetBool(result)) - PG_RETURN_BOOL(true); + return true; - PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]); + return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]); /* * Adjacent strategy @@ -423,12 +481,12 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) RTOverlapStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); if (DatumGetBool(result)) - PG_RETURN_BOOL(true); + return true; finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, - RTAdjacentStrategyNumber); + RTAdjacentStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_DATUM(result); + return DatumGetBool(result); /* * Basic comparison strategies @@ -458,9 +516,9 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) RTRightStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); if (!DatumGetBool(result)) - PG_RETURN_BOOL(true); + return true; - PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]); + return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]); case RTSameStrategyNumber: case RTEqualStrategyNumber: @@ -468,30 +526,30 @@ brin_inclusion_consistent(PG_FUNCTION_ARGS) RTContainsStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); if (DatumGetBool(result)) - PG_RETURN_BOOL(true); + return true; - PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]); + return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]); case RTGreaterEqualStrategyNumber: finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTLeftStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); if (!DatumGetBool(result)) - PG_RETURN_BOOL(true); + return true; - PG_RETURN_DATUM(column->bv_values[INCLUSION_CONTAINS_EMPTY]); + return DatumGetBool(column->bv_values[INCLUSION_CONTAINS_EMPTY]); case RTGreaterStrategyNumber: /* no need to check for empty elements */ finfo = inclusion_get_strategy_procinfo(bdesc, attno, subtype, RTLeftStrategyNumber); result = FunctionCall2Coll(finfo, colloid, unionval, query); - PG_RETURN_BOOL(!DatumGetBool(result)); + return !DatumGetBool(result); default: /* shouldn't happen */ elog(ERROR, "invalid strategy number %d", key->sk_strategy); - PG_RETURN_BOOL(false); + return false; } } diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c index 2ffbd9bf0d..12878ff3a0 100644 --- a/src/backend/access/brin/brin_minmax.c +++ b/src/backend/access/brin/brin_minmax.c @@ -30,6 +30,8 @@ typedef struct MinmaxOpaque static FmgrInfo *minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype, uint16 strategynum); +static bool minmax_consistent_key(BrinDesc *bdesc, BrinValues *column, + ScanKey key, Oid colloid); Datum @@ -146,47 +148,104 @@ brin_minmax_consistent(PG_FUNCTION_ARGS) { BrinDesc *bdesc = (BrinDesc *) PG_GETARG_POINTER(0); BrinValues *column = (BrinValues *) PG_GETARG_POINTER(1); - ScanKey key = (ScanKey) PG_GETARG_POINTER(2); - Oid colloid = PG_GET_COLLATION(), - subtype; - AttrNumber attno; - Datum value; - Datum matches; - FmgrInfo *finfo; - - Assert(key->sk_attno == column->bv_attno); + ScanKey *keys = (ScanKey *) PG_GETARG_POINTER(2); + int nkeys = PG_GETARG_INT32(3); + Oid colloid = PG_GET_COLLATION(); + int keyno; + bool regular_keys = false; - /* handle IS NULL/IS NOT NULL tests */ - if (key->sk_flags & SK_ISNULL) + /* + * 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++) { - if (key->sk_flags & SK_SEARCHNULL) + 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 (column->bv_allnulls || column->bv_hasnulls) - PG_RETURN_BOOL(true); + 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; + } - /* - * For IS NOT NULL, we can only skip ranges that are known to have - * only nulls. - */ - if (key->sk_flags & SK_SEARCHNOTNULL) - PG_RETURN_BOOL(!column->bv_allnulls); + /* + * 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); - /* - * Neither IS NULL nor IS NOT NULL was used; assume all indexable - * operators are strict and return false. + /* 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; + + /* + * When there are multiple scan keys, failure to meet the + * criteria for a single one of them is enough to discard + * the range as a whole, so break out of the loop as soon + * as a false return value is obtained. */ - PG_RETURN_BOOL(false); + if (!minmax_consistent_key(bdesc, column, key, colloid)) + PG_RETURN_DATUM(false);; } - /* if the range is all empty, it cannot possibly be consistent */ - if (column->bv_allnulls) - PG_RETURN_BOOL(false); + PG_RETURN_DATUM(true); +} + +/* + * minmax_consistent_key + * Determine if the range is consistent with a single scan key. + */ +static bool +minmax_consistent_key(BrinDesc *bdesc, BrinValues *column, ScanKey key, + Oid colloid) +{ + FmgrInfo *finfo; + AttrNumber attno = key->sk_attno; + Oid subtype = key->sk_subtype; + Datum value = key->sk_argument; + Datum matches; - attno = key->sk_attno; - subtype = key->sk_subtype; - value = key->sk_argument; switch (key->sk_strategy) { case BTLessStrategyNumber: @@ -229,7 +288,7 @@ brin_minmax_consistent(PG_FUNCTION_ARGS) break; } - PG_RETURN_DATUM(matches); + return DatumGetBool(matches); } /* diff --git a/src/backend/access/brin/brin_validate.c b/src/backend/access/brin/brin_validate.c index 6d4253c05e..11835d85cd 100644 --- a/src/backend/access/brin/brin_validate.c +++ b/src/backend/access/brin/brin_validate.c @@ -97,8 +97,8 @@ brinvalidate(Oid opclassoid) break; case BRIN_PROCNUM_CONSISTENT: ok = check_amproc_signature(procform->amproc, BOOLOID, true, - 3, 3, INTERNALOID, INTERNALOID, - INTERNALOID); + 3, 4, INTERNALOID, INTERNALOID, + INTERNALOID, INT4OID); break; case BRIN_PROCNUM_UNION: ok = check_amproc_signature(procform->amproc, BOOLOID, true, diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index d27336adcd..f3ead7d53d 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -8145,7 +8145,7 @@ prosrc => 'brin_minmax_add_value' }, { oid => '3385', descr => 'BRIN minmax support', proname => 'brin_minmax_consistent', prorettype => 'bool', - proargtypes => 'internal internal internal', + proargtypes => 'internal internal internal int4', prosrc => 'brin_minmax_consistent' }, { oid => '3386', descr => 'BRIN minmax support', proname => 'brin_minmax_union', prorettype => 'bool', @@ -8161,7 +8161,7 @@ prosrc => 'brin_inclusion_add_value' }, { oid => '4107', descr => 'BRIN inclusion support', proname => 'brin_inclusion_consistent', prorettype => 'bool', - proargtypes => 'internal internal internal', + proargtypes => 'internal internal internal int4', prosrc => 'brin_inclusion_consistent' }, { oid => '4108', descr => 'BRIN inclusion support', proname => 'brin_inclusion_union', prorettype => 'bool', -- 2.26.2 --------------310A2AE1CC4C2E2E77559E3D Content-Type: text/x-patch; charset=UTF-8; name="0002-Move-IS-NOT-NULL-handling-from-BRIN-suppo-20210114-2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0002-Move-IS-NOT-NULL-handling-from-BRIN-suppo-20210114-2.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Add TOAST support for more system tables @ 2023-07-17 22:13 Sofia Kopikova <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Sofia Kopikova @ 2023-07-17 22:13 UTC (permalink / raw) To: pgsql-hackers Hi, hackers! I've tried sending this patch to community before, let me try it second time. Patch is revised and improved compared to previous version. This patch adds TOAST support for system tables pg_class, pg_attribute and pg_largeobject_metadata, as they include ACL columns, which may be potentially large in size. Patch fixes possible pg_upgrade bug (problem with seeing a non-empty new cluster). During code developing it turned out that heap_inplace_update function is not suitable for use with TOAST, so its work could lead to wrong statistics update (for example, during VACUUM). This problem is fixed by adding new heap_inplace_update_prepare_tuple function -- we assume TOASTed attributes are never changed by in-place update, and just replace them with old values. I also added pg_catalog_toast1 test that does check for "invalid tupple length" error when creating index with toasted pg_class. Test grants and drops roles on certain table many times to make ACL column long and then creates index on this table. I wonder what other bugs can happen there, but if anyone can give me a hint, I'll try to fix them. Anyway, in PostgresPro we didn't encounter any problems with this feature. First attempt here: https://www.postgresql.org/message-id/[email protected] This time I'll do it better -- Sofia Kopikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company Attachments: [text/x-patch] 0001-Add-TOAST-support-for-several-system-tables.patch (13.2K, ../../[email protected]/2-0001-Add-TOAST-support-for-several-system-tables.patch) download | inline diff: From 6812e6cd47c03e22f953732a24223411d80d6c95 Mon Sep 17 00:00:00 2001 From: Sofia Kopikova <[email protected]> Date: Mon, 17 Jul 2023 11:56:25 +0300 Subject: [PATCH] Add TOAST support for several system tables ACL lists may have large size. Using TOAST for system tables pg_class, pg_attribute and pg_largeobject_metadata with aclitem[] columns. In 96cdeae07f9 some system catalogs were excluded from adding TOATS to them due to several possible bugs. Here bug with pg_upgrade seeing a non-empty new cluster is fixed. A workaround is added to heap_inplace_update function for its correct work with TOAST. Also test for "invalig tupple length" error case when creating index with toasted pg_class is added. --- src/backend/access/heap/heapam.c | 64 +++++++++++++++++-- src/backend/catalog/catalog.c | 2 + src/bin/pg_upgrade/check.c | 3 +- src/include/catalog/pg_attribute.h | 2 + src/include/catalog/pg_class.h | 2 + src/include/catalog/pg_largeobject_metadata.h | 2 + src/test/regress/expected/misc_sanity.out | 30 ++++----- .../regress/expected/pg_catalog_toast1.out | 25 ++++++++ src/test/regress/parallel_schedule | 3 + src/test/regress/sql/misc_sanity.sql | 10 +-- src/test/regress/sql/pg_catalog_toast1.sql | 20 ++++++ 11 files changed, 134 insertions(+), 29 deletions(-) create mode 100644 src/test/regress/expected/pg_catalog_toast1.out create mode 100644 src/test/regress/sql/pg_catalog_toast1.sql diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 7ed72abe597..e043a8e4401 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -5860,6 +5860,53 @@ heap_abort_speculative(Relation relation, ItemPointer tid) pgstat_count_heap_delete(relation); } +/* + * Prepare tuple for inplace update. TOASTed attributes can't be modified + * by in-place upgrade. Simultaneously, new tuple is flatten. So, we just + * replace TOASTed attributes with their values of old tuple. + */ +static HeapTuple +heap_inplace_update_prepare_tuple(Relation relation, + HeapTuple oldtup, + HeapTuple newtup) +{ + TupleDesc desc = relation->rd_att; + HeapTuple result; + Datum *oldvals, + *newvals; + bool *oldnulls, + *newnulls; + int i, + natts = desc->natts; + + oldvals = (Datum *) palloc(sizeof(Datum) * natts); + newvals = (Datum *) palloc(sizeof(Datum) * natts); + oldnulls = (bool *) palloc(sizeof(bool) * natts); + newnulls = (bool *) palloc(sizeof(bool) * natts); + + heap_deform_tuple(oldtup, desc, oldvals, oldnulls); + heap_deform_tuple(newtup, desc, newvals, newnulls); + + for (i = 0; i < natts; i++) + { + Form_pg_attribute att = &desc->attrs[i]; + + if (att->attlen == -1 && + !oldnulls[i] && + VARATT_IS_EXTENDED(oldvals[i])) + { + Assert(!newnulls[i]); + newvals[i] = oldvals[i]; + } + } + + result = heap_form_tuple(desc, newvals, newnulls); + + result->t_self = newtup->t_self; + + return result; +} + /* * heap_inplace_update - update a tuple "in place" (ie, overwrite it) * @@ -5889,6 +5936,8 @@ heap_inplace_update(Relation relation, HeapTuple tuple) HeapTupleHeader htup; uint32 oldlen; uint32 newlen; + HeapTupleData oldtup; + HeapTuple newtup; /* * For now, we don't allow parallel updates. Unlike a regular update, @@ -5914,16 +5963,23 @@ heap_inplace_update(Relation relation, HeapTuple tuple) htup = (HeapTupleHeader) PageGetItem(page, lp); + oldtup.t_tableOid = RelationGetRelid(relation); + oldtup.t_data = htup; + oldtup.t_len = ItemIdGetLength(lp); + oldtup.t_self = tuple->t_self; + + newtup = heap_inplace_update_prepare_tuple(relation, &oldtup, tuple); + oldlen = ItemIdGetLength(lp) - htup->t_hoff; - newlen = tuple->t_len - tuple->t_data->t_hoff; - if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff) + newlen = newtup->t_len - newtup->t_data->t_hoff; + if (oldlen != newlen || htup->t_hoff != newtup->t_data->t_hoff) elog(ERROR, "wrong tuple length"); /* NO EREPORT(ERROR) from here till changes are logged */ START_CRIT_SECTION(); memcpy((char *) htup + htup->t_hoff, - (char *) tuple->t_data + tuple->t_data->t_hoff, + (char *) newtup->t_data + newtup->t_data->t_hoff, newlen); MarkBufferDirty(buffer); @@ -5934,7 +5990,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple) xl_heap_inplace xlrec; XLogRecPtr recptr; - xlrec.offnum = ItemPointerGetOffsetNumber(&tuple->t_self); + xlrec.offnum = ItemPointerGetOffsetNumber(&newtup->t_self); XLogBeginInsert(); XLogRegisterData((char *) &xlrec, SizeOfHeapInplace); diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 1bf6c5633cd..2f463f9216d 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -297,6 +297,8 @@ IsSharedRelation(Oid relationId) relationId == PgShseclabelToastIndex || relationId == PgSubscriptionToastTable || relationId == PgSubscriptionToastIndex || + relationId == PgDatabaseToastTable || + relationId == PgDatabaseToastIndex || relationId == PgTablespaceToastTable || relationId == PgTablespaceToastIndex) return true; diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 64024e3b9ec..e9994367995 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -358,7 +358,8 @@ check_new_cluster_is_empty(void) relnum++) { /* pg_largeobject and its index should be skipped */ - if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0) + if (strcmp(rel_arr->rels[relnum].nspname, "pg_catalog") != 0 && + strcmp(rel_arr->rels[relnum].nspname, "pg_toast") != 0) pg_fatal("New cluster database \"%s\" is not empty: found relation \"%s.%s\"", new_cluster.dbarr.dbs[dbnum].db_name, rel_arr->rels[relnum].nspname, diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h index f00df488ce7..e932cb82836 100644 --- a/src/include/catalog/pg_attribute.h +++ b/src/include/catalog/pg_attribute.h @@ -208,6 +208,8 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75, */ typedef FormData_pg_attribute *Form_pg_attribute; +DECLARE_TOAST(pg_attribute, 9001, 9002); + DECLARE_UNIQUE_INDEX(pg_attribute_relid_attnam_index, 2658, AttributeRelidNameIndexId, on pg_attribute using btree(attrelid oid_ops, attname name_ops)); DECLARE_UNIQUE_INDEX_PKEY(pg_attribute_relid_attnum_index, 2659, AttributeRelidNumIndexId, on pg_attribute using btree(attrelid oid_ops, attnum int2_ops)); diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 2d1bb7af3a9..9bdb058fe78 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -152,6 +152,8 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat */ typedef FormData_pg_class *Form_pg_class; +DECLARE_TOAST(pg_class, 9003, 9004); + DECLARE_UNIQUE_INDEX_PKEY(pg_class_oid_index, 2662, ClassOidIndexId, on pg_class using btree(oid oid_ops)); DECLARE_UNIQUE_INDEX(pg_class_relname_nsp_index, 2663, ClassNameNspIndexId, on pg_class using btree(relname name_ops, relnamespace oid_ops)); DECLARE_INDEX(pg_class_tblspc_relfilenode_index, 3455, ClassTblspcRelfilenodeIndexId, on pg_class using btree(reltablespace oid_ops, relfilenode oid_ops)); diff --git a/src/include/catalog/pg_largeobject_metadata.h b/src/include/catalog/pg_largeobject_metadata.h index 80db926079f..c0b99f5b295 100644 --- a/src/include/catalog/pg_largeobject_metadata.h +++ b/src/include/catalog/pg_largeobject_metadata.h @@ -46,6 +46,8 @@ CATALOG(pg_largeobject_metadata,2995,LargeObjectMetadataRelationId) */ typedef FormData_pg_largeobject_metadata *Form_pg_largeobject_metadata; +DECLARE_TOAST(pg_largeobject_metadata, 9040, 9041); + DECLARE_UNIQUE_INDEX_PKEY(pg_largeobject_metadata_oid_index, 2996, LargeObjectMetadataOidIndexId, on pg_largeobject_metadata using btree(oid oid_ops)); #endif /* PG_LARGEOBJECT_METADATA_H */ diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out index a57fd142a94..2275c48ad3a 100644 --- a/src/test/regress/expected/misc_sanity.out +++ b/src/test/regress/expected/misc_sanity.out @@ -35,11 +35,11 @@ WHERE refclassid = 0 OR refobjid = 0 OR -- Look for system tables with varlena columns but no toast table. All -- system tables with toastable columns should have toast tables, with -- the following exceptions: --- 1. pg_class, pg_attribute, and pg_index, due to fear of recursive --- dependencies as toast tables depend on them. --- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs --- and toast tables are mutually exclusive and large object data is handled --- as user data by pg_upgrade, which would cause failures. +-- 1. pg_index. +-- 2. pg_largeobject. +-- These and some other tables were excluded in PostgreSQL for various reasons, +-- but in Postgres Pro Enterprise we added toast tables for system tables with +-- ACL columns. SELECT relname, attname, atttypid::regtype FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid WHERE c.oid < 16384 AND @@ -47,20 +47,12 @@ WHERE c.oid < 16384 AND relkind = 'r' AND attstorage != 'p' ORDER BY 1, 2; - relname | attname | atttypid --------------------------+---------------+-------------- - pg_attribute | attacl | aclitem[] - pg_attribute | attfdwoptions | text[] - pg_attribute | attmissingval | anyarray - pg_attribute | attoptions | text[] - pg_class | relacl | aclitem[] - pg_class | reloptions | text[] - pg_class | relpartbound | pg_node_tree - pg_index | indexprs | pg_node_tree - pg_index | indpred | pg_node_tree - pg_largeobject | data | bytea - pg_largeobject_metadata | lomacl | aclitem[] -(11 rows) + relname | attname | atttypid +----------------+----------+-------------- + pg_index | indexprs | pg_node_tree + pg_index | indpred | pg_node_tree + pg_largeobject | data | bytea +(3 rows) -- system catalogs without primary keys -- diff --git a/src/test/regress/expected/pg_catalog_toast1.out b/src/test/regress/expected/pg_catalog_toast1.out new file mode 100644 index 00000000000..1918351fede --- /dev/null +++ b/src/test/regress/expected/pg_catalog_toast1.out @@ -0,0 +1,25 @@ +CREATE OR REPLACE FUNCTION toast_acl() +RETURNS BOOLEAN AS $$ +DECLARE + I INTEGER :=0; +BEGIN + SET LOCAL CLIENT_MIN_MESSAGES=WARNING; + EXECUTE 'DROP TABLE IF EXISTS test2510'; + EXECUTE 'CREATE TABLE test2510 (a INT)'; + FOR I IN 1..4096 LOOP + EXECUTE 'DROP ROLE IF EXISTS role' || I; + EXECUTE 'CREATE ROLE role' || I; + EXECUTE 'GRANT ALL ON test2510 TO role' || I; + END LOOP; + RESET CLIENT_MIN_MESSAGES; + RETURN TRUE; +END; +$$ +LANGUAGE PLPGSQL; +SELECT toast_acl(); + toast_acl +----------- + t +(1 row) + +CREATE INDEX ON test2510 USING BTREE(a); diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index 4df9d8503b9..06620d79ec8 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -132,3 +132,6 @@ test: fast_default # run tablespace test at the end because it drops the tablespace created during # setup that other tests may use. test: tablespace + +#check for "invalig tupple length" error when creating index with toasted pg_class +test: pg_catalog_toast1 diff --git a/src/test/regress/sql/misc_sanity.sql b/src/test/regress/sql/misc_sanity.sql index 2c0f87a651f..658f17fef99 100644 --- a/src/test/regress/sql/misc_sanity.sql +++ b/src/test/regress/sql/misc_sanity.sql @@ -38,11 +38,11 @@ WHERE refclassid = 0 OR refobjid = 0 OR -- Look for system tables with varlena columns but no toast table. All -- system tables with toastable columns should have toast tables, with -- the following exceptions: --- 1. pg_class, pg_attribute, and pg_index, due to fear of recursive --- dependencies as toast tables depend on them. --- 2. pg_largeobject and pg_largeobject_metadata. Large object catalogs --- and toast tables are mutually exclusive and large object data is handled --- as user data by pg_upgrade, which would cause failures. +-- 1. pg_index. +-- 2. pg_largeobject. +-- These and some other tables were excluded in PostgreSQL for various reasons, +-- but in Postgres Pro Enterprise we added toast tables for system tables with +-- ACL columns. SELECT relname, attname, atttypid::regtype FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid diff --git a/src/test/regress/sql/pg_catalog_toast1.sql b/src/test/regress/sql/pg_catalog_toast1.sql new file mode 100644 index 00000000000..d33122214fe --- /dev/null +++ b/src/test/regress/sql/pg_catalog_toast1.sql @@ -0,0 +1,20 @@ +CREATE OR REPLACE FUNCTION toast_acl() +RETURNS BOOLEAN AS $$ +DECLARE + I INTEGER :=0; +BEGIN + SET LOCAL CLIENT_MIN_MESSAGES=WARNING; + EXECUTE 'DROP TABLE IF EXISTS test2510'; + EXECUTE 'CREATE TABLE test2510 (a INT)'; + FOR I IN 1..4096 LOOP + EXECUTE 'DROP ROLE IF EXISTS role' || I; + EXECUTE 'CREATE ROLE role' || I; + EXECUTE 'GRANT ALL ON test2510 TO role' || I; + END LOOP; + RESET CLIENT_MIN_MESSAGES; + RETURN TRUE; +END; +$$ +LANGUAGE PLPGSQL; +SELECT toast_acl(); +CREATE INDEX ON test2510 USING BTREE(a); -- 2.20.1 ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-07-17 22:13 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 1/6] Pass all scan keys to BRIN consistent function at once Tomas Vondra <[email protected]> 2023-07-17 22:13 Add TOAST support for more system tables Sofia Kopikova <[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