public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 5/8] Optimize allocations in bringetbitmap 4+ messages / 4 participants [nested] [flat]
* [PATCH 5/8] Optimize allocations in bringetbitmap @ 2021-03-02 18:57 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Tomas Vondra @ 2021-03-02 18:57 UTC (permalink / raw) The bringetbitmap function allocates memory for various purposes, which may be quite expensive, depending on the number of scan keys. Instead of allocating them separately, allocate one bit chunk of memory an carve it into smaller pieces as needed - all the pieces have the same lifespan, and it saves quite a bit of CPU and memory overhead. Author: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin.c | 60 ++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 9f2656b8d9..1f82e965f9 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -373,6 +373,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) int *nkeys, *nnullkeys; int keyno; + char *ptr; + Size len; + char *tmp PG_USED_FOR_ASSERTS_ONLY; opaque = (BrinOpaque *) scan->opaque; bdesc = opaque->bo_bdesc; @@ -402,15 +405,52 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * We keep null and regular keys separate, so that we can pass just the * regular keys to the consistent function easily. * + * To reduce the allocation overhead, we allocate one big chunk and then + * carve it into smaller arrays ourselves. All the pieces have exactly the + * same lifetime, so that's OK. + * * XXX The widest table can have ~1600 attributes, so this may allocate a * couple kilobytes of memory). We could invent a more compact approach * (with just space for used attributes) but that would make the matching * more complicated, so it may not be a win. */ - keys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); - nullkeys = palloc0(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); - nkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts); - nnullkeys = palloc0(sizeof(int) * bdesc->bd_tupdesc->natts); + len = + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + /* regular keys */ + MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys) * bdesc->bd_tupdesc->natts + + MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts) + + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + /* NULL keys */ + MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys) * bdesc->bd_tupdesc->natts + + MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts); + + ptr = palloc(len); + tmp = ptr; + + keys = (ScanKey **) ptr; + ptr += MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); + + nullkeys = (ScanKey **) ptr; + ptr += MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts); + + nkeys = (int *) ptr; + ptr += MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts); + + nnullkeys = (int *) ptr; + ptr += MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts); + + for (int i = 0; i < bdesc->bd_tupdesc->natts; i++) + { + keys[i] = (ScanKey *) ptr; + ptr += MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys); + + nullkeys[i] = (ScanKey *) ptr; + ptr += MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys); + } + + Assert(tmp + len == ptr); + + /* zero the number of keys */ + memset(nkeys, 0, sizeof(int) * bdesc->bd_tupdesc->natts); + memset(nnullkeys, 0, sizeof(int) * bdesc->bd_tupdesc->natts); /* * Preprocess the scan keys - split them into per-attribute arrays. @@ -444,9 +484,9 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) { FmgrInfo *tmp; - /* 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)); + /* First time we see this attribute, so no key/null keys. */ + Assert(nkeys[keyattno - 1] == 0); + Assert(nnullkeys[keyattno - 1] == 0); tmp = index_getprocinfo(idxRel, keyattno, BRIN_PROCNUM_CONSISTENT); @@ -457,17 +497,11 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) /* 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]++; } -- 2.26.2 --------------556A1DC61262AF15641DB204 Content-Type: text/x-patch; charset=UTF-8; name="0006-BRIN-bloom-indexes-20210308b.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="0006-BRIN-bloom-indexes-20210308b.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Should we document how column DEFAULT expressions work? @ 2024-06-26 05:35 David G. Johnston <[email protected]> 0 siblings, 2 replies; 4+ messages in thread From: David G. Johnston @ 2024-06-26 05:35 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Tom Lane <[email protected]>; James Coleman <[email protected]>; pgsql-hackers On Tue, Jun 25, 2024 at 9:50 PM David Rowley <[email protected]> wrote: > On Wed, 26 Jun 2024 at 13:31, David G. Johnston > <[email protected]> wrote: > > I'd suggest adding to: > > > > DEFAULT default_expr > > The DEFAULT clause assigns a default data value for the column whose > column definition it appears within. The value is any variable-free > expression (in particular, cross-references to other columns in the current > table are not allowed). Subqueries are not allowed either. The data type of > the default expression must match the data type of the column. > > > > The default expression will be used in any insert operation that does > not specify a value for the column. If there is no default for a column, > then the default is null. > > > > + Be aware that the [special timestamp values 1] are resolved > immediately, not upon insert. Use the [date/time constructor functions 2] > to produce a time relative to the future insertion. > Annoyingly even this advice isn't correct: postgres=# create table tdts2 (ts timestamptz default 'now()'); CREATE TABLE postgres=# \d tdts2 Table "public.tdts2" Column | Type | Collation | Nullable | Default --------+--------------------------+-----------+----------+------------------------------------------- ---------------- ts | timestamp with time zone | | | '2024-06-25 18:05:33.055377-07'::timestamp with time zone I expected writing what looked like the function now() to be delayed evaluated but since I put it into quotes, the OPs complaint, it got read as the literal with ignored extra bits. > FWIW, I disagree that we need to write anything about that in this > part of the documentation. I think any argument for doing this could > equally be applied to something like re-iterating what the operator > precedence rules for arithmetic are, and I don't think that should be > mentioned. I disagree on this equivalence. The time literals are clear deviations from expected behavior. Knowing operator precedence rules, they apply everywhere equally. And we should document the deviations directly where they happen. Even if it's just a short link back to the source that describes the deviation. I'm fine with something less verbose pointing only to the data types page, but not with nothing. Also, what about all the other places where someone could > use one of the special timestamp input values? Should CREATE VIEW get > a memo too? How about PREPARE? > Yes. > If people don't properly understand these special timestamp input > values, then maybe the documentation in [1] needs to be improved. Recall, and awareness, is the greater issue, not comprehension. This intends to increase the former. I don't believe the latter is an issue, though I haven't deep dived into it. And the whole type casting happening right away just seems misleading. postgres=# create table testboold2 (expr boolean default boolean 'false'); CREATE TABLE postgres=# \d testboold2 Table "public.testboold2" Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+--------- expr | boolean | | | false I would expect 'f' in the default column if the boolean casting of the literal happened sooner. Or I'd expect to see "boolean 'false'" as the default expression if it is captured as-is. So yes, saving an expression into the default column has nuances that should be documented where default is defined. Maybe the wording needs to be: "If the default expression contains any constants [1] they are converted into their typed value during create table execution. Thus time constants [1] save into the default expression the time the command was executed." [1] https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-CONSTANTS [2] https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-SPECIAL-VALUES I'd be happy to be pointed to other constants that resolve to an execution-time specific environment in a similar manner. If there is another one I'll rethink the wisdom of trying to document all of them in each place. But reminding people that time is special and we have these special values seems to provide meaningful reader benefit for the cost of a couple of sentences repeated in a few places. That were valid a decade ago no more or less than they are valid now. David J. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Should we document how column DEFAULT expressions work? @ 2024-06-26 13:36 Alvaro Herrera <[email protected]> parent: David G. Johnston <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Alvaro Herrera @ 2024-06-26 13:36 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: David Rowley <[email protected]>; Tom Lane <[email protected]>; James Coleman <[email protected]>; pgsql-hackers On 2024-Jun-25, David G. Johnston wrote: > On Tue, Jun 25, 2024 at 9:50 PM David Rowley <[email protected]> wrote: > > FWIW, I disagree that we need to write anything about that in this > > part of the documentation. I think any argument for doing this could > > equally be applied to something like re-iterating what the operator > > precedence rules for arithmetic are, and I don't think that should be > > mentioned. > > I disagree on this equivalence. The time literals are clear deviations > from expected behavior. Knowing operator precedence rules, they apply > everywhere equally. And we should document the deviations directly where > they happen. Even if it's just a short link back to the source that > describes the deviation. I'm fine with something less verbose pointing > only to the data types page, but not with nothing. I agree that it'd be good to have _something_ -- the other stance seems super unhelpful. "We're not going to spend two lines to explain some funny rules that determine surprising behavior here, because we assume you have read all of our other 3000 pages of almost impenetrably dense documentation" is not great from a user's point of view. The behavior of 'now' in DEFAULT clauses is something that has been asked about for decades. Arithmetic precedence is a terrible straw man argument. Let's put that aside. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "If it is not right, do not do it. If it is not true, do not say it." (Marcus Aurelius, Meditations) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Should we document how column DEFAULT expressions work? @ 2024-06-26 23:54 David Rowley <[email protected]> parent: David G. Johnston <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: David Rowley @ 2024-06-26 23:54 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Tom Lane <[email protected]>; James Coleman <[email protected]>; pgsql-hackers On Wed, 26 Jun 2024 at 17:36, David G. Johnston <[email protected]> wrote: > > On Tue, Jun 25, 2024 at 9:50 PM David Rowley <[email protected]> wrote: >> FWIW, I disagree that we need to write anything about that in this >> part of the documentation. I think any argument for doing this could >> equally be applied to something like re-iterating what the operator >> precedence rules for arithmetic are, and I don't think that should be >> mentioned. > > > I disagree on this equivalence. The time literals are clear deviations from expected behavior. Knowing operator precedence rules, they apply everywhere equally. And we should document the deviations directly where they happen. Even if it's just a short link back to the source that describes the deviation. I'm fine with something less verbose pointing only to the data types page, but not with nothing. Are you able to share what the special behaviour is with DEFAULT constraints and time literals that does not apply everywhere equally? Maybe I'm slow on the uptake, but I've yet to see anything here where time literals act in a special way DEFAULT constraints. This is why I couldn't understand why we should be adding documentation about this under CREATE TABLE. I'd be happy to reconsider or retract my argument if you can show me what I'm missing. David ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-06-26 23:54 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-02 18:57 [PATCH 5/8] Optimize allocations in bringetbitmap Tomas Vondra <[email protected]> 2024-06-26 05:35 Re: Should we document how column DEFAULT expressions work? David G. Johnston <[email protected]> 2024-06-26 13:36 ` Re: Should we document how column DEFAULT expressions work? Alvaro Herrera <[email protected]> 2024-06-26 23:54 ` Re: Should we document how column DEFAULT expressions work? David Rowley <[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