Re: [PATCHES] TransactionIdIsInProgress() cache
I didn't check whether your transformation is correct, but if so then it
can be changed like this and save the extra XidDidCommit call:
xvac_committed = TransactionIdDidCommit(xvac);
if (xvac_committed)
{
/* committed */
}
else if (!TransactionIdIsInProgress(xvac))
{
if (xvac_committed)
{
/* committed */
}
else
{
/* aborted */
}
}
else
{
/* in-progress */
}
--
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
On Tue, Mar 11, 2008 at 6:37 PM, Alvaro Herrera
[EMAIL PROTECTED] wrote:
I didn't check whether your transformation is correct, but if so then it
can be changed like this and save the extra XidDidCommit call:
xvac_committed = TransactionIdDidCommit(xvac);
if (xvac_committed)
{
/* committed */
}
else if (!TransactionIdIsInProgress(xvac))
{
if (xvac_committed)
{
/* committed */
}
else
{
/* aborted */
}
}
else
{
/* in-progress */
}
I doubt if the transformation is correct. If xvac_committed is true, why would
one even get into the else part ?
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
Alvaro Herrera wrote:
I didn't check whether your transformation is correct, but if so then it
can be changed like this and save the extra XidDidCommit call:
xvac_committed = TransactionIdDidCommit(xvac);
if (xvac_committed)
{
/* committed */
}
else if (!TransactionIdIsInProgress(xvac))
{
if (xvac_committed)
{
/* committed */
}
else
{
/* aborted */
}
}
else
{
/* in-progress */
}
Nope, that's not good. Per comments in tqual.c, you have to call
TransactionIdIsInProgress *before* TransactionIdDidCommit, to avoid this
race condition:
1. Xact A inserts a record
2. Xact B does TransactionIdDidCommit, which retuns false because it's
still in progress
3. Xact B commits
4. Xact B does TransactionIdIsInProgress to see if A is still in
progress. It returns false. We conclude that A aborted, while it
actually committed.
My proposal was basically to add an extra TransactionIdDidCommit call
before the TransactionIdIsInProgress call, in the hope that it returns
true and we can skip the rest.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
Heikki Linnakangas wrote: Nope, that's not good. Per comments in tqual.c, you have to call TransactionIdIsInProgress *before* TransactionIdDidCommit, to avoid this race condition: 1. Xact A inserts a record 2. Xact B does TransactionIdDidCommit, which retuns false because it's still in progress 3. Xact B commits 4. Xact B does TransactionIdIsInProgress to see if A is still in progress. It returns false. We conclude that A aborted, while it actually committed. Ah, right -- I knew there was a reason for the other coding, I just didn't remember what it was and based my transformation purely on the snippet you posted. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
Heikki Linnakangas [EMAIL PROTECTED] writes: I presume the case where this would help would be when you populate a large table, with COPY for example, and the run a seq scan on it. As all rows in the table have the same xmin, you keep testing for the same XID over and over again. Yeah, that's the reasoning for having the single-element cache in transam.c in the first place. Mass updates and mass deletes would also result in a lot of probes of the same XID on the next examination of the table. Simon's idea looks sane, although TransactionIdIsKnownNotInProgress seems a kinda weird name ... it feels like a double negative to me, although strictly speaking it's not. Maybe instead TransactionIdIsKnownCompleted? To matter from scalability point of view, there would need to be a lot of concurrent activity that compete for the lock. I think it's probably useful just from a cycles-saved point of view. And we do know that ProcArrayLock is a hot spot. Hmm. The pattern in tqual.c is: ... We could do this instead: ... I dislike this alternative because tqual.c is mind-bendingly complex already. Simon's approach hides the issue somewhere else where there aren't so many code paths to keep straight. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
On Tue, 2008-03-11 at 12:57 +, Heikki Linnakangas wrote:
We could do this instead:
if (TransactionIdDidCommit(xvac))
{
/* committed */
}
else if (!TransactionIdIsInProgress(xvac))
{
if (TransactionIdDidCommit(xvac))
{
/* committed */
}
else
{
/* aborted */
}
}
else
{
/* in-progress */
}
(hopefully there would be a way to macroize that or something to avoid
bloating the code any more.)
For committed transactions, this would save the
TransactionIdIsInProgress call completely, whether or not it's in the
one-item cache. The tradeoff is that we would have to call
TransactionIdDidCommit twice for aborted transactions.
I thought about doing it the way you suggest but
TransactionIdIsInProgress is the offending code, so thats the part I
tuned. TransactionIdDidCommit uses the single item cache so it seemed
easier to make isolated changes as proposed rather than touching tqual
code in multiple places to do exactly the same thing.
No, I haven't done any formal performance testing on it. It seemed an
obvious hole that everyone would agree we should avoid, since we can do
it so cheaply: one integer comparison against scanning the whole
procarray and taking an LWlock.
--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com
PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
On Tue, 2008-03-11 at 13:15 -0400, Tom Lane wrote: Maybe instead TransactionIdIsKnownCompleted? Works for me. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com PostgreSQL UK 2008 Conference: http://www.postgresql.org.uk -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
Simon Riggs [EMAIL PROTECTED] writes: No, I haven't done any formal performance testing on it. It seemed an obvious hole that everyone would agree we should avoid, since we can do it so cheaply: one integer comparison against scanning the whole procarray and taking an LWlock. [ after reading the patch a bit closer... ] Actually, it's not as obvious as all that. TransactionIdIsInProgress already falls out before the proposed test for any XID RecentXmin, which means that in reality it's fairly probable that the target XID is running. So while this test may not cost much, it's not clear that it really buys much either. I'm going to go ahead and apply it, but it'd be good if someone would verify that it at least doesn't cost anything on some real benchmarks. BTW, I think we should put it in front of, not after, the TransactionIdIsCurrentTransactionId test, since as was just being discussed that could have nontrivial execution time. (I'll go look at Heikki's improvement on that next, but it'll still be not-free if there's lots of subtransactions.) regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
I wrote: [ after reading the patch a bit closer... ] Actually, it's not as obvious as all that. TransactionIdIsInProgress already falls out before the proposed test for any XID RecentXmin, which means that in reality it's fairly probable that the target XID is running. So while this test may not cost much, it's not clear that it really buys much either. I realized that a pretty simple test was already built into the code: look at the stats gathered by XIDCACHE_DEBUG. Here are the totals for a set of parallel regression tests: xmin: 185668 known: 25728 myxact: 5521 latest: 239 mainxid:484 childxid: 0 nooflo: 3331 slow: 0 So even though the xmin test takes out a lot, this seems worth doing. Some more-formal performance testing might still be a good idea though. For completeness, the raw per-backend stats are attached. Unsurprisingly, the wins come in bunches. regards, tom lane XidCache: xmin: 1, known: 0, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 0, slow: 0 XidCache: xmin: 1, known: 0, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 0, slow: 0 XidCache: xmin: 0, known: 0, myxact: 4, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 0, known: 9, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 0, known: 14, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 17, slow: 0 XidCache: xmin: 1, known: 3, myxact: 1, latest: 0, mainxid: 0, childxid: 0, nooflo: 3, slow: 0 XidCache: xmin: 0, known: 11, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 11, slow: 0 XidCache: xmin: 18, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 0, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 7, slow: 0 XidCache: xmin: 0, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 7, slow: 0 XidCache: xmin: 0, known: 2, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 7, slow: 0 XidCache: xmin: 0, known: 6, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 8, slow: 0 XidCache: xmin: 32, known: 4, myxact: 2, latest: 0, mainxid: 0, childxid: 0, nooflo: 5, slow: 0 XidCache: xmin: 0, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 15, slow: 0 XidCache: xmin: 25, known: 2, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 2, slow: 0 XidCache: xmin: 6, known: 5, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 13, slow: 0 XidCache: xmin: 103, known: 2, myxact: 0, latest: 2, mainxid: 0, childxid: 0, nooflo: 5, slow: 0 XidCache: xmin: 174, known: 0, myxact: 13, latest: 0, mainxid: 1, childxid: 0, nooflo: 7, slow: 0 XidCache: xmin: 94, known: 54, myxact: 1, latest: 2, mainxid: 0, childxid: 0, nooflo: 28, slow: 0 XidCache: xmin: 97, known: 89, myxact: 19, latest: 0, mainxid: 2, childxid: 0, nooflo: 50, slow: 0 XidCache: xmin: 2401, known: 12, myxact: 17, latest: 2, mainxid: 0, childxid: 0, nooflo: 16, slow: 0 XidCache: xmin: 156, known: 11, myxact: 15, latest: 0, mainxid: 0, childxid: 0, nooflo: 5, slow: 0 XidCache: xmin: 137, known: 6, myxact: 9, latest: 0, mainxid: 0, childxid: 0, nooflo: 4, slow: 0 XidCache: xmin: 8, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 6, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 59, known: 3, myxact: 5, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 7, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 5, slow: 0 XidCache: xmin: 0, known: 0, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 0, slow: 0 XidCache: xmin: 43, known: 1, myxact: 7, latest: 0, mainxid: 0, childxid: 0, nooflo: 1, slow: 0 XidCache: xmin: 1, known: 3, myxact: 1, latest: 0, mainxid: 0, childxid: 0, nooflo: 5, slow: 0 XidCache: xmin: 1, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 7, slow: 0 XidCache: xmin: 0, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 8, slow: 0 XidCache: xmin: 0, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 14, slow: 0 XidCache: xmin: 0, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 7, slow: 0 XidCache: xmin: 6, known: 4, myxact: 1, latest: 0, mainxid: 0, childxid: 0, nooflo: 19, slow: 0 XidCache: xmin: 0, known: 3, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 11, slow: 0 XidCache: xmin: 12, known: 8, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 14, slow: 0 XidCache: xmin: 0, known: 0, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 0, slow: 0 XidCache: xmin: 42, known: 7, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 35, slow: 0 XidCache: xmin: 15, known: 1, myxact: 0, latest: 0, mainxid: 0, childxid: 0, nooflo: 2, slow: 0 XidCache: xmin: 308, known: 8, myxact: 67, latest: 0, mainxid: 0, childxid: 0, nooflo: 2, slow: 0 XidCache: xmin: 51,
Re: [PATCHES] TransactionIdIsInProgress() cache
Simon Riggs [EMAIL PROTECTED] writes: We currently have a single item cache of the last checked TransactionId, which optimises the call to TransactionIdDidCommit() during HeapTupleSatisfiesMVCC() and partners. Before we call TransactionIdDidCommit() we always call TransactionIdIsInProgress(). TransactionIdIsInProgress() doesn't check the single item cache, so even if we have just checked for this xid, we will check it again. Since this function takes ProcArrayLock and may be called while holding other locks it will improve scalability if we can skip the call, for the cost of an integer comparison. Following patch implements fastpath in TransactionIdIsInProgress() to utilise single item cache. Applied with minor adjustments and some desultory comment-improvement. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] TransactionIdIsInProgress() cache
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches It will be applied as soon as one of the PostgreSQL committers reviews and approves it. --- Simon Riggs wrote: We currently have a single item cache of the last checked TransactionId, which optimises the call to TransactionIdDidCommit() during HeapTupleSatisfiesMVCC() and partners. Before we call TransactionIdDidCommit() we always call TransactionIdIsInProgress(). TransactionIdIsInProgress() doesn't check the single item cache, so even if we have just checked for this xid, we will check it again. Since this function takes ProcArrayLock and may be called while holding other locks it will improve scalability if we can skip the call, for the cost of an integer comparison. Following patch implements fastpath in TransactionIdIsInProgress() to utilise single item cache. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com [ Attachment, skipping... ] ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
