public inbox for [email protected]
help / color / mirror / Atom feedRe: Error "initial slot snapshot too large" in create replication slot
21+ messages / 7 participants
[nested] [flat]
* Re: Error "initial slot snapshot too large" in create replication slot
@ 2022-02-13 14:35 Yura Sokolov <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Yura Sokolov @ 2022-02-13 14:35 UTC (permalink / raw)
To: Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; +Cc: Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
В Пн, 07/02/2022 в 13:52 +0530, Dilip Kumar пишет:
> 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.
>
Good day.
I've looked to the patch. Personally I'd prefer dynamically resize
xip array. But I think there is issue with upgrade if replica source
is upgraded before destination, right?
Concerning patch, I think more comments should be written about new
usage case for `takenDuringRecovery`. May be this field should be renamed
at all?
And there are checks for `takenDuringRecovery` in `heapgetpage` and
`heapam_scan_sample_next_tuple`. Are this checks affected by the change?
Neither the preceding discussion nor commit message answer me.
-------
regards
Yura Sokolov
Postgres Professional
[email protected]
[email protected]
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot
@ 2022-04-01 05:44 Kyotaro Horiguchi <[email protected]>
parent: Yura Sokolov <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-04-01 05:44 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]
At Sun, 13 Feb 2022 17:35:38 +0300, Yura Sokolov <[email protected]> wrote in
> В Пн, 07/02/2022 в 13:52 +0530, Dilip Kumar пишет:
> > Right. I think the patch looks fine to me.
> >
>
> Good day.
>
> I've looked to the patch. Personally I'd prefer dynamically resize
> xip array. But I think there is issue with upgrade if replica source
> is upgraded before destination, right?
I don't think it is relevant. I think we don't convey snapshot via
streaming replication. But I think that expanding xip or subxip is
wrong, since it is tied with ProcArray structure. (Even though we
abuse the arrays in some situations, like this).
> Concerning patch, I think more comments should be written about new
> usage case for `takenDuringRecovery`. May be this field should be renamed
> at all?
I don't feel the need to rename it so much. It just signals that "this
snapshot is in the form for recovery". The more significant reason is
that I don't come up better name:p
And the comment is slightly modified and gets a pointer to detailed
comment.
+ * Although this snapshot is not acutally taken during recovery, all XIDs
+ * are stored in subxip. See GetSnapshotData for the details of that form
+ * of snapshot.
> And there are checks for `takenDuringRecovery` in `heapgetpage` and
> `heapam_scan_sample_next_tuple`. Are this checks affected by the change?
> Neither the preceding discussion nor commit message answer me.
The snapshot works correctly, but for the heapgetpage case, it foces
all_visible to be false. That unnecessarily prevents visibility check
from skipping.
An annoying thing in SnapBuildInitialSnapshot is that we don't know
the number of xids before looping over the xid range, and we don't
want to bother sorting top-level xids and subxids unless we have to do
so.
Is it better that we hassle in SnapBuildInitialSnapshot to create a
!takenDuringRecovery snapshot?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot
@ 2022-04-01 06:53 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Kyotaro Horiguchi @ 2022-04-01 06:53 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]
At Fri, 01 Apr 2022 14:44:30 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
> At Sun, 13 Feb 2022 17:35:38 +0300, Yura Sokolov <[email protected]> wrote in
> > And there are checks for `takenDuringRecovery` in `heapgetpage` and
> > `heapam_scan_sample_next_tuple`. Are this checks affected by the change?
> > Neither the preceding discussion nor commit message answer me.
>
> The snapshot works correctly, but for the heapgetpage case, it foces
> all_visible to be false. That unnecessarily prevents visibility check
> from skipping.
>
> An annoying thing in SnapBuildInitialSnapshot is that we don't know
> the number of xids before looping over the xid range, and we don't
> want to bother sorting top-level xids and subxids unless we have to do
> so.
>
> Is it better that we hassle in SnapBuildInitialSnapshot to create a
> !takenDuringRecovery snapshot?
So this is that. v5 creates a regular snapshot.
By the way, is there any chance this could be committed to 15?
Otherwise I'll immediately move this to the next CF.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] v5-0001-Create-correct-snapshot-during-CREATE_REPLICATION.patch (5.0K, ../../[email protected]/2-v5-0001-Create-correct-snapshot-during-CREATE_REPLICATION.patch)
download | inline diff:
From 6f3cfb58bc2c6294124831d6b410b0e51d067fb2 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Mon, 31 Jan 2022 15:03:33 +0900
Subject: [PATCH v5] Create correct snapshot during CREATE_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.
We could instead create a "takenDuringRecovery" snapshot, which later
leads to unnecessary visibility checks. Therefore we take trouble to
create a regular snapshot by identifying whether each xids is
top-level and storing it in the appropriate xid array.
Author: Dilip Kumar and Kyotaro Horiguchi
---
.../replication/logical/reorderbuffer.c | 20 +++++++++
src/backend/replication/logical/snapbuild.c | 45 +++++++++++++++----
src/include/replication/reorderbuffer.h | 1 +
3 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index c2d9be81fa..42adbe5f15 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3658,6 +3658,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..fa08e406e3 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -532,7 +532,10 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
Snapshot snap;
TransactionId xid;
TransactionId *newxip;
- int newxcnt = 0;
+ TransactionId *newsubxip;
+ int newxcnt;
+ int newsubxcnt;
+ bool overflowed = false;
Assert(!FirstSnapshotSet);
Assert(XactIsoLevel == XACT_REPEATABLE_READ);
@@ -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.
+ */
newxip = (TransactionId *)
palloc(sizeof(TransactionId) * GetMaxSnapshotXidCount());
+ newsubxip = (TransactionId *)
+ palloc(sizeof(TransactionId) * GetMaxSnapshotSubxidCount());
/*
* snapbuild.c builds transactions in an "inverted" manner, which means it
@@ -578,6 +589,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
* classical snapshot by marking all non-committed transactions as
* in-progress. This can be expensive.
*/
+ newxcnt = 0;
+ newsubxcnt = 0;
+
for (xid = snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax);)
{
void *test;
@@ -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;
+ }
}
TransactionIdAdvance(xid);
@@ -606,6 +632,9 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
snap->snapshot_type = SNAPSHOT_MVCC;
snap->xcnt = newxcnt;
snap->xip = newxip;
+ snap->subxcnt = newsubxcnt;
+ snap->subxip = newsubxip;
+ snap->suboverflowed = overflowed;
return snap;
}
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 0bcc150b33..b26c34ce9d 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -707,6 +707,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] 21+ messages in thread
* Re: Error "initial slot snapshot too large" in create replication slot
@ 2022-07-05 18:32 Jacob Champion <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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; 21+ 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] 21+ 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, 0 replies; 21+ 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] 21+ messages in thread
* [PATCH v14 1/2] Reusable decimalLength functions
@ 2023-02-17 09:17 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Dmitrii Dolgov @ 2023-02-17 09:17 UTC (permalink / raw)
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +-----------------------
src/include/utils/numutils.h | 67 ++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 471fbb7ee6..df7418cce7 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include <limits.h>
#include <ctype.h>
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 10000, 100000,
- 1000000, 10000000, 100000000,
- 1000000000
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
-static inline int
-decimalLength64(const uint64 v)
-{
- int t;
- static const uint64 PowersOfTen[] = {
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(10000), UINT64CONST(100000),
- UINT64CONST(1000000), UINT64CONST(10000000),
- UINT64CONST(100000000), UINT64CONST(1000000000),
- UINT64CONST(10000000000), UINT64CONST(100000000000),
- UINT64CONST(1000000000000), UINT64CONST(10000000000000),
- UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
- UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
- UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
diff --git a/src/include/utils/numutils.h b/src/include/utils/numutils.h
new file mode 100644
index 0000000000..876e64f2df
--- /dev/null
+++ b/src/include/utils/numutils.h
@@ -0,0 +1,67 @@
+/*-------------------------------------------------------------------------
+ *
+ * numutils.h
+ * Decimal length functions for numutils.c
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/numutils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef NUMUTILS_H
+#define NUMUTILS_H
+
+#include "common/int.h"
+#include "port/pg_bitutils.h"
+
+/*
+ * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+ */
+static inline int
+decimalLength32(const uint32 v)
+{
+ int t;
+ static const uint32 PowersOfTen[] = {
+ 1, 10, 100,
+ 1000, 10000, 100000,
+ 1000000, 10000000, 100000000,
+ 1000000000
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+static inline int
+decimalLength64(const uint64 v)
+{
+ int t;
+ static const uint64 PowersOfTen[] = {
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CONST(10000), UINT64CONST(100000),
+ UINT64CONST(1000000), UINT64CONST(10000000),
+ UINT64CONST(100000000), UINT64CONST(1000000000),
+ UINT64CONST(10000000000), UINT64CONST(100000000000),
+ UINT64CONST(1000000000000), UINT64CONST(10000000000000),
+ UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
+ UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
+ UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+#endif /* NUMUTILS_H */
--
2.32.0
--5yruzipouymq7ks3
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v14-0002-Prevent-jumbling-of-every-element-in-ArrayExpr.patch"
^ permalink raw reply [nested|flat] 21+ messages in thread
* [PATCH v15 2/4] Reusable decimalLength functions
@ 2023-02-17 09:17 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Dmitrii Dolgov @ 2023-02-17 09:17 UTC (permalink / raw)
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +-----------------------
src/include/utils/numutils.h | 67 ++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index a597e5ed796..02fe132f285 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include <limits.h>
#include <ctype.h>
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 10000, 100000,
- 1000000, 10000000, 100000000,
- 1000000000
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
-static inline int
-decimalLength64(const uint64 v)
-{
- int t;
- static const uint64 PowersOfTen[] = {
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(10000), UINT64CONST(100000),
- UINT64CONST(1000000), UINT64CONST(10000000),
- UINT64CONST(100000000), UINT64CONST(1000000000),
- UINT64CONST(10000000000), UINT64CONST(100000000000),
- UINT64CONST(1000000000000), UINT64CONST(10000000000000),
- UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
- UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
- UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
diff --git a/src/include/utils/numutils.h b/src/include/utils/numutils.h
new file mode 100644
index 00000000000..876e64f2df9
--- /dev/null
+++ b/src/include/utils/numutils.h
@@ -0,0 +1,67 @@
+/*-------------------------------------------------------------------------
+ *
+ * numutils.h
+ * Decimal length functions for numutils.c
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/numutils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef NUMUTILS_H
+#define NUMUTILS_H
+
+#include "common/int.h"
+#include "port/pg_bitutils.h"
+
+/*
+ * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+ */
+static inline int
+decimalLength32(const uint32 v)
+{
+ int t;
+ static const uint32 PowersOfTen[] = {
+ 1, 10, 100,
+ 1000, 10000, 100000,
+ 1000000, 10000000, 100000000,
+ 1000000000
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+static inline int
+decimalLength64(const uint64 v)
+{
+ int t;
+ static const uint64 PowersOfTen[] = {
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CONST(10000), UINT64CONST(100000),
+ UINT64CONST(1000000), UINT64CONST(10000000),
+ UINT64CONST(100000000), UINT64CONST(1000000000),
+ UINT64CONST(10000000000), UINT64CONST(100000000000),
+ UINT64CONST(1000000000000), UINT64CONST(10000000000000),
+ UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
+ UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
+ UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+#endif /* NUMUTILS_H */
--
2.41.0
--svnhsmawrtczycxj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v15-0003-Merge-constants-in-ArrayExpr-into-groups.patch"
^ permalink raw reply [nested|flat] 21+ messages in thread
* [PATCH v16 2/4] Reusable decimalLength functions
@ 2023-02-17 09:17 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Dmitrii Dolgov @ 2023-02-17 09:17 UTC (permalink / raw)
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +-----------------------
src/include/utils/numutils.h | 67 ++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index a597e5ed796..02fe132f285 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include <limits.h>
#include <ctype.h>
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 10000, 100000,
- 1000000, 10000000, 100000000,
- 1000000000
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
-static inline int
-decimalLength64(const uint64 v)
-{
- int t;
- static const uint64 PowersOfTen[] = {
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(10000), UINT64CONST(100000),
- UINT64CONST(1000000), UINT64CONST(10000000),
- UINT64CONST(100000000), UINT64CONST(1000000000),
- UINT64CONST(10000000000), UINT64CONST(100000000000),
- UINT64CONST(1000000000000), UINT64CONST(10000000000000),
- UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
- UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
- UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
diff --git a/src/include/utils/numutils.h b/src/include/utils/numutils.h
new file mode 100644
index 00000000000..876e64f2df9
--- /dev/null
+++ b/src/include/utils/numutils.h
@@ -0,0 +1,67 @@
+/*-------------------------------------------------------------------------
+ *
+ * numutils.h
+ * Decimal length functions for numutils.c
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/numutils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef NUMUTILS_H
+#define NUMUTILS_H
+
+#include "common/int.h"
+#include "port/pg_bitutils.h"
+
+/*
+ * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+ */
+static inline int
+decimalLength32(const uint32 v)
+{
+ int t;
+ static const uint32 PowersOfTen[] = {
+ 1, 10, 100,
+ 1000, 10000, 100000,
+ 1000000, 10000000, 100000000,
+ 1000000000
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+static inline int
+decimalLength64(const uint64 v)
+{
+ int t;
+ static const uint64 PowersOfTen[] = {
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CONST(10000), UINT64CONST(100000),
+ UINT64CONST(1000000), UINT64CONST(10000000),
+ UINT64CONST(100000000), UINT64CONST(1000000000),
+ UINT64CONST(10000000000), UINT64CONST(100000000000),
+ UINT64CONST(1000000000000), UINT64CONST(10000000000000),
+ UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
+ UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
+ UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+#endif /* NUMUTILS_H */
--
2.41.0
--pjkjqtbeiiimipyr
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v16-0003-Merge-constants-in-ArrayExpr-into-groups.patch"
^ permalink raw reply [nested|flat] 21+ messages in thread
* [PATCH v13 1/2] Reusable decimalLength functions
@ 2023-02-17 09:17 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Dmitrii Dolgov @ 2023-02-17 09:17 UTC (permalink / raw)
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +-----------------------
src/include/utils/numutils.h | 67 ++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 471fbb7ee6..df7418cce7 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include <limits.h>
#include <ctype.h>
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 10000, 100000,
- 1000000, 10000000, 100000000,
- 1000000000
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
-static inline int
-decimalLength64(const uint64 v)
-{
- int t;
- static const uint64 PowersOfTen[] = {
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(10000), UINT64CONST(100000),
- UINT64CONST(1000000), UINT64CONST(10000000),
- UINT64CONST(100000000), UINT64CONST(1000000000),
- UINT64CONST(10000000000), UINT64CONST(100000000000),
- UINT64CONST(1000000000000), UINT64CONST(10000000000000),
- UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
- UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
- UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
diff --git a/src/include/utils/numutils.h b/src/include/utils/numutils.h
new file mode 100644
index 0000000000..876e64f2df
--- /dev/null
+++ b/src/include/utils/numutils.h
@@ -0,0 +1,67 @@
+/*-------------------------------------------------------------------------
+ *
+ * numutils.h
+ * Decimal length functions for numutils.c
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/numutils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef NUMUTILS_H
+#define NUMUTILS_H
+
+#include "common/int.h"
+#include "port/pg_bitutils.h"
+
+/*
+ * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+ */
+static inline int
+decimalLength32(const uint32 v)
+{
+ int t;
+ static const uint32 PowersOfTen[] = {
+ 1, 10, 100,
+ 1000, 10000, 100000,
+ 1000000, 10000000, 100000000,
+ 1000000000
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+static inline int
+decimalLength64(const uint64 v)
+{
+ int t;
+ static const uint64 PowersOfTen[] = {
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CONST(10000), UINT64CONST(100000),
+ UINT64CONST(1000000), UINT64CONST(10000000),
+ UINT64CONST(100000000), UINT64CONST(1000000000),
+ UINT64CONST(10000000000), UINT64CONST(100000000000),
+ UINT64CONST(1000000000000), UINT64CONST(10000000000000),
+ UINT64CONST(100000000000000), UINT64CONST(1000000000000000),
+ UINT64CONST(10000000000000000), UINT64CONST(100000000000000000),
+ UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000)
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+#endif /* NUMUTILS_H */
--
2.32.0
--ydohaqx6er4mg5aj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v13-0002-Prevent-jumbling-of-every-element-in-ArrayExpr.patch"
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2023-02-17 09:17 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-13 14:35 Re: Error "initial slot snapshot too large" in create replication slot Yura Sokolov <[email protected]>
2022-04-01 05:44 ` Kyotaro Horiguchi <[email protected]>
2022-04-01 06:53 ` Kyotaro Horiguchi <[email protected]>
2022-07-05 18:32 ` 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]>
2023-02-17 09:17 [PATCH v16 2/4] Reusable decimalLength functions Dmitrii Dolgov <[email protected]>
2023-02-17 09:17 [PATCH v15 2/4] Reusable decimalLength functions Dmitrii Dolgov <[email protected]>
2023-02-17 09:17 [PATCH v14 1/2] Reusable decimalLength functions Dmitrii Dolgov <[email protected]>
2023-02-17 09:17 [PATCH v13 1/2] Reusable decimalLength functions Dmitrii Dolgov <[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