public inbox for [email protected]help / color / mirror / Atom feed
Error "initial slot snapshot too large" in create replication slot 29+ messages / 9 participants [nested] [flat]
* Error "initial slot snapshot too large" in create replication slot @ 2021-10-11 06:19 Dilip Kumar <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Dilip Kumar @ 2021-10-11 06:19 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> While creating an "export snapshot" I don't see any protection why the number of xids in the snapshot can not cross the "GetMaxSnapshotXidCount()"?. Basically, while converting the HISTORIC snapshot to the MVCC snapshot in "SnapBuildInitialSnapshot()", we add all the xids between snap->xmin to snap->xmax to the MVCC snap->xip array (xids for which commit were not recorded). The problem is that we add both topxids as well as the subxids into the same array and expect that the "xid" count does not cross the "GetMaxSnapshotXidCount()". So it seems like an issue but I am not sure what is the fix for this, some options are a) Don't limit the xid count in the exported snapshot and dynamically resize the array b) Increase the limit to GetMaxSnapshotXidCount() + GetMaxSnapshotSubxidCount(). But in option b) there would still be a problem that how do we handle the overflowed subtransaction? I have locally, reproduced the issue, 1. Configuration max_connections= 5 autovacuum = off max_worker_processes = 0 2.Then from pgbench I have run the attached script (test.sql) from 5 clients. ./pgbench -i postgres ./pgbench -c4 -j4 -T 3000 -f test1.sql -P1 postgres 3. Concurrently, create replication slot, [dilipkumar@localhost bin]$ ./psql "dbname=postgres replication=database" postgres[7367]=# postgres[6463]=# CREATE_REPLICATION_SLOT "slot" LOGICAL "test_decoding"; ERROR: 40001: initial slot snapshot too large LOCATION: SnapBuildInitialSnapshot, snapbuild.c:597 postgres[6463]=# CREATE_REPLICATION_SLOT "slot" LOGICAL "test_decoding"; ERROR: XX000: clearing exported snapshot in wrong transaction state LOCATION: SnapBuildClearExportedSnapshot, snapbuild.c:690 I could reproduce this issue, at least once in 8-10 attempts of creating the replication slot. Note: After that issue, I have noticed one more issue "clearing exported snapshot in wrong transaction state", that is because the "ExportInProgress" is not cleared on the transaction abort, for this, a simple fix is we can clear this state on the transaction abort, maybe I will raise this as a separate issue? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com Attachments: [application/sql] test.sql (1.0K, ../../CAFiTN-tqopqpfS6HHug2nnOGieJJ_nm-Nvy0WBZ=Zpo-LqtSJA@mail.gmail.com/2-test.sql) download ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-10-11 10:59 Kyotaro Horiguchi <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2021-10-11 10:59 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <[email protected]> wrote in > While creating an "export snapshot" I don't see any protection why the > number of xids in the snapshot can not cross the > "GetMaxSnapshotXidCount()"?. > > Basically, while converting the HISTORIC snapshot to the MVCC snapshot > in "SnapBuildInitialSnapshot()", we add all the xids between > snap->xmin to snap->xmax to the MVCC snap->xip array (xids for which > commit were not recorded). The problem is that we add both topxids as > well as the subxids into the same array and expect that the "xid" > count does not cross the "GetMaxSnapshotXidCount()". So it seems like > an issue but I am not sure what is the fix for this, some options are It seems to me that it is a compromise between the restriction of the legitimate snapshot and snapshots created by snapbuild. If the xids overflow, the resulting snapshot may lose a siginificant xid, i.e, a top-level xid. > a) Don't limit the xid count in the exported snapshot and dynamically > resize the array b) Increase the limit to GetMaxSnapshotXidCount() + > GetMaxSnapshotSubxidCount(). But in option b) there would still be a > problem that how do we handle the overflowed subtransaction? I'm afraid that we shouldn't expand the size limits. If I understand it correctly, we only need the top-level xids in the exported snapshot and reorder buffer knows whether a xid is a top-level or not after establishing a consistent snapshot. The attached diff tries to make SnapBuildInitialSnapshot exclude subtransaction from generated snapshots. It seems working fine for you repro. (Of course, I'm not confident that it is the correct thing, though..) What do you think about this? regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 46e66608cf..4e452cce7c 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 a5333349a8..12d283f4de 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -591,12 +591,18 @@ SnapBuildInitialSnapshot(SnapBuild *builder) if (test == NULL) { - if (newxcnt >= GetMaxSnapshotXidCount()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("initial slot snapshot too large"))); + bool issubxact = + ReorderBufferXidIsKnownSubXact(builder->reorder, xid); - newxip[newxcnt++] = xid; + if (!issubxact) + { + if (newxcnt >= GetMaxSnapshotXidCount()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("initial slot snapshot too large"))); + + newxip[newxcnt++] = xid; + } } TransactionIdAdvance(xid); diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h index 5b40ff75f7..e5fa1051d7 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, Attachments: [text/plain] exclude_subxact_from_snapbuild_snapshot.diff.txt (2.4K, ../../[email protected]/2-exclude_subxact_from_snapbuild_snapshot.diff.txt) download | inline diff: diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 46e66608cf..4e452cce7c 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 a5333349a8..12d283f4de 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -591,12 +591,18 @@ SnapBuildInitialSnapshot(SnapBuild *builder) if (test == NULL) { - if (newxcnt >= GetMaxSnapshotXidCount()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("initial slot snapshot too large"))); + bool issubxact = + ReorderBufferXidIsKnownSubXact(builder->reorder, xid); - newxip[newxcnt++] = xid; + if (!issubxact) + { + if (newxcnt >= GetMaxSnapshotXidCount()) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("initial slot snapshot too large"))); + + newxip[newxcnt++] = xid; + } } TransactionIdAdvance(xid); diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h index 5b40ff75f7..e5fa1051d7 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, ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-10-11 11:18 Dilip Kumar <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Dilip Kumar @ 2021-10-11 11:18 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Mon, Oct 11, 2021 at 4:29 PM Kyotaro Horiguchi <[email protected]> wrote: > > At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <[email protected]> wrote in > > While creating an "export snapshot" I don't see any protection why the > > number of xids in the snapshot can not cross the > > "GetMaxSnapshotXidCount()"?. > > > > Basically, while converting the HISTORIC snapshot to the MVCC snapshot > > in "SnapBuildInitialSnapshot()", we add all the xids between > > snap->xmin to snap->xmax to the MVCC snap->xip array (xids for which > > commit were not recorded). The problem is that we add both topxids as > > well as the subxids into the same array and expect that the "xid" > > count does not cross the "GetMaxSnapshotXidCount()". So it seems like > > an issue but I am not sure what is the fix for this, some options are > > It seems to me that it is a compromise between the restriction of the > legitimate snapshot and snapshots created by snapbuild. If the xids > overflow, the resulting snapshot may lose a siginificant xid, i.e, a > top-level xid. > > > a) Don't limit the xid count in the exported snapshot and dynamically > > resize the array b) Increase the limit to GetMaxSnapshotXidCount() + > > GetMaxSnapshotSubxidCount(). But in option b) there would still be a > > problem that how do we handle the overflowed subtransaction? > > I'm afraid that we shouldn't expand the size limits. If I understand > it correctly, we only need the top-level xids in the exported snapshot But since we are using this as an MVCC snapshot, if we don't have the subxid and if we also don't mark the "suboverflowed" flag then IMHO the sub-transaction visibility might be wrong, Am I missing something? > and reorder buffer knows whether a xid is a top-level or not after > establishing a consistent snapshot. > > The attached diff tries to make SnapBuildInitialSnapshot exclude > subtransaction from generated snapshots. It seems working fine for > you repro. (Of course, I'm not confident that it is the correct thing, > though..) > > What do you think about this? If your statement that we only need top-xids in the exported snapshot, is true then this fix looks fine to me. If not then we might need to add the sub-xids in the snapshot->subxip array and if it crosses the limit of GetMaxSnapshotSubxidCount(), then we can mark "suboverflowed" flag. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-10-12 04:59 Kyotaro Horiguchi <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2021-10-12 04:59 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] At Mon, 11 Oct 2021 16:48:10 +0530, Dilip Kumar <[email protected]> wrote in > On Mon, Oct 11, 2021 at 4:29 PM Kyotaro Horiguchi > <[email protected]> wrote: > > > > At Mon, 11 Oct 2021 11:49:41 +0530, Dilip Kumar <[email protected]> wrote in > > > While creating an "export snapshot" I don't see any protection why the > > > number of xids in the snapshot can not cross the > > > "GetMaxSnapshotXidCount()"?. > > > > > > Basically, while converting the HISTORIC snapshot to the MVCC snapshot > > > in "SnapBuildInitialSnapshot()", we add all the xids between > > > snap->xmin to snap->xmax to the MVCC snap->xip array (xids for which > > > commit were not recorded). The problem is that we add both topxids as > > > well as the subxids into the same array and expect that the "xid" > > > count does not cross the "GetMaxSnapshotXidCount()". So it seems like > > > an issue but I am not sure what is the fix for this, some options are > > > > It seems to me that it is a compromise between the restriction of the > > legitimate snapshot and snapshots created by snapbuild. If the xids > > overflow, the resulting snapshot may lose a siginificant xid, i.e, a > > top-level xid. > > > > > a) Don't limit the xid count in the exported snapshot and dynamically > > > resize the array b) Increase the limit to GetMaxSnapshotXidCount() + > > > GetMaxSnapshotSubxidCount(). But in option b) there would still be a > > > problem that how do we handle the overflowed subtransaction? > > > > I'm afraid that we shouldn't expand the size limits. If I understand > > it correctly, we only need the top-level xids in the exported snapshot > > But since we are using this as an MVCC snapshot, if we don't have the > subxid and if we also don't mark the "suboverflowed" flag then IMHO > the sub-transaction visibility might be wrong, Am I missing something? Sorry I should have set suboverflowed in the generated snapshot. However, we can store subxid list as well when the snapshot (or running_xact) is not overflown. These (should) works the same way. On physical standby, snapshot is created just filling up only subxip with all top and sub xids (procrray.c:2400). It would be better we do the same thing here. > > and reorder buffer knows whether a xid is a top-level or not after > > establishing a consistent snapshot. > > > > The attached diff tries to make SnapBuildInitialSnapshot exclude > > subtransaction from generated snapshots. It seems working fine for > > you repro. (Of course, I'm not confident that it is the correct thing, > > though..) > > > > What do you think about this? > > If your statement that we only need top-xids in the exported snapshot, > is true then this fix looks fine to me. If not then we might need to > add the sub-xids in the snapshot->subxip array and if it crosses the > limit of GetMaxSnapshotSubxidCount(), then we can mark "suboverflowed" > flag. So I came up with the attached version. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-10-12 05:05 Kyotaro Horiguchi <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2021-10-12 05:05 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] 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. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-10-12 06:00 Dilip Kumar <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Dilip Kumar @ 2021-10-12 06:00 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> 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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-10-19 08:55 Dilip Kumar <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Dilip Kumar @ 2021-10-19 08:55 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> 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. Another issue is that the total xids could be GetMaxSnapshotSubxidCount() +GetMaxSnapshotXidCount(). I think what we should be doing is that if the xid is know subxid then add in the snap->subxip array otherwise in snap->xip array. So snap->xip array size will be GetMaxSnapshotXidCount() whereas the snap->subxip array size will be GetMaxSnapshotSubxidCount(). And if the array size is full then we can stop adding the subxids to the array. What is your thought on this? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2021-11-02 11:10 Dilip Kumar <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Dilip Kumar @ 2021-11-02 11:10 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> 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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com Attachments: [text/x-patch] v3-0001-Allow-overflowed-snapshot-when-creating-logical-r.patch (5.2K, ../../CAFiTN-ui+vtbUq_-3+SzT4KU_=iAJz7_-en+_tnPz-p34h76mQ@mail.gmail.com/2-v3-0001-Allow-overflowed-snapshot-when-creating-logical-r.patch) download | inline diff: From b6ca7a05b05a9244bf400d14b75dfcd1f99c76cd Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <[email protected]> Date: Tue, 12 Oct 2021 13:53:27 +0900 Subject: [PATCH v3] Allow overflowed snapshot when 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 fails for certain circumstances. Instead, create a "takenDuringRecovery" snapshot instead, which stores all XIDs in subxip array. Addition to that, allow to create an overflowed snapshot by adding to reorder buffer a function to tell whether an XID is a top-level or not. --- src/backend/replication/logical/reorderbuffer.c | 20 +++++++++ src/backend/replication/logical/snapbuild.c | 57 +++++++++++++++++++------ src/include/replication/reorderbuffer.h | 1 + 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 46e6660..4e452cc 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -3327,6 +3327,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 dbdc172..9fd28d4 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 maxxidcnt; + bool overflowed = false; Assert(!FirstSnapshotSet); Assert(XactIsoLevel == XACT_REPEATABLE_READ); @@ -568,9 +569,13 @@ 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 + * GetSnapshotData for details. + */ + newsubxip = (TransactionId *) palloc(sizeof(TransactionId) * + (GetMaxSnapshotXidCount() + + GetMaxSnapshotSubxidCount())); /* * snapbuild.c builds transactions in an "inverted" manner, which means it @@ -578,6 +583,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 +599,29 @@ SnapBuildInitialSnapshot(SnapBuild *builder) if (test == NULL) { - if (newxcnt >= GetMaxSnapshotXidCount()) - ereport(ERROR, - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), - errmsg("initial slot snapshot too large"))); + bool add = true; - newxip[newxcnt++] = xid; + /* exlude subxids when subxip is overflowed */ + if (overflowed && + ReorderBufferXidIsKnownSubXact(builder->reorder, xid)) + add = false; + + if (add) + { + if (newsubxcnt >= GetMaxSnapshotSubxidCount()) + { + if (overflowed) + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("initial slot snapshot too large"))); + + /* retry excluding subxids */ + overflowed = true; + goto retry; + } + + newsubxip[newsubxcnt++] = xid; + } } TransactionIdAdvance(xid); @@ -604,8 +629,16 @@ 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 5b40ff7..e5fa105 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, -- 1.8.3.1 ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-07-05 18:32 Jacob Champion <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Jacob Champion @ 2022-07-05 18:32 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; PostgreSQL Hackers <[email protected]> On Thu, Mar 31, 2022 at 11:53 PM Kyotaro Horiguchi <[email protected]> wrote: > So this is that. v5 creates a regular snapshot. This patch will need a quick rebase over 905c020bef9, which added `extern` to several missing locations. Thanks, --Jacob ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-07-19 02:55 Kyotaro Horiguchi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 2 replies; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-07-19 02:55 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected] At Tue, 5 Jul 2022 11:32:42 -0700, Jacob Champion <[email protected]> wrote in > On Thu, Mar 31, 2022 at 11:53 PM Kyotaro Horiguchi > <[email protected]> wrote: > > So this is that. v5 creates a regular snapshot. > > This patch will need a quick rebase over 905c020bef9, which added > `extern` to several missing locations. Thanks! Just rebased. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-09 17:19 Robert Haas <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 1 sibling, 1 reply; 29+ messages in thread From: Robert Haas @ 2022-09-09 17:19 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; Andres Freund <[email protected]>; +Cc: Jacob Champion <[email protected]>; Sokolov Yura <[email protected]>; Dilip Kumar <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Jul 18, 2022 at 10:55 PM Kyotaro Horiguchi <[email protected]> wrote: > Thanks! Just rebased. Hi, I mentioned this patch to Andres in conversation, and he expressed a concern that there might be no guarantee that we retain enough CLOG to look up XIDs. Presumably this wouldn't be an issue when the snapshot doesn't get marked suboverflowed, but what if it does? Adding Andres in the hopes that he may comment further. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-12 21:51 Andres Freund <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 1 sibling, 2 replies; 29+ messages in thread From: Andres Freund @ 2022-09-12 21:51 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] Hi, Thanks for working on this! I think this should include a test that fails without this change and succeeds with it... On 2022-07-19 11:55:06 +0900, Kyotaro Horiguchi wrote: > From abcf0a0e0b3e2de9927d8943a3e3c145ab189508 Mon Sep 17 00:00:00 2001 > From: Kyotaro Horiguchi <[email protected]> > Date: Tue, 19 Jul 2022 11:50:29 +0900 > Subject: [PATCH v6] Create correct snapshot during CREATE_REPLICATION_SLOT This sees a tad misleading - the previous snapshot wasn't borken, right? > +/* > + * 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; > +} The comments here just seem to restate the code.... It's not obvious to me that it's the right design (or even correct) to ask reorderbuffer about an xid being a subxid. Maybe I'm missing something, but why would reorderbuffer even be guaranteed to know about all these subxids? > @@ -568,9 +571,17 @@ SnapBuildInitialSnapshot(SnapBuild *builder) > > MyProc->xmin = snap->xmin; > > - /* allocate in transaction context */ > + /* > + * Allocate in transaction context. > + * > + * We could use only subxip to store all xids (takenduringrecovery > + * snapshot) but that causes useless visibility checks later so we hasle to > + * create a normal snapshot. > + */ I can't really parse this comment at this point, and I seriously doubt I could later on. > @@ -591,12 +605,24 @@ SnapBuildInitialSnapshot(SnapBuild *builder) > > if (test == NULL) > { > - if (newxcnt >= GetMaxSnapshotXidCount()) > - ereport(ERROR, > - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), > - errmsg("initial slot snapshot too large"))); > - > - newxip[newxcnt++] = xid; > + /* Store the xid to the appropriate xid array */ > + if (ReorderBufferXidIsKnownSubXact(builder->reorder, xid)) > + { > + if (!overflowed) > + { > + if (newsubxcnt >= GetMaxSnapshotSubxidCount()) > + overflowed = true; > + else > + newsubxip[newsubxcnt++] = xid; > + } > + } > + else > + { > + if (newxcnt >= GetMaxSnapshotXidCount()) > + elog(ERROR, > + "too many transactions while creating snapshot"); > + newxip[newxcnt++] = xid; > + } > } Hm, this is starting to be pretty deeply nested... I wonder if a better fix here wouldn't be to allow importing a snapshot with a larger ->xid array. Yes, we can't do that in CurrentSnapshotData, but IIRC we need to be in a transactional snapshot anyway, which is copied anyway? Greetings, Andres Freund ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-12 21:53 Andres Freund <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Andres Freund @ 2022-09-12 21:53 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; Jacob Champion <[email protected]>; Sokolov Yura <[email protected]>; Dilip Kumar <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, On 2022-09-09 13:19:14 -0400, Robert Haas wrote: > I mentioned this patch to Andres in conversation, and he expressed a > concern that there might be no guarantee that we retain enough CLOG to > look up XIDs. I was concerned we wouldn't keep enough subtrans, rather than clog. But I think we're ok, because we need to have an appropriate ->xmin for exporting / importing the snapshot. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 01:30 Dilip Kumar <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 29+ messages in thread From: Dilip Kumar @ 2022-09-13 01:30 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected] On Tue, Sep 13, 2022 at 3:22 AM Andres Freund <[email protected]> wrote: > > It's not obvious to me that it's the right design (or even correct) to ask > reorderbuffer about an xid being a subxid. Maybe I'm missing something, but > why would reorderbuffer even be guaranteed to know about all these subxids? Yeah, you are right, the reorderbuffer will only know about the transaction for which changes got added to the reorder buffer. So this seems not to be the right design idea. > > I wonder if a better fix here wouldn't be to allow importing a snapshot with a > larger ->xid array. Yes, we can't do that in CurrentSnapshotData, but IIRC we > need to be in a transactional snapshot anyway, which is copied anyway? Yeah when I first found this issue, I thought that should be the solution. But later it went in a different direction. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 06:22 Kyotaro Horiguchi <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 06:22 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] Thanks for raizing this up, Robert and the comment, Andres. At Tue, 13 Sep 2022 07:00:42 +0530, Dilip Kumar <[email protected]> wrote in > On Tue, Sep 13, 2022 at 3:22 AM Andres Freund <[email protected]> wrote: > > > > > It's not obvious to me that it's the right design (or even correct) to ask > > reorderbuffer about an xid being a subxid. Maybe I'm missing something, but > > why would reorderbuffer even be guaranteed to know about all these subxids? > > Yeah, you are right, the reorderbuffer will only know about the > transaction for which changes got added to the reorder buffer. So > this seems not to be the right design idea. That function is called after the SnapBuild reaches SNAPBUILD_CONSISTENT state ,or SnapBuildInitialSnapshot() rejects other than that state. That is, IIUC the top-sub relationship of all the currently running transactions is fully known to reorder buffer. We need a comment about that. > > I wonder if a better fix here wouldn't be to allow importing a snapshot with a > > larger ->xid array. Yes, we can't do that in CurrentSnapshotData, but IIRC we > > need to be in a transactional snapshot anyway, which is copied anyway? > > Yeah when I first found this issue, I thought that should be the > solution. But later it went in a different direction. I think that that is the best solution if rbtxn_is_known_subxzact() is known to be unreliable at the time. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 06:38 Dilip Kumar <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Dilip Kumar @ 2022-09-13 06:38 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] On Tue, Sep 13, 2022 at 11:52 AM Kyotaro Horiguchi <[email protected]> wrote: > > Thanks for raizing this up, Robert and the comment, Andres. > > At Tue, 13 Sep 2022 07:00:42 +0530, Dilip Kumar <[email protected]> wrote in > > On Tue, Sep 13, 2022 at 3:22 AM Andres Freund <[email protected]> wrote: > > > > > > > > It's not obvious to me that it's the right design (or even correct) to ask > > > reorderbuffer about an xid being a subxid. Maybe I'm missing something, but > > > why would reorderbuffer even be guaranteed to know about all these subxids? > > > > Yeah, you are right, the reorderbuffer will only know about the > > transaction for which changes got added to the reorder buffer. So > > this seems not to be the right design idea. > > That function is called after the SnapBuild reaches > SNAPBUILD_CONSISTENT state ,or SnapBuildInitialSnapshot() rejects > other than that state. That is, IIUC the top-sub relationship of all > the currently running transactions is fully known to reorder buffer. > We need a comment about that. I don't think this assumption is true, any xid started after switching to the SNAPBUILD_FULL_SNAPSHOT and before switching to the SNAPBUILD_CONSISTENT, might still be in progress so we can not identify whether they are subxact or not from reorder buffer. refer to this comment: /* * c) transition from FULL_SNAPSHOT to CONSISTENT. * * In FULL_SNAPSHOT state (see d) ), and this xl_running_xacts' * oldestRunningXid is >= than nextXid from when we switched to * FULL_SNAPSHOT. This means all transactions that are currently in * progress have a catalog snapshot, and all their changes have been * collected. Switch to CONSISTENT. */ -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 06:45 Kyotaro Horiguchi <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 06:45 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <[email protected]> wrote in > Hi, > > Thanks for working on this! > > > I think this should include a test that fails without this change and succeeds > with it... > > > On 2022-07-19 11:55:06 +0900, Kyotaro Horiguchi wrote: > > From abcf0a0e0b3e2de9927d8943a3e3c145ab189508 Mon Sep 17 00:00:00 2001 > > From: Kyotaro Horiguchi <[email protected]> > > Date: Tue, 19 Jul 2022 11:50:29 +0900 > > Subject: [PATCH v6] Create correct snapshot during CREATE_REPLICATION_SLOT > > This sees a tad misleading - the previous snapshot wasn't borken, right? I saw it kind of broken that ->xip contains sub transactions. But I didn't meant it's broken by "correct". Is "proper" suitable there? > > +/* > > + * 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; > > +} > > The comments here just seem to restate the code.... Yeah, it is pulled from the existing code but result looks like so.. > It's not obvious to me that it's the right design (or even correct) to ask > reorderbuffer about an xid being a subxid. Maybe I'm missing something, but > why would reorderbuffer even be guaranteed to know about all these subxids? I think you're missing that the code is visited only after the reorder buffer's state becomes SNAPBUILD_CONSISTENT. I think rbtxn_is_known_subxact() is reliable at that stage. > > @@ -568,9 +571,17 @@ SnapBuildInitialSnapshot(SnapBuild *builder) > > > > MyProc->xmin = snap->xmin; > > > > - /* allocate in transaction context */ > > + /* > > + * Allocate in transaction context. > > + * > > + * We could use only subxip to store all xids (takenduringrecovery > > + * snapshot) but that causes useless visibility checks later so we hasle to > > + * create a normal snapshot. > > + */ > > I can't really parse this comment at this point, and I seriously doubt I could > later on. Mmm. The "takenduringrecovery" is relly perplexing (it has been somehow lower-cased..), but after removing the parenthesized part, it looks like this. And it had a misspelling but I removed that word. Is this still unreadable? We could use only subxip to store all xids but that causes useless visibility checks later so we create a normal snapshot. > > @@ -591,12 +605,24 @@ SnapBuildInitialSnapshot(SnapBuild *builder) > > > > if (test == NULL) > > { > > - if (newxcnt >= GetMaxSnapshotXidCount()) > > - ereport(ERROR, > > - (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), > > - errmsg("initial slot snapshot too large"))); > > - > > - newxip[newxcnt++] = xid; > > + /* Store the xid to the appropriate xid array */ > > + if (ReorderBufferXidIsKnownSubXact(builder->reorder, xid)) > > + { > > + if (!overflowed) > > + { > > + if (newsubxcnt >= GetMaxSnapshotSubxidCount()) > > + overflowed = true; > > + else > > + newsubxip[newsubxcnt++] = xid; > > + } > > + } > > + else > > + { > > + if (newxcnt >= GetMaxSnapshotXidCount()) > > + elog(ERROR, > > + "too many transactions while creating snapshot"); > > + newxip[newxcnt++] = xid; > > + } > > } > > Hm, this is starting to be pretty deeply nested... Yeah, at least one if() is removable. > I wonder if a better fix here wouldn't be to allow importing a snapshot with a > larger ->xid array. Yes, we can't do that in CurrentSnapshotData, but IIRC we > need to be in a transactional snapshot anyway, which is copied anyway? The other reason for oversized xip array is it causes visibility check when it is used. AFAICS XidInMVCCSnapshot has additional path for takenDuringRecovery snapshots that contains a linear search (currently it is replaced by pg_lfind32()). This saves us from doing this for that snapshot. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 07:10 Kyotaro Horiguchi <[email protected]> parent: Dilip Kumar <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 07:10 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] At Tue, 13 Sep 2022 12:08:18 +0530, Dilip Kumar <[email protected]> wrote in > On Tue, Sep 13, 2022 at 11:52 AM Kyotaro Horiguchi > <[email protected]> wrote: > > That function is called after the SnapBuild reaches > > SNAPBUILD_CONSISTENT state ,or SnapBuildInitialSnapshot() rejects > > other than that state. That is, IIUC the top-sub relationship of all > > the currently running transactions is fully known to reorder buffer. > > We need a comment about that. > > I don't think this assumption is true, any xid started after switching > to the SNAPBUILD_FULL_SNAPSHOT and before switching to the > SNAPBUILD_CONSISTENT, might still be in progress so we can not > identify whether they are subxact or not from reorder buffer. Yeah, I misunderstood that the relationship is recorded earlier (how?). Thus it is not reliable in the first place. I agree that the best way is oversized xip. By the way, I feel that "is >= than" is redundant or plain wrong.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 07:15 Kyotaro Horiguchi <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 07:15 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] At Tue, 13 Sep 2022 15:45:07 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <[email protected]> wrote in > > This sees a tad misleading - the previous snapshot wasn't borken, right? > > I saw it kind of broken that ->xip contains sub transactions. But I > didn't meant it's broken by "correct". Is "proper" suitable there? No. It's not broken if it is takenDuringRecovery. So this flag can be used to notify that xip can be oversized. I realized that rbtxn_is_known_subxact is not reliable. I'm redirecting to oversized xip. Pleas wait for a while. regareds. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 07:30 Kyotaro Horiguchi <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 07:30 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] At Tue, 13 Sep 2022 16:10:59 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Tue, 13 Sep 2022 12:08:18 +0530, Dilip Kumar <[email protected]> wrote in > > On Tue, Sep 13, 2022 at 11:52 AM Kyotaro Horiguchi > > <[email protected]> wrote: > > > That function is called after the SnapBuild reaches > > > SNAPBUILD_CONSISTENT state ,or SnapBuildInitialSnapshot() rejects > > > other than that state. That is, IIUC the top-sub relationship of all > > > the currently running transactions is fully known to reorder buffer. > > > We need a comment about that. > > > > I don't think this assumption is true, any xid started after switching > > to the SNAPBUILD_FULL_SNAPSHOT and before switching to the > > SNAPBUILD_CONSISTENT, might still be in progress so we can not > > identify whether they are subxact or not from reorder buffer. > > Yeah, I misunderstood that the relationship is recorded earlier > (how?). Thus it is not reliable in the first place. > > I agree that the best way is oversized xip. > > > By the way, I feel that "is >= than" is redundant or plain wrong.. By the way GetSnapshotData() does this: > snapshot->subxip = (TransactionId *) > malloc(GetMaxSnapshotSubxidCount() * sizeof(TransactionId)); ... > if (!snapshot->takenDuringRecovery) ... > else > { > subcount = KnownAssignedXidsGetAndSetXmin(snapshot->subxip, &xmin, > xmax); It is possible that the subxip is overrun. We need to expand the array somehow. Or assign the array of the size (GetMaxSnapshotXidCount() + GetMaxSnapshotSubxidCount()) for takenDuringRecovery snapshots. (I feel deja vu..) regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 07:31 Kyotaro Horiguchi <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 07:31 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] Sigh.. At Tue, 13 Sep 2022 16:30:06 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > It is possible that the subxip is overrun. We need to expand the array > somehow. Or assign the array of the size (GetMaxSnapshotXidCount() + > GetMaxSnapshotSubxidCount()) for takenDuringRecovery snapshots. And I found that this is already done. What we should is doing the same thing in snapbuild. Sorry for the noise.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-09-13 08:31 Kyotaro Horiguchi <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Kyotaro Horiguchi @ 2022-09-13 08:31 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] At Tue, 13 Sep 2022 16:15:34 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > At Tue, 13 Sep 2022 15:45:07 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in > > At Mon, 12 Sep 2022 14:51:56 -0700, Andres Freund <[email protected]> wrote in > > > This sees a tad misleading - the previous snapshot wasn't borken, right? > > > > I saw it kind of broken that ->xip contains sub transactions. But I > > didn't meant it's broken by "correct". Is "proper" suitable there? > > No. It's not broken if it is takenDuringRecovery. So this flag can be > used to notify that xip can be oversized. > > I realized that rbtxn_is_known_subxact is not reliable. I'm > redirecting to oversized xip. Pleas wait for a while. However, the reader of saved snapshots (ImportSnapshot) has the restriction that > if (xcnt < 0 || xcnt > GetMaxSnapshotXidCount()) > ereport(ERROR, and > if (xcnt < 0 || xcnt > GetMaxSnapshotSubxidCount()) > ereport(ERROR, (this xid is subxcnt) And ExportSnapshot repalces oversized subxip with overflowed. So even when GetSnapshotData() returns a snapshot with oversized subxip, it is saved as just "overflowed" on exporting. I don't think this is the expected behavior since such (no xip and overflowed) snapshot no longer works. On the other hand, it seems to me that snapbuild doesn't like takenDuringRecovery snapshots. So snapshot needs additional flag signals that xip is oversized and all xid are holded there. And also need to let takenDuringRecovery suggest subxip is oversized. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot @ 2022-10-12 05:10 Michael Paquier <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Michael Paquier @ 2022-10-12 05:10 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected] On Tue, Sep 13, 2022 at 05:31:05PM +0900, Kyotaro Horiguchi wrote: > And ExportSnapshot repalces oversized subxip with overflowed. > > So even when GetSnapshotData() returns a snapshot with oversized > subxip, it is saved as just "overflowed" on exporting. I don't think > this is the expected behavior since such (no xip and overflowed) > snapshot no longer works. > > On the other hand, it seems to me that snapbuild doesn't like > takenDuringRecovery snapshots. > > So snapshot needs additional flag signals that xip is oversized and > all xid are holded there. And also need to let takenDuringRecovery > suggest subxip is oversized. The discussion seems to have stopped here. As this is classified as a bug fix, I have moved this patch to next CF, waiting on author for the moment. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: OpenSSL 3.0.0 vs old branches @ 2023-02-06 16:13 Tom Lane <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Tom Lane @ 2023-02-06 16:13 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Andrew Dunstan <[email protected]> writes: > I recently moved crake to a new machine running Fedora 36, which has > OpenSSL 3.0.0. This causes the SSL tests to fail on branches earlier > than release 13, so I propose to backpatch commit f0d2c65f17 to the > release 11 and 12 branches. Hmm ... according to that commit message, Note that the minimum supported OpenSSL version is 1.0.1 as of 7b283d0e1d1d79bf1c962d790c94d2a53f3bb38a, so this does not introduce any new version requirements. So presumably, changing this test would break it for OpenSSL 0.9.8, which is still nominally supported in those branches. On the other hand, this test isn't run by default, so users would likely never notice anyway. On the whole, +1 for doing this (after the release freeze lifts). regards, tom lane ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: OpenSSL 3.0.0 vs old branches @ 2023-02-06 21:19 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 29+ messages in thread From: Andrew Dunstan @ 2023-02-06 21:19 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 2023-02-06 Mo 11:13, Tom Lane wrote: > Andrew Dunstan<[email protected]> writes: >> I recently moved crake to a new machine running Fedora 36, which has >> OpenSSL 3.0.0. This causes the SSL tests to fail on branches earlier >> than release 13, so I propose to backpatch commit f0d2c65f17 to the >> release 11 and 12 branches. > Hmm ... according to that commit message, > > Note that the minimum supported OpenSSL version is 1.0.1 as of > 7b283d0e1d1d79bf1c962d790c94d2a53f3bb38a, so this does not introduce > any new version requirements. > > So presumably, changing this test would break it for OpenSSL 0.9.8, > which is still nominally supported in those branches. On the other > hand, this test isn't run by default, so users would likely never > notice anyway. > > On the whole, +1 for doing this (after the release freeze lifts). > > Presumably we don't have any buildfarm animals running with such old versions of openssl, or they would be failing the same test on release >= 13. I'll push this in due course. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: OpenSSL 3.0.0 vs old branches @ 2023-02-06 22:01 Tom Lane <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Tom Lane @ 2023-02-06 22:01 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> Andrew Dunstan <[email protected]> writes: > On 2023-02-06 Mo 11:13, Tom Lane wrote: >> So presumably, changing this test would break it for OpenSSL 0.9.8, >> which is still nominally supported in those branches. On the other >> hand, this test isn't run by default, so users would likely never >> notice anyway. > Presumably we don't have any buildfarm animals running with such old > versions of openssl, or they would be failing the same test on release > >= 13. That test isn't run by default in the buildfarm either, no? But indeed, probably nobody in the community is testing such builds at all. I did have such setups on my old dinosaur BF animals, but they bit the dust last year for unrelated reasons. I wonder how realistic it is to claim that we still support those old OpenSSL versions. regards, tom lane ^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH] Review for wording on tablespaces on partitioned tables @ 2024-03-19 08:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Alvaro Herrera @ 2024-03-19 08:49 UTC (permalink / raw) Remove a redundant comment, and document pg_class.relam in catalogs.sgml. After commits a36c84c3e4a9, 87259588d0ab and others. Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/catalogs.sgml | 8 +++++--- src/backend/commands/tablecmds.c | 4 ---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 2f091ad09d..183669272b 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2011,9 +2011,11 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l (references <link linkend="catalog-pg-tablespace"><structname>pg_tablespace</structname></link>.<structfield>oid</structfield>) </para> <para> - The tablespace in which this relation is stored. If zero, - the database's default tablespace is implied. (Not meaningful - if the relation has no on-disk file.) + The tablespace in which this relation is stored; for partitioned + tables, the tablespace in which partitions are created + when one is not specified in the creation command. + If zero, the database's default tablespace is implied. + (Not meaningful if the relation has no on-disk file.) </para></entry> </row> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3ed0618b4e..6c0c899210 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -807,10 +807,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, } else if (stmt->partbound) { - /* - * For partitions, when no other tablespace is specified, we default - * the tablespace to the parent partitioned table's. - */ Assert(list_length(inheritOids) == 1); tablespaceId = get_rel_tablespace(linitial_oid(inheritOids)); } -- 2.39.2 --clmzxrgc4yg2hor6-- ^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH] Review for wording on tablespaces on partitioned tables @ 2024-03-19 08:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Alvaro Herrera @ 2024-03-19 08:49 UTC (permalink / raw) Remove a redundant comment, and document pg_class.relam in catalogs.sgml. After commits a36c84c3e4a9, 87259588d0ab and others. Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/catalogs.sgml | 8 +++++--- src/backend/commands/tablecmds.c | 4 ---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 2f091ad09d..183669272b 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2011,9 +2011,11 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l (references <link linkend="catalog-pg-tablespace"><structname>pg_tablespace</structname></link>.<structfield>oid</structfield>) </para> <para> - The tablespace in which this relation is stored. If zero, - the database's default tablespace is implied. (Not meaningful - if the relation has no on-disk file.) + The tablespace in which this relation is stored; for partitioned + tables, the tablespace in which partitions are created + when one is not specified in the creation command. + If zero, the database's default tablespace is implied. + (Not meaningful if the relation has no on-disk file.) </para></entry> </row> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3ed0618b4e..6c0c899210 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -807,10 +807,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, } else if (stmt->partbound) { - /* - * For partitions, when no other tablespace is specified, we default - * the tablespace to the parent partitioned table's. - */ Assert(list_length(inheritOids) == 1); tablespaceId = get_rel_tablespace(linitial_oid(inheritOids)); } -- 2.39.2 --clmzxrgc4yg2hor6-- ^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH] Review for wording on tablespaces on partitioned tables @ 2024-03-19 08:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 29+ messages in thread From: Alvaro Herrera @ 2024-03-19 08:49 UTC (permalink / raw) Remove a redundant comment, and document pg_class.relam in catalogs.sgml. After commits a36c84c3e4a9, 87259588d0ab and others. Discussion: https://postgr.es/m/[email protected] --- doc/src/sgml/catalogs.sgml | 8 +++++--- src/backend/commands/tablecmds.c | 4 ---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 2f091ad09d..183669272b 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2011,9 +2011,11 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l (references <link linkend="catalog-pg-tablespace"><structname>pg_tablespace</structname></link>.<structfield>oid</structfield>) </para> <para> - The tablespace in which this relation is stored. If zero, - the database's default tablespace is implied. (Not meaningful - if the relation has no on-disk file.) + The tablespace in which this relation is stored; for partitioned + tables, the tablespace in which partitions are created + when one is not specified in the creation command. + If zero, the database's default tablespace is implied. + (Not meaningful if the relation has no on-disk file.) </para></entry> </row> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3ed0618b4e..6c0c899210 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -807,10 +807,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, } else if (stmt->partbound) { - /* - * For partitions, when no other tablespace is specified, we default - * the tablespace to the parent partitioned table's. - */ Assert(list_length(inheritOids) == 1); tablespaceId = get_rel_tablespace(linitial_oid(inheritOids)); } -- 2.39.2 --clmzxrgc4yg2hor6-- ^ permalink raw reply [nested|flat] 29+ messages in thread
end of thread, other threads:[~2024-03-19 08:49 UTC | newest] Thread overview: 29+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-10-11 06:19 Error "initial slot snapshot too large" in create replication slot Dilip Kumar <[email protected]> 2021-10-11 10:59 ` Kyotaro Horiguchi <[email protected]> 2021-10-11 11:18 ` Dilip Kumar <[email protected]> 2021-10-12 04:59 ` Kyotaro Horiguchi <[email protected]> 2021-10-12 05:05 ` Kyotaro Horiguchi <[email protected]> 2021-10-12 06:00 ` Dilip Kumar <[email protected]> 2021-10-19 08:55 ` Dilip Kumar <[email protected]> 2021-11-02 11:10 ` Dilip Kumar <[email protected]> 2022-07-05 18:32 Re: Error "initial slot snapshot too large" in create replication slot Jacob Champion <[email protected]> 2022-07-19 02:55 ` Kyotaro Horiguchi <[email protected]> 2022-09-09 17:19 ` Robert Haas <[email protected]> 2022-09-12 21:53 ` Andres Freund <[email protected]> 2022-09-12 21:51 ` Andres Freund <[email protected]> 2022-09-13 01:30 ` Dilip Kumar <[email protected]> 2022-09-13 06:22 ` Kyotaro Horiguchi <[email protected]> 2022-09-13 06:38 ` Dilip Kumar <[email protected]> 2022-09-13 07:10 ` Kyotaro Horiguchi <[email protected]> 2022-09-13 07:30 ` Kyotaro Horiguchi <[email protected]> 2022-09-13 07:31 ` Kyotaro Horiguchi <[email protected]> 2022-09-13 06:45 ` Kyotaro Horiguchi <[email protected]> 2022-09-13 07:15 ` Kyotaro Horiguchi <[email protected]> 2022-09-13 08:31 ` Kyotaro Horiguchi <[email protected]> 2022-10-12 05:10 ` Michael Paquier <[email protected]> 2023-02-06 16:13 Re: OpenSSL 3.0.0 vs old branches Tom Lane <[email protected]> 2023-02-06 21:19 ` Re: OpenSSL 3.0.0 vs old branches Andrew Dunstan <[email protected]> 2023-02-06 22:01 ` Re: OpenSSL 3.0.0 vs old branches Tom Lane <[email protected]> 2024-03-19 08:49 [PATCH] Review for wording on tablespaces on partitioned tables Alvaro Herrera <[email protected]> 2024-03-19 08:49 [PATCH] Review for wording on tablespaces on partitioned tables Alvaro Herrera <[email protected]> 2024-03-19 08:49 [PATCH] Review for wording on tablespaces on partitioned tables Alvaro Herrera <[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