Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
At 2013-07-12 16:25:14 -0700, jeff.ja...@gmail.com wrote:
I think the reviewer of a performance patch should do some independent
testing of the performance, to replicate the author's numbers; and
hopefully with a few different scenarios.
You're quite right. I apologise for being lazy; doubly so because I
can't actually see any difference while running the test case with
the patches applied.
unpatched:
before: 1629.831391, 1559.793758, 1498.765018, 1639.384038
during: 37.434492, 37.044989, 37.112422, 36.950895
after : 46.591688, 46.341256, 46.042169, 46.260684
patched:
before: 1813.091975, 1798.923524, 1629.301356, 1606.849033
during: 37.344987, 37.207359, 37.406788, 37.316925
after : 46.657747, 46.537420, 46.746377, 46.577052
(before is before starting session 2; during is after session 2
inserts, but before it commits; after is after session 2 issues a
rollback.)
The timings above are with both xid_in_snapshot_cache.v1.patch and
cache_TransactionIdInProgress.v2.patch applied, but the numbers are
not noticeably different with only the first patch applied. After I
vacuum plan, the timings in both cases return to normal.
In a quick test with gdb (and also in perf report output), I didn't see
the following block in procarray.c being entered at all:
+ if (max_prepared_xacts == 0 pgprocno = 0
+ (TransactionIdEquals(xid, pxid) || TransactionIdEquals(xid,
cxid)))
+ {
…
I'll keep looking, but comments are welcome. I'm setting this back to
Needs Review in the meantime.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
At 2013-07-13 14:19:23 +0530, a...@2ndquadrant.com wrote:
The timings above are with both xid_in_snapshot_cache.v1.patch and
cache_TransactionIdInProgress.v2.patch applied
For anyone who wants to try to reproduce the results, here's the patch I
used, which is both patches above plus some typo fixes in comments.
-- Abhijit
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index c2f86ff..660fab0 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -840,6 +840,9 @@ TransactionIdIsInProgress(TransactionId xid)
TransactionId topxid;
int i,
j;
+ static int pgprocno = -1; /* cached last pgprocno */
+ static TransactionId pxid; /* cached last parent xid */
+ static TransactionId cxid; /* cached last child xid */
/*
* Don't bother checking a transaction older than RecentXmin; it could not
@@ -875,6 +878,60 @@ TransactionIdIsInProgress(TransactionId xid)
}
/*
+ * Check to see if we have cached the pgprocno for the xid we seek.
+ * We will have cached either pxid only or both pxid and cxid.
+ * So we check to see whether pxid or cxid matches the xid we seek,
+ * but then re-check just the parent pxid. If the PGXACT doesn't
+ * match then the transaction must be complete because an xid is
+ * only associated with one PGPROC/PGXACT during its lifetime
+ * except when we are using prepared transactions.
+ * Transaction wraparound is not a concern because we are checking
+ * the status of an xid we see in data and that might be running;
+ * anything very old will already be deleted or frozen. So stale
+ * cached values for pxid and cxid don't affect the correctness
+ * of the result.
+ */
+ if (max_prepared_xacts == 0 pgprocno = 0
+ (TransactionIdEquals(xid, pxid) || TransactionIdEquals(xid, cxid)))
+ {
+ volatile PGXACT *pgxact;
+
+ pgxact = allPgXact[pgprocno];
+
+ pxid = pgxact-xid;
+
+ /*
+ * XXX Can we test without the lock first? If the values don't
+ * match without the lock they never will match...
+ */
+
+ /*
+ * xid matches, so wait for the lock and re-check.
+ */
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ pxid = pgxact-xid;
+
+ if (TransactionIdIsValid(pxid) TransactionIdEquals(pxid, xid))
+ {
+ LWLockRelease(ProcArrayLock);
+ return true;
+ }
+
+ LWLockRelease(ProcArrayLock);
+
+ pxid = cxid = InvalidTransactionId;
+ return false;
+ }
+
+ /*
+ * Our cache didn't match, so zero the cxid so that when we reset pxid
+ * we don't become confused that the cxid and pxid still relate.
+ * cxid will be reset to something useful later, if approproate.
+ */
+ cxid = InvalidTransactionId;
+
+ /*
* If first time through, get workspace to remember main XIDs in. We
* malloc it permanently to avoid repeated palloc/pfree overhead.
*/
@@ -910,10 +967,12 @@ TransactionIdIsInProgress(TransactionId xid)
/* No shortcuts, gotta grovel through the array */
for (i = 0; i arrayP-numProcs; i++)
{
- int pgprocno = arrayP-pgprocnos[i];
- volatile PGPROC *proc = allProcs[pgprocno];
- volatile PGXACT *pgxact = allPgXact[pgprocno];
- TransactionId pxid;
+ volatile PGPROC *proc;
+ volatile PGXACT *pgxact;
+
+ pgprocno = arrayP-pgprocnos[i];
+ proc = allProcs[pgprocno];
+ pgxact = allPgXact[pgprocno];
/* Ignore my own proc --- dealt with it above */
if (proc == MyProc)
@@ -948,7 +1007,7 @@ TransactionIdIsInProgress(TransactionId xid)
for (j = pgxact-nxids - 1; j = 0; j--)
{
/* Fetch xid just once - see GetNewTransactionId */
- TransactionId cxid = proc-subxids.xids[j];
+ cxid = proc-subxids.xids[j];
if (TransactionIdEquals(cxid, xid))
{
@@ -975,6 +1034,14 @@ TransactionIdIsInProgress(TransactionId xid)
*/
if (RecoveryInProgress())
{
+ /*
+ * Hot Standby doesn't use pgprocno, so we can clear the cache.
+ *
+ * XXX we could try to remember the offset into the KnownAssignedXids
+ * array, which is a possibility for another day.
+ */
+ pxid = cxid = InvalidTransactionId;
+
/* none of the PGXACT entries should have XIDs in hot standby mode */
Assert(nxids == 0);
@@ -999,6 +1066,12 @@ TransactionIdIsInProgress(TransactionId xid)
LWLockRelease(ProcArrayLock);
/*
+ * After this point we don't remember pgprocno, so the cache is
+ * no use to us anymore.
+ */
+ pxid = cxid = InvalidTransactionId;
+
+ /*
* If none of the relevant caches overflowed, we know the Xid is not
* running without even looking at pg_subtrans.
*/
@@ -1509,6 +1582,9 @@ GetSnapshotData(Snapshot snapshot)
snapshot-curcid = GetCurrentCommandId(false);
+ /* Initialise the single xid cache for this snapshot */
+ snapshot-xid_in_snapshot = InvalidTransactionId;
+
/*
* This is a new snapshot, so set both refcounts are zero, and mark it as
* not copied in persistent memory.
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 55563ea..dc4bf54 100644
--- a/src/backend/utils/time/tqual.c
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
At 2013-07-11 17:47:58 -0700, j...@agliodbs.com wrote: So, where are we with this patch, then? It's ready for committer. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
On Wed, Jul 10, 2013 at 10:09 PM, Abhijit Menon-Sen a...@2ndquadrant.com wrote: At 2013-07-10 09:47:34 -0700, j...@agliodbs.com wrote: Due to the apparent lack of performance testing, I'm setting this back to needs review. The original submission (i.e. the message linked from the CF page) includes test results that showed a clear performance improvement. I think the reviewer of a performance patch should do some independent testing of the performance, to replicate the author's numbers; and hopefully with a few different scenarios. Cheers, Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
On 07/10/2013 10:09 PM, Abhijit Menon-Sen wrote: At 2013-07-10 09:47:34 -0700, j...@agliodbs.com wrote: Due to the apparent lack of performance testing, I'm setting this back to needs review. The original submission (i.e. the message linked from the CF page) includes test results that showed a clear performance improvement. Here's an excerpt: I didn't see that, and nobody replied to my email. So, where are we with this patch, then? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
On 07/08/2013 10:11 AM, Josh Berkus wrote: On 06/23/2013 09:43 PM, Abhijit Menon-Sen wrote: (Cc: to pgsql-performance dropped, pgsql-hackers added.) At 2013-05-06 09:14:01 +0100, si...@2ndquadrant.com wrote: New version of patch attached which fixes a few bugs. I read the patch, but only skimmed the earlier discussion about it. In isolation, I can say that the patch applies cleanly and looks sensible for what it does (i.e., cache pgprocno to speed up repeated calls to TransactionIdIsInProgress(somexid)). In that sense, it's ready for committer, but I don't know if there's a better/more complete/etc. way to address the original problem. Has this patch had performance testing? Because of the list crossover I don't have any information on that. Due to the apparent lack of performance testing, I'm setting this back to needs review. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
At 2013-07-10 09:47:34 -0700, j...@agliodbs.com wrote: Due to the apparent lack of performance testing, I'm setting this back to needs review. The original submission (i.e. the message linked from the CF page) includes test results that showed a clear performance improvement. Here's an excerpt: OK, here's a easily reproducible test... Prep: DROP TABLE IF EXISTS plan; CREATE TABLE plan ( id INTEGER NOT NULL, typ INTEGER NOT NULL, dat TIMESTAMP, val TEXT NOT NULL ); insert into plan select generate_series(1,10), 0, current_timestamp, 'some texts'; CREATE UNIQUE INDEX plan_id ON plan(id); CREATE INDEX plan_dat ON plan(dat); testcase.pgb select count(*) from plan where dat is null and typ = 3; Session 1: pgbench -n -f testcase.pgb -t 100 Session 2: BEGIN; insert into plan select 100 + generate_series(1, 10), 3, NULL, 'b'; Transaction rate in Session 1: (in tps) (a) before we run Session 2: Current: 5600tps Patched: 5600tps (b) after Session 2 has run, yet before transaction end Current: 56tps Patched: 65tps (c ) after Session 2 has aborted Current/Patched: 836, 1028, 5400tps VACUUM improves timing again New version of patch attached which fixes a few bugs. Patch works and improves things, but we're still swamped by the block accesses via the index. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
On 06/23/2013 09:43 PM, Abhijit Menon-Sen wrote: (Cc: to pgsql-performance dropped, pgsql-hackers added.) At 2013-05-06 09:14:01 +0100, si...@2ndquadrant.com wrote: New version of patch attached which fixes a few bugs. I read the patch, but only skimmed the earlier discussion about it. In isolation, I can say that the patch applies cleanly and looks sensible for what it does (i.e., cache pgprocno to speed up repeated calls to TransactionIdIsInProgress(somexid)). In that sense, it's ready for committer, but I don't know if there's a better/more complete/etc. way to address the original problem. Has this patch had performance testing? Because of the list crossover I don't have any information on that. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PERFORM] In progress INSERT wrecks plans on table
(Cc: to pgsql-performance dropped, pgsql-hackers added.) At 2013-05-06 09:14:01 +0100, si...@2ndquadrant.com wrote: New version of patch attached which fixes a few bugs. I read the patch, but only skimmed the earlier discussion about it. In isolation, I can say that the patch applies cleanly and looks sensible for what it does (i.e., cache pgprocno to speed up repeated calls to TransactionIdIsInProgress(somexid)). In that sense, it's ready for committer, but I don't know if there's a better/more complete/etc. way to address the original problem. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
