agora inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Fix initial logical decoding snapshat race condition. 4+ messages / 4 participants [nested] [flat]
* [PATCH] Fix initial logical decoding snapshat race condition. @ 2017-05-01 01:24 Andres Freund <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Andres Freund @ 2017-05-01 01:24 UTC (permalink / raw) LogStandbySnapshot() has no interlock preventing RecordTransactionCommit() from logging its commit record (abort situation is similar), before the xl_running_xacts record is generated. That can lead to the situation that logical decoding forever waits for a commit/abort record, that was logged in the past. To fix, check for such "missed" transactions when xl_running_xacts are observed, while in SNAPBUILD_FULL_SNAPSHOT. This also reverts changes made to GetRunningTransactionData() and LogStandbySnapshot() by b89e151 as the additional locking does not solve the problem, and is not required anymore. Author: Petr Jelined and Andres Freund Reviewed-By: Andres Freund Discussion: https://postgr.es/m/[email protected] Backpatch: 9.4, where logical decoding was introduced --- src/backend/replication/logical/snapbuild.c | 98 +++++++++++++++++++++++------ src/backend/storage/ipc/procarray.c | 5 +- src/backend/storage/ipc/standby.c | 19 ------ 3 files changed, 81 insertions(+), 41 deletions(-) diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 068d214fa1..8605625555 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -203,6 +203,7 @@ struct SnapBuild size_t xcnt; /* number of used xip entries */ size_t xcnt_space; /* allocated size of xip */ TransactionId *xip; /* running xacts array, xidComparator-sorted */ + bool *xip_running; /* xid in ->xip still running? */ } running; /* @@ -253,7 +254,7 @@ static bool ExportInProgress = false; static void SnapBuildEndTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid); /* ->running manipulation */ -static bool SnapBuildTxnIsRunning(SnapBuild *builder, TransactionId xid); +static bool SnapBuildTxnIsRunning(SnapBuild *builder, TransactionId xid, int *off); /* ->committed manipulation */ static void SnapBuildPurgeCommittedTxn(SnapBuild *builder); @@ -700,7 +701,7 @@ SnapBuildProcessChange(SnapBuild *builder, TransactionId xid, XLogRecPtr lsn) * we got into the SNAPBUILD_FULL_SNAPSHOT state. */ if (builder->state < SNAPBUILD_CONSISTENT && - SnapBuildTxnIsRunning(builder, xid)) + SnapBuildTxnIsRunning(builder, xid, NULL)) return false; /* @@ -776,7 +777,7 @@ SnapBuildProcessNewCid(SnapBuild *builder, TransactionId xid, * only exist after we freshly started from an < CONSISTENT snapshot. */ static bool -SnapBuildTxnIsRunning(SnapBuild *builder, TransactionId xid) +SnapBuildTxnIsRunning(SnapBuild *builder, TransactionId xid, int *off) { Assert(builder->state < SNAPBUILD_CONSISTENT); Assert(TransactionIdIsNormal(builder->running.xmin)); @@ -793,8 +794,17 @@ SnapBuildTxnIsRunning(SnapBuild *builder, TransactionId xid) if (search != NULL) { Assert(*search == xid); + + if (off) + { + *off = search - builder->running.xip; + Assert(builder->running.xip[*off] == xid); + } + return true; } + + } return false; @@ -928,6 +938,8 @@ SnapBuildPurgeCommittedTxn(SnapBuild *builder) static void SnapBuildEndTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid) { + int off; + if (builder->state == SNAPBUILD_CONSISTENT) return; @@ -938,10 +950,13 @@ SnapBuildEndTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid) * subxids and since they end at the same time it's sufficient to deal * with them here. */ - if (SnapBuildTxnIsRunning(builder, xid)) + if (SnapBuildTxnIsRunning(builder, xid, &off)) { Assert(builder->running.xcnt > 0); + Assert(builder->running.xip_running[off]); + builder->running.xip_running[off] = false; + if (!--builder->running.xcnt) { /* @@ -1250,9 +1265,15 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn * * a) There were no running transactions when the xl_running_xacts record * was inserted, jump to CONSISTENT immediately. We might find such a - * state we were waiting for b) or c). + * state while waiting for b) or c). * - * b) Wait for all toplevel transactions that were running to end. We + * b) This (in a previous run) or another decoding slot serialized a + * snapshot to disk that we can use. Can't use this method for the + * initial snapshot when slot is being created and needs full snapshot + * for export or direct use, as that snapshot will only contain catalog + * modifying transactions. + * + * c) Wait for all toplevel transactions that were running to end. We * simply track the number of in-progress toplevel transactions and * lower it whenever one commits or aborts. When that number * (builder->running.xcnt) reaches zero, we can go from FULL_SNAPSHOT @@ -1264,11 +1285,13 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn * subtransactions - and by extension suboverflowed xl_running_xacts - * at all. * - * c) This (in a previous run) or another decoding slot serialized a - * snapshot to disk that we can use. Can't use this method for the - * initial snapshot when slot is being created and needs full snapshot - * for export or direct use, as that snapshot will only contain catalog - * modifying transactions. + * Unfortunately there's a race condition around LogStandbySnapshot(), + * where transactions might have logged their commit record, before + * xl_running_xacts itself is logged. In that case the decoding logic + * would have missed that fact. Thus + * + * d) xl_running_xacts shows us that transaction(s) assumed to be still + * running have actually already finished. Adjust their status. * --- */ @@ -1323,16 +1346,15 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn return false; } - /* c) valid on disk state and not building full snapshot */ + /* b) valid on disk state and not building full snapshot */ else if (!builder->building_full_snapshot && SnapBuildRestore(builder, lsn)) { /* there won't be any state to cleanup */ return false; } - /* - * b) first encounter of a useable xl_running_xacts record. If we had + * c) first encounter of a useable xl_running_xacts record. If we had * found one earlier we would either track running transactions (i.e. * builder->running.xcnt != 0) or be consistent (this function wouldn't * get called). @@ -1367,6 +1389,11 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn builder->running.xcnt * sizeof(TransactionId)); memcpy(builder->running.xip, running->xids, builder->running.xcnt * sizeof(TransactionId)); + builder->running.xip_running = + MemoryContextAlloc(builder->context, + builder->running.xcnt * sizeof(bool)); + memset(builder->running.xip_running, 1, + builder->running.xcnt * sizeof(bool)); /* sort so we can do a binary search */ qsort(builder->running.xip, builder->running.xcnt, @@ -1414,13 +1441,44 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn /* nothing could have built up so far, so don't perform cleanup */ return false; } + /* d) already tracking running xids, check whether any were missed */ + else + { + size_t xcnt = running->xcnt; + TransactionId *xip = + MemoryContextAlloc(builder->context, xcnt * sizeof(TransactionId)); + int i; - /* - * We already started to track running xacts and need to wait for all - * in-progress ones to finish. We fall through to the normal processing of - * records so incremental cleanup can be performed. - */ - return true; + memcpy(xip, running->xids, xcnt * sizeof(TransactionId)); + + /* sort so we can do a binary search */ + qsort(xip, xcnt, sizeof(TransactionId), xidComparator); + + /* + * Mark all transactions as finished that we assumed were running, but + * actually aren't according to the xl_running_xacts record. + */ + for (i = 0; i < builder->running.xcnt_space; i++) + { + TransactionId still_running = builder->running.xip[i]; + void *test; + + if (!builder->running.xip_running[i]) + continue; + + test = bsearch(&still_running, xip, xcnt, + sizeof(TransactionId), xidComparator); + if (!test) + SnapBuildEndTxn(builder, lsn, still_running); + } + + /* + * We already started to track running xacts and need to wait for all + * in-progress ones to finish. We fall through to the normal processing of + * records so incremental cleanup can be performed. + */ + return true; + } } diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 8a71536791..de3ae92dd7 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -2060,12 +2060,13 @@ GetRunningTransactionData(void) CurrentRunningXacts->oldestRunningXid = oldestRunningXid; CurrentRunningXacts->latestCompletedXid = latestCompletedXid; + /* We don't release XidGenLock here, the caller is responsible for that */ + LWLockRelease(ProcArrayLock); + Assert(TransactionIdIsValid(CurrentRunningXacts->nextXid)); Assert(TransactionIdIsValid(CurrentRunningXacts->oldestRunningXid)); Assert(TransactionIdIsNormal(CurrentRunningXacts->latestCompletedXid)); - /* We don't release the locks here, the caller is responsible for that */ - return CurrentRunningXacts; } diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index 8e57f933ca..ddb279e274 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -929,27 +929,8 @@ LogStandbySnapshot(void) */ running = GetRunningTransactionData(); - /* - * GetRunningTransactionData() acquired ProcArrayLock, we must release it. - * For Hot Standby this can be done before inserting the WAL record - * because ProcArrayApplyRecoveryInfo() rechecks the commit status using - * the clog. For logical decoding, though, the lock can't be released - * early because the clog might be "in the future" from the POV of the - * historic snapshot. This would allow for situations where we're waiting - * for the end of a transaction listed in the xl_running_xacts record - * which, according to the WAL, has committed before the xl_running_xacts - * record. Fortunately this routine isn't executed frequently, and it's - * only a shared lock. - */ - if (wal_level < WAL_LEVEL_LOGICAL) - LWLockRelease(ProcArrayLock); - recptr = LogCurrentRunningXacts(running); - /* Release lock if we kept it longer ... */ - if (wal_level >= WAL_LEVEL_LOGICAL) - LWLockRelease(ProcArrayLock); - /* GetRunningTransactionData() acquired XidGenLock, we must release it */ LWLockRelease(XidGenLock); -- 2.12.0.264.gd6db3f2165.dirty --ptyjiezm3fnjgfig Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers --ptyjiezm3fnjgfig-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* [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; 4+ 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 58fe109d2d..8a1a9da78f 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -389,6 +389,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; @@ -410,6 +413,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); @@ -470,7 +528,7 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) } else { - int keyno; + int attno; /* * Compare scan keys with summary values stored for the range. @@ -480,51 +538,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 d7b55f57ea..871e024e00 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -8143,7 +8143,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', @@ -8159,7 +8159,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 --------------D21F32E0F9E7408E9BE1AF2E Content-Type: text/x-patch; charset=UTF-8; name="0002-Move-IS-NOT-NULL-handling-from-BRIN-support-20210114.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0002-Move-IS-NOT-NULL-handling-from-BRIN-support-20210114.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pgsql: Add support for building with ZSTD. @ 2022-02-19 06:18 Michael Paquier <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michael Paquier @ 2022-02-19 06:18 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Postgres hackers <[email protected]> Hi, On Fri, Feb 18, 2022 at 06:53:10PM +0000, Robert Haas wrote: > Add support for building with ZSTD. > > This commit doesn't actually add anything that uses ZSTD; that will be > done separately. It just puts the basic infrastructure into place. > > Jeevan Ladhe, Robert Haas, and Michael Paquier. Reviewed by Justin > Pryzby and Andres Freund. I completely forgot that the section of the docs dedicated to the TAP tests with MSVC also needs a refresh to mention the new variable ZSTD, as of the attached. Do you mind if I apply that? Thanks, -- Michael Attachments: [text/x-diff] doc-zstd-msvc.patch (686B, ../../YhCL0fKnDv%[email protected]/2-doc-zstd-msvc.patch) download | inline diff: diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml index 98fa6962f6..91b1410e4a 100644 --- a/doc/src/sgml/install-windows.sgml +++ b/doc/src/sgml/install-windows.sgml @@ -557,6 +557,15 @@ $ENV{PROVE_TESTS}='t/020*.pl t/010*.pl' <varname>PATH</varname>. </para></listitem> </varlistentry> + + <varlistentry> + <term><varname>ZSTD</varname></term> + <listitem><para> + Path to a <application>zstd</application> command. The default is + <literal>zstd</literal>, that would be the command found in + <varname>PATH</varname>. + </para></listitem> + </varlistentry> </variablelist> </para> </sect2> [application/pgp-signature] signature.asc (833B, ../../YhCL0fKnDv%[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pgsql: Add support for building with ZSTD. @ 2022-02-20 14:45 Robert Haas <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Robert Haas @ 2022-02-20 14:45 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]> On Sat, Feb 19, 2022 at 1:19 AM Michael Paquier <[email protected]> wrote: > On Fri, Feb 18, 2022 at 06:53:10PM +0000, Robert Haas wrote: > > Add support for building with ZSTD. > > > > This commit doesn't actually add anything that uses ZSTD; that will be > > done separately. It just puts the basic infrastructure into place. > > > > Jeevan Ladhe, Robert Haas, and Michael Paquier. Reviewed by Justin > > Pryzby and Andres Freund. > > I completely forgot that the section of the docs dedicated to the TAP > tests with MSVC also needs a refresh to mention the new variable ZSTD, > as of the attached. Do you mind if I apply that? No issues at all with you adjusting this, but I think that sentence reads a little awkwardly. Perhaps instead of "The default is <literal>zstd</literal>, that would be the command found in <varname>PATH</varname>." you could write something like "The default is <literal>zstd</literal>, which will search for a command by that name in the configured <varname>PATH</varname>." Or maybe something else is better, not sure exactly, your version just seems a little odd to me. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-02-20 14:45 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2017-05-01 01:24 [PATCH] Fix initial logical decoding snapshat race condition. Andres Freund <[email protected]> 2020-09-12 13:07 [PATCH 1/6] Pass all scan keys to BRIN consistent function at once Tomas Vondra <[email protected]> 2022-02-19 06:18 Re: pgsql: Add support for building with ZSTD. Michael Paquier <[email protected]> 2022-02-20 14:45 ` Re: pgsql: Add support for building with ZSTD. Robert Haas <[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