public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 3/9] Optimize allocations in bringetbitmap 3+ messages / 3 participants [nested] [flat]
* [PATCH 3/9] Optimize allocations in bringetbitmap @ 2020-09-13 10:12 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tomas Vondra @ 2020-09-13 10:12 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 | 62 +++++++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 14da9ed17f..3735c41788 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; @@ -398,11 +401,50 @@ bringetbitmap(IndexScanDesc scan, TIDBitmap *tbm) * Make room for per-attribute lists of scan keys that we'll pass to the * consistent support procedure. We keep null and regular keys separate, * so that we can easily pass regular keys to the consistent function. + * + * 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. */ - 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 = + /* regular keys */ + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + + MAXALIGN(sizeof(ScanKey) * scan->numberOfKeys * bdesc->bd_tupdesc->natts) + + MAXALIGN(sizeof(int) * bdesc->bd_tupdesc->natts) + + /* NULL keys */ + MAXALIGN(sizeof(ScanKey *) * bdesc->bd_tupdesc->natts) + + 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. @@ -438,9 +480,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); @@ -451,17 +493,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 --------------4B194FF8F3EA3786FF9EAE1F Content-Type: text/x-patch; charset=UTF-8; name="0002-Move-IS-NOT-NULL-handling-from-BRIN-support-20210203.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0002-Move-IS-NOT-NULL-handling-from-BRIN-support-20210203.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: LogwrtResult contended spinlock @ 2024-04-04 22:50 Jeff Davis <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Jeff Davis @ 2024-04-04 22:50 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; [email protected]; Jaime Casanova <[email protected]> On Thu, 2024-04-04 at 19:45 +0200, Alvaro Herrera wrote: > 1. Using pg_atomic_write_membarrier_u64 is useless and it imposes > mora > barriers than we actually need. So I switched back to > pg_atomic_write_u64 and add one barrier between the two writes. Same > for reads. +1. This looks correct to me. Just before the writes there's a spinlock, which acts as a full barrier; and just afterwards, the function returns and the WALWriteLock is released, again acting as a full barrier. The write barrier in between enforces the Write >= Flush invariant. > 2. Using monotonic_advance for Write and Flush is useless. +1. > 3. Testing the invariant that the Copy pointer cannot be 0 is > useless, > because we initialize that pointer to EndOfLog during StartupXLOG. > So, removed. +1. > 4. If we're not modifying any callers of WALReadFromBuffers(), then > AFAICS the added check that the request is not past the Copy pointer > is > useless. In a quick look at that code, I think we only try to read > data > that's been flushed, not written, so the stricter check that we don't > read data that hasn't been Copied does nothing. Bharath has indicated that he may call WALReadFromBuffers() in an extension, so I believe some error checking is appropriate there. > (Honestly I'm not sure > that we want XLogSendPhysical to be reading data that has not been > written, or do we?) Not yet, but there has been some discussion[1][2] about future work to allow replicating data before it's been flushed locally. > Please argue why we need this patch. I'm not sure what you mean by "this patch"? > 5. The existing weird useless-looking block at the end of XLogWrite > is > there because we used to have it to declare a volatile pointer to > XLogCtl (cf. commit 6ba4ecbf477e). That's no longer needed, so we > could remove it. Or we could leave it alone (just because it's > ancient > and it doesn't hurt anything), but there's no reason to have the new > invariant-testing block inside the same block. So I added another > weird > useless-looking block, except that this one does have two variable > declaration at its top. That didn't bother me, but it could be cleaned up a bit in a later patch. > 6. In a few places, we read both Write and Flush to only use one of > them. This is wasteful and we could dial this back to reading only > the > one we need. Andres suggested as much in [1]. I didn't touch this > in > the current patch, and I don't necessarily think we need to address > it > right now. Addressing this should probably done similar to what I > posted in [2]'s 0002. I agree that it should be a separate patch. I haven't thought about the consequences of making them fully independent -- I think that means we give up the invariant that Copy >= Write >= Flush? Regarding the patches themselves, 0001 looks good to me. For 0002, did you consider having pg_atomic_monotonic_advance_u64() return the currval? Regards, Jeff Davis [1] https://www.postgresql.org/message-id/CALj2ACV6rS%2B7iZx5%2BoAvyXJaN4AG-djAQeM1mrM%3DYSDkVrUs7g%40ma... [2] https://www.postgresql.org/message-id/20230125211540.zylu74dj2uuh3k7w%40awork3.anarazel.de [3] https://www.postgresql.org/message-id/CALj2ACW65mqn6Ukv57SqDTMzAJgd1N_AdQtDgy%2BgMDqu6v618Q%40mail.g... ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: LogwrtResult contended spinlock @ 2024-04-05 04:29 Bharath Rupireddy <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Bharath Rupireddy @ 2024-04-05 04:29 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Tom Lane <[email protected]>; Andres Freund <[email protected]>; [email protected]; Jaime Casanova <[email protected]> On Fri, Apr 5, 2024 at 4:21 AM Jeff Davis <[email protected]> wrote: > > > 4. If we're not modifying any callers of WALReadFromBuffers(), then > > AFAICS the added check that the request is not past the Copy pointer > > is > > useless. In a quick look at that code, I think we only try to read > > data > > that's been flushed, not written, so the stricter check that we don't > > read data that hasn't been Copied does nothing. > > Bharath has indicated that he may call WALReadFromBuffers() in an > extension, so I believe some error checking is appropriate there. > > > (Honestly I'm not sure > > that we want XLogSendPhysical to be reading data that has not been > > written, or do we?) > > Not yet, but there has been some discussion[1][2] about future work to > allow replicating data before it's been flushed locally. Right. Although callers of WALReadFromBuffers() in core postgres doesn't need it now (in future, they will), but it allows one to write something up externally - for example, see 0004 patch in https://www.postgresql.org/message-id/[email protected]... where I implemented a xlogreader page_read callback that just keeps reading the WAL that's fully copied to WAL buffers. > Regarding the patches themselves, 0001 looks good to me. A few comments on 0001: 1. /* * Update local copy of shared XLogCtl->log{Write,Flush}Result + * + * It's critical that Flush always trails Write, so the order of the reads is + * important, as is the barrier. */ #define RefreshXLogWriteResult(_target) \ do { \ - _target.Write = XLogCtl->logWriteResult; \ - _target.Flush = XLogCtl->logFlushResult; \ + _target.Flush = pg_atomic_read_u64(&XLogCtl->logFlushResult); \ + pg_read_barrier(); \ + _target.Write = pg_atomic_read_u64(&XLogCtl->logWriteResult); \ } while (0) Is it "Flush always trails Write" or "Flush always leades Write"? I guess the latter, no? 2. + + pg_atomic_write_u64(&XLogCtl->logWriteResult, LogwrtResult.Write); + pg_write_barrier(); + pg_atomic_write_u64(&XLogCtl->logFlushResult, LogwrtResult.Flush); } Maybe add the reason as to why we had to write logWriteResult first and then logFlushResult, similar to the comment atop RefreshXLogWriteResult? > For 0002, did you consider having pg_atomic_monotonic_advance_u64() > return the currval? +1 for returning currval here. Except for the above comments, the patches look good to me. I've also run the test loop for any assertion failure - for i in {1..100}; do make check PROVE_TESTS="t/027_stream_regress.pl"; if [ $? -ne 0 ]; then echo "The command failed on iteration $i"; break; fi; done. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-04-05 04:29 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-13 10:12 [PATCH 3/9] Optimize allocations in bringetbitmap Tomas Vondra <[email protected]> 2024-04-04 22:50 Re: LogwrtResult contended spinlock Jeff Davis <[email protected]> 2024-04-05 04:29 ` Re: LogwrtResult contended spinlock Bharath Rupireddy <[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