public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 5/8] Optimize allocations in bringetbitmap 5+ messages / 3 participants [nested] [flat]
* [PATCH 5/8] Optimize allocations in bringetbitmap @ 2021-03-02 18:57 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 5+ 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 --------------AC3FFB734F56C5F52D422EFC Content-Type: text/x-patch; charset=UTF-8; name="0006-BRIN-bloom-indexes-20210305.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="0006-BRIN-bloom-indexes-20210305.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-01-31 05:31 Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2022-01-31 05:31 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] At Tue, 2 Nov 2021 16:40:39 +0530, Dilip Kumar <[email protected]> wrote in > On Tue, Oct 19, 2021 at 2:25 PM Dilip Kumar <[email protected]> wrote: > > > > On Tue, Oct 12, 2021 at 11:30 AM Dilip Kumar <[email protected]> wrote: > > > > > > On Tue, Oct 12, 2021 at 10:35 AM Kyotaro Horiguchi > > > <[email protected]> wrote: > > > > > > > > At Tue, 12 Oct 2021 13:59:59 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > > > > > So I came up with the attached version. > > > > > > > > Sorry, it was losing a piece of change. > > > > > > Yeah, at a high level this is on the idea I have in mind, I will do a > > > detailed review in a day or two. Thanks for working on this. > > > > While doing the detailed review, I think there are a couple of > > problems with the patch, the main problem of storing all the xid in > > the snap->subxip is that once we mark the snapshot overflown then the > > XidInMVCCSnapshot, will not search the subxip array, instead it will > > fetch the topXid and search in the snap->xip array. > > I missed that you are marking the snapshot as takenDuringRecovery so > your fix looks fine. > > Another issue is > > that the total xids could be GetMaxSnapshotSubxidCount() > > +GetMaxSnapshotXidCount(). > > > > I have fixed this, the updated patch is attached. Mmm. The size of the array cannot be larger than the numbers the *Connt() functions return. Thus we cannot attach the oversized array to ->subxip. (I don't recall clearly but that would lead to assertion failure somewhere..) regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-01-31 06:20 Kyotaro Horiguchi <[email protected]> 1 sibling, 2 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2022-01-31 06:20 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected] At Mon, 17 Jan 2022 09:27:14 +0530, Dilip Kumar <[email protected]> wrote in > On Wed, Jan 12, 2022 at 4:09 PM Julien Rouhaud <[email protected]> wrote: > > The cfbot reports that this patch doesn't compile: > > https://cirrus-ci.com/task/5642000073490432?logs=build > > > > [03:01:24.477] snapbuild.c: In function ‘SnapBuildInitialSnapshot’: > > [03:01:24.477] snapbuild.c:587:2: error: ‘newsubxcnt’ undeclared (first > > use in this function); did you mean ‘newsubxip’? > > [03:01:24.477] 587 | newsubxcnt = 0; > > [03:01:24.477] | ^~~~~~~~~~ > > [03:01:24.477] | newsubxip > > [03:01:24.477] snapbuild.c:587:2: note: each undeclared identifier is > > reported only once for each function it appears in > > [03:01:24.477] snapbuild.c:535:8: warning: unused variable ‘maxxidcnt’ > > [-Wunused-variable] > > [03:01:24.477] 535 | int maxxidcnt; > > [03:01:24.477] | ^~~~~~~~~ > > > > Could you send a new version? In the meantime I will switch the patch to > > Waiting on Author. > > > > Thanks for notifying, I will work on this and send the update patch soon. me> Mmm. The size of the array cannot be larger than the numbers the me> *Connt() functions return. Thus we cannot attach the oversized array me> to ->subxip. (I don't recall clearly but that would lead to assertion me> failure somewhere..) Then, I fixed the v3 error and post v4. To recap: SnapBUildInitialSnapshot tries to store XIDS of both top and sub transactions into snapshot->xip array but the array is easily overflowed and CREATE_REPLICATOIN_SLOT command ends with an error. To fix this, this patch is doing the following things. - Use subxip array instead of xip array to allow us have larger array for xids. So the snapshot is marked as takenDuringRecovery, which is a kind of abuse but largely reduces the chance of getting "initial slot snapshot too large" error. - Still if subxip is overflowed, retry with excluding subtransactions then set suboverflowed. This causes XidInMVCCSnapshot (finally) scans over subxip array for targetted top-level xid. We could take another way: make a !takenDuringRecovery snapshot by using xip instead of subxip. It is cleaner but it has far larger chance of needing to retry. (renamed the patch since it represented a part of the patch) regards. -- Kyotaro Horiguchi NTT Open Source Software Center Attachments: [text/x-patch] v4-0001-Avoid-an-error-while-creating-logical-replication.patch (5.4K, ../../[email protected]/2-v4-0001-Avoid-an-error-while-creating-logical-replication.patch) download | inline diff: From 1e3ec40d70c2e7f8482632333001fe52527ef17a Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <[email protected]> Date: Mon, 31 Jan 2022 15:03:33 +0900 Subject: [PATCH v4] Avoid an error while creating logical replication slot Snapshot can hold top XIDs up to the number of server processes but SnapBuildInitialSnapshot tries to store all top-level and sub XIDs to there and easily fails with "initial slot snapshot too large" error. Instead, create a "takenDuringRecovery" snapshot, which stores all XIDs in subxip array. This alone largely reduces the chance of getting the error. But to eliminate the chance of the error to occur, allow to create an overflowed snapshot by adding to reorder buffer a function to tell whether an XID is a top-level or not. Author: Dilip Kumar and Kyotaro Horiguchi --- .../replication/logical/reorderbuffer.c | 20 +++++++ src/backend/replication/logical/snapbuild.c | 54 ++++++++++++++----- src/include/replication/reorderbuffer.h | 1 + 3 files changed, 63 insertions(+), 12 deletions(-) diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 19b2ba2000..429e6296ab 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -3326,6 +3326,26 @@ ReorderBufferXidHasBaseSnapshot(ReorderBuffer *rb, TransactionId xid) } +/* + * ReorderBufferXidIsKnownSubXact + * Returns true if the xid is a known subtransaction. + */ +bool +ReorderBufferXidIsKnownSubXact(ReorderBuffer *rb, TransactionId xid) +{ + ReorderBufferTXN *txn; + + txn = ReorderBufferTXNByXid(rb, xid, false, + NULL, InvalidXLogRecPtr, false); + + /* a known subtxn? */ + if (txn && rbtxn_is_known_subxact(txn)) + return true; + + return false; +} + + /* * --------------------------------------- * Disk serialization support diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 83fca8a77d..21f30fa1d3 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -531,8 +531,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder) { Snapshot snap; TransactionId xid; - TransactionId *newxip; - int newxcnt = 0; + TransactionId *newsubxip; + int newsubxcnt; + bool overflowed = false; Assert(!FirstSnapshotSet); Assert(XactIsoLevel == XACT_REPEATABLE_READ); @@ -568,9 +569,12 @@ SnapBuildInitialSnapshot(SnapBuild *builder) MyProc->xmin = snap->xmin; - /* allocate in transaction context */ - newxip = (TransactionId *) - palloc(sizeof(TransactionId) * GetMaxSnapshotXidCount()); + /* + * Allocate in transaction context. We use only subxip to store xids. See + * below and GetSnapshotData for details. + */ + newsubxip = (TransactionId *) + palloc(sizeof(TransactionId) * GetMaxSnapshotSubxidCount()); /* * snapbuild.c builds transactions in an "inverted" manner, which means it @@ -578,6 +582,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder) * classical snapshot by marking all non-committed transactions as * in-progress. This can be expensive. */ +retry: + newsubxcnt = 0; + for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);) { void *test; @@ -591,12 +598,28 @@ SnapBuildInitialSnapshot(SnapBuild *builder) if (test == NULL) { - if (newxcnt >= GetMaxSnapshotXidCount()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("initial slot snapshot too large"))); + /* + * If subtransactions fit the subxid array, we fill the array and + * call it a day. Otherwise we store only top-level transactions + * into the same subxip array. + */ + if (!overflowed || + !ReorderBufferXidIsKnownSubXact(builder->reorder, xid)) + { + if (newsubxcnt >= GetMaxSnapshotSubxidCount()) + { + if (overflowed) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("initial slot snapshot too large"))); - newxip[newxcnt++] = xid; + /* retry excluding subxids */ + overflowed = true; + goto retry; + } + + newsubxip[newsubxcnt++] = xid; + } } TransactionIdAdvance(xid); @@ -604,8 +627,15 @@ SnapBuildInitialSnapshot(SnapBuild *builder) /* adjust remaining snapshot fields as needed */ snap->snapshot_type = SNAPSHOT_MVCC; - snap->xcnt = newxcnt; - snap->xip = newxip; + snap->xcnt = 0; + snap->subxcnt = newsubxcnt; + snap->subxip = newsubxip; + snap->suboverflowed = overflowed; + /* + * Although this snapshot is taken actually not during recovery, all XIDs + * are stored in subxip even if it is not overflowed. + */ + snap->takenDuringRecovery = true; return snap; } diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h index aa0a73382f..c05cf3078c 100644 --- a/src/include/replication/reorderbuffer.h +++ b/src/include/replication/reorderbuffer.h @@ -669,6 +669,7 @@ void ReorderBufferXidSetCatalogChanges(ReorderBuffer *, TransactionId xid, XLog bool ReorderBufferXidHasCatalogChanges(ReorderBuffer *, TransactionId xid); bool ReorderBufferXidHasBaseSnapshot(ReorderBuffer *, TransactionId xid); +bool ReorderBufferXidIsKnownSubXact(ReorderBuffer *rb, TransactionId xid); bool ReorderBufferRememberPrepareInfo(ReorderBuffer *rb, TransactionId xid, XLogRecPtr prepare_lsn, XLogRecPtr end_lsn, TimestampTz prepare_time, -- 2.27.0 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-01-31 07:04 Dilip Kumar <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Dilip Kumar @ 2022-01-31 07:04 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Jan 31, 2022 at 11:50 AM Kyotaro Horiguchi <[email protected]> wrote: > > SnapBUildInitialSnapshot tries to store XIDS of both top and sub > transactions into snapshot->xip array but the array is easily > overflowed and CREATE_REPLICATOIN_SLOT command ends with an error. > > To fix this, this patch is doing the following things. > > - Use subxip array instead of xip array to allow us have larger array > for xids. So the snapshot is marked as takenDuringRecovery, which > is a kind of abuse but largely reduces the chance of getting > "initial slot snapshot too large" error. > > - Still if subxip is overflowed, retry with excluding subtransactions > then set suboverflowed. This causes XidInMVCCSnapshot (finally) > scans over subxip array for targetted top-level xid. > > We could take another way: make a !takenDuringRecovery snapshot by > using xip instead of subxip. It is cleaner but it has far larger > chance of needing to retry. > > (renamed the patch since it represented a part of the patch) > Thanks for the updated version. I will look into it this week. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-02-07 08:22 Dilip Kumar <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Dilip Kumar @ 2022-02-07 08:22 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Jan 31, 2022 at 11:50 AM Kyotaro Horiguchi <[email protected]> wrote: > > At Mon, 17 Jan 2022 09:27:14 +0530, Dilip Kumar <[email protected]> wrote in > > me> Mmm. The size of the array cannot be larger than the numbers the > me> *Connt() functions return. Thus we cannot attach the oversized array > me> to ->subxip. (I don't recall clearly but that would lead to assertion > me> failure somewhere..) > > Then, I fixed the v3 error and post v4. Yeah you are right, SetTransactionSnapshot() has that assertion. Anyway after looking again it appears that GetMaxSnapshotSubxidCount is the correct size because this is PGPROC_MAX_CACHED_SUBXIDS +1, i.e. it considers top transactions as well so we don't need to add them separately. > > SnapBUildInitialSnapshot tries to store XIDS of both top and sub > transactions into snapshot->xip array but the array is easily > overflowed and CREATE_REPLICATOIN_SLOT command ends with an error. > > To fix this, this patch is doing the following things. > > - Use subxip array instead of xip array to allow us have larger array > for xids. So the snapshot is marked as takenDuringRecovery, which > is a kind of abuse but largely reduces the chance of getting > "initial slot snapshot too large" error. Right. I think the patch looks fine to me. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-02-07 08:22 UTC | newest] Thread overview: 5+ 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]> 2022-01-31 05:31 ` Re: Error "initial slot snapshot too large" in create replication slot Kyotaro Horiguchi <[email protected]> 2022-01-31 06:20 ` Re: Error "initial slot snapshot too large" in create replication slot Kyotaro Horiguchi <[email protected]> 2022-01-31 07:04 ` Re: Error "initial slot snapshot too large" in create replication slot Dilip Kumar <[email protected]> 2022-02-07 08:22 ` Re: Error "initial slot snapshot too large" in create replication slot Dilip Kumar <[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