public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/8] Pass all scan keys to BRIN consistent function at once 3+ messages / 3 participants [nested] [flat]
* [PATCH 2/8] Pass all scan keys to BRIN consistent function at once @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 3+ 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. 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 | 158 +++++++++++++++++++----- src/backend/access/brin/brin_validate.c | 4 +- 2 files changed, 126 insertions(+), 36 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 27ba596c6e..f9a0476024 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,66 @@ 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. We allocate space for all attributes, so + * that we don't have to bother determining which attributes are used. + * + * XXX The widest table can have ~1600 attributes, so this may allocate a + * couple kilobytes of memory). We could invent a more compact approach + * (with just space for used attributes) but that would make the matching + * more complicated, so it may not be a win. + */ + keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); + 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 +534,7 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) } else { - int keyno; + int attno; /* * Compare scan keys with summary values stored for the range. @@ -481,51 +544,78 @@ 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 scan 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, as + * before. */ - 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); + + /* mismatching key, no need to look further */ + if (!addrange) + break; + } + } + if (!addrange) break; } 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, -- 2.26.2 --------------556A1DC61262AF15641DB204 Content-Type: text/x-patch; charset=UTF-8; name="0003-Process-all-scan-keys-in-existing-BRIN-opc-20210308b.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0003-Process-all-scan-keys-in-existing-BRIN-opc-20210308b.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-20 11:06 Robert Haas <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Robert Haas @ 2024-02-20 11:06 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Ian Lawrence Barwick <[email protected]>; pgsql-hackers On Tue, Feb 20, 2024 at 4:18 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > I found an inconsistency. When I ran ` pg_basebackup -D data_N2 -U postgres -R`, > dbname would be set as username. > > ``` > primary_conninfo = 'user=postgres ... dbname=postgres > ``` > > However, when I ran `pg_basebackup -D data_N2 -d "user=postgres" -R`, > dbname would be set as "replication". Is it an intentional item? > > ``` > primary_conninfo = 'user=postgres ... dbname=replication... > ``` Seems weird to me. You don't use dbname=replication to ask for a replication connection, so why would we ever end up with that anywhere? And especially in only one of two such closely related cases? -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 3+ messages in thread
* RE: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-20 12:28 Hayato Kuroda (Fujitsu) <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2024-02-20 12:28 UTC (permalink / raw) To: 'Robert Haas' <[email protected]>; +Cc: Ian Lawrence Barwick <[email protected]>; pgsql-hackers Dear Robert, > Seems weird to me. You don't use dbname=replication to ask for a > replication connection, so why would we ever end up with that > anywhere? And especially in only one of two such closely related > cases? Just FYI - here is an extreme case. And note that I have applied proposed patch. When `pg_basebackup -D data_N2 -R` is used: ``` primary_conninfo = 'user=hayato ... dbname=hayato ... ``` But when `pg_basebackup -d "" -D data_N2 -R` is used: ``` primary_conninfo = 'user=hayato ... dbname=replication ``` Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/ ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-02-20 12:28 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 2/8] Pass all scan keys to BRIN consistent function at once Tomas Vondra <[email protected]> 2024-02-20 11:06 Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Robert Haas <[email protected]> 2024-02-20 12:28 ` RE: Have pg_basebackup write "dbname" in "primary_conninfo"? Hayato Kuroda (Fujitsu) <[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