Thread: Static snapshot data
Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
free'd for every transaction or statement, respectively. This patch
puts these data structures into static memory, thus saving a few CPU
cycles and two malloc calls per transaction or (in isolation level
READ COMMITTED) per query.
Servus
Manfred
diff -ruN ../base/src/backend/storage/ipc/sinval.c src/backend/storage/ipc/sinval.c
--- ../base/src/backend/storage/ipc/sinval.c 2003-04-04 15:45:37.000000000 +0200
+++ src/backend/storage/ipc/sinval.c 2003-05-08 21:39:15.000000000 +0200
@@ -305,9 +305,8 @@
*----------
*/
Snapshot
-GetSnapshotData(bool serializable)
+GetSnapshotData(Snapshot snapshot, bool serializable)
{
- Snapshot snapshot = (Snapshot) malloc(sizeof(SnapshotData));
SISeg *segP = shmInvalBuffer;
ProcState *stateP = segP->procState;
TransactionId xmin;
@@ -316,18 +315,29 @@
int index;
int count = 0;
- if (snapshot == NULL)
- elog(ERROR, "Memory exhausted in GetSnapshotData");
+ Assert(snapshot != NULL);
/*
* Allocating space for MaxBackends xids is usually overkill;
* lastBackend would be sufficient. But it seems better to do the
* malloc while not holding the lock, so we can't look at lastBackend.
+ *
+ * if (snapshot->xip != NULL)
+ * no need to free and reallocate xip;
+ *
+ * We can reuse the old xip array, because MaxBackends does not change
+ * at runtime.
*/
- snapshot->xip = (TransactionId *)
- malloc(MaxBackends * sizeof(TransactionId));
if (snapshot->xip == NULL)
- elog(ERROR, "Memory exhausted in GetSnapshotData");
+ {
+ /*
+ * First call for this snapshot
+ */
+ snapshot->xip = (TransactionId *)
+ malloc(MaxBackends * sizeof(TransactionId));
+ if (snapshot->xip == NULL)
+ elog(ERROR, "Memory exhausted in GetSnapshotData");
+ }
globalxmin = xmin = GetCurrentTransactionId();
diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
--- ../base/src/backend/utils/time/tqual.c 2003-04-04 15:45:45.000000000 +0200
+++ src/backend/utils/time/tqual.c 2003-05-08 21:45:55.000000000 +0200
@@ -30,6 +30,8 @@
static SnapshotData SnapshotDirtyData;
Snapshot SnapshotDirty = &SnapshotDirtyData;
+static SnapshotData QuerySnapshotData;
+static SnapshotData SerializableSnapshotData;
Snapshot QuerySnapshot = NULL;
Snapshot SerializableSnapshot = NULL;
@@ -941,23 +943,16 @@
/* 1st call in xaction? */
if (SerializableSnapshot == NULL)
{
- SerializableSnapshot = GetSnapshotData(true);
+ SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true);
QuerySnapshot = SerializableSnapshot;
Assert(QuerySnapshot != NULL);
return;
}
- if (QuerySnapshot != SerializableSnapshot)
- {
- free(QuerySnapshot->xip);
- free(QuerySnapshot);
- QuerySnapshot = NULL;
- }
-
if (XactIsoLevel == XACT_SERIALIZABLE)
QuerySnapshot = SerializableSnapshot;
else
- QuerySnapshot = GetSnapshotData(false);
+ QuerySnapshot = GetSnapshotData(&QuerySnapshotData, false);
Assert(QuerySnapshot != NULL);
}
@@ -1003,19 +998,11 @@
void
FreeXactSnapshot(void)
{
- if (QuerySnapshot != NULL && QuerySnapshot != SerializableSnapshot)
- {
- free(QuerySnapshot->xip);
- free(QuerySnapshot);
- }
-
+ /*
+ * We do not free(QuerySnapshot->xip);
+ * or free(SerializableSnapshot->xip);
+ * they will be reused soon
+ */
QuerySnapshot = NULL;
-
- if (SerializableSnapshot != NULL)
- {
- free(SerializableSnapshot->xip);
- free(SerializableSnapshot);
- }
-
SerializableSnapshot = NULL;
}
diff -ruN ../base/src/include/utils/tqual.h src/include/utils/tqual.h
--- ../base/src/include/utils/tqual.h 2003-04-04 15:45:50.000000000 +0200
+++ src/include/utils/tqual.h 2003-05-08 21:40:04.000000000 +0200
@@ -113,7 +113,7 @@
extern HTSV_Result HeapTupleSatisfiesVacuum(HeapTupleHeader tuple,
TransactionId OldestXmin);
-extern Snapshot GetSnapshotData(bool serializable);
+extern Snapshot GetSnapshotData(Snapshot snapshot, bool serializable);
extern void SetQuerySnapshot(void);
extern Snapshot CopyQuerySnapshot(void);
extern void FreeXactSnapshot(void);
Manfred Koizar <mkoi-pg@aon.at> writes:
> Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
> free'd for every transaction or statement, respectively. This patch
> puts these data structures into static memory, thus saving a few CPU
> cycles and two malloc calls per transaction or (in isolation level
> READ COMMITTED) per query.
I do not like this patch. Two mallocs per transaction is an utterly
insignificant overhead. And isn't the patch going in quite the wrong
direction for nested transactions? The assumption that there's
never more than one QuerySnapshot seems to fly in the face of that...
regards, tom lane
On Fri, 09 May 2003 23:08:38 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote: >I do not like this patch. That's not a surprise, but ... >Two mallocs per transaction is an utterly insignificant overhead. 2002-05-25 you said: "a cycle saved is a cycle earned." More importantly the patch makes it clearer that there is always at most one instance of SerializableSnapshotData and [current] QuerySnapshotData. >And isn't the patch going in quite the wrong >direction for nested transactions? Our (Alvaro's and my) current understanding is that snapshots are not influenced by nested transactions. ad SerializableSnapshot: A subtransaction operates in the context of the main transaction. We do not want to see different snapshots at different nesting levels. > The assumption that there's >never more than one QuerySnapshot seems to fly in the face of that... ad QuerySnapshot: If there is a need for a query snapshot stack, then it is not because of nested transactions but due to queries invoking functions containing queries ... This is currently handled by CopyQuerySnapshot(), AFAIK. Servus Manfred
Manfred Koizar <mkoi-pg@aon.at> writes:
>> And isn't the patch going in quite the wrong
>> direction for nested transactions?
> Our (Alvaro's and my) current understanding is that snapshots are not
> influenced by nested transactions.
What was that long article Alvaro posted yesterday, then? He definitely
came to the conclusion that nested transactions need different
QuerySnapshots, and I think it was still open whether they need
different SerializableSnapshots.
regards, tom lane
After lots of discussion, it seems this is to be applied.
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Manfred Koizar wrote:
> Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
> free'd for every transaction or statement, respectively. This patch
> puts these data structures into static memory, thus saving a few CPU
> cycles and two malloc calls per transaction or (in isolation level
> READ COMMITTED) per query.
>
> Servus
> Manfred
> diff -ruN ../base/src/backend/storage/ipc/sinval.c src/backend/storage/ipc/sinval.c
> --- ../base/src/backend/storage/ipc/sinval.c 2003-04-04 15:45:37.000000000 +0200
> +++ src/backend/storage/ipc/sinval.c 2003-05-08 21:39:15.000000000 +0200
> @@ -305,9 +305,8 @@
> *----------
> */
> Snapshot
> -GetSnapshotData(bool serializable)
> +GetSnapshotData(Snapshot snapshot, bool serializable)
> {
> - Snapshot snapshot = (Snapshot) malloc(sizeof(SnapshotData));
> SISeg *segP = shmInvalBuffer;
> ProcState *stateP = segP->procState;
> TransactionId xmin;
> @@ -316,18 +315,29 @@
> int index;
> int count = 0;
>
> - if (snapshot == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + Assert(snapshot != NULL);
>
> /*
> * Allocating space for MaxBackends xids is usually overkill;
> * lastBackend would be sufficient. But it seems better to do the
> * malloc while not holding the lock, so we can't look at lastBackend.
> + *
> + * if (snapshot->xip != NULL)
> + * no need to free and reallocate xip;
> + *
> + * We can reuse the old xip array, because MaxBackends does not change
> + * at runtime.
> */
> - snapshot->xip = (TransactionId *)
> - malloc(MaxBackends * sizeof(TransactionId));
> if (snapshot->xip == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + {
> + /*
> + * First call for this snapshot
> + */
> + snapshot->xip = (TransactionId *)
> + malloc(MaxBackends * sizeof(TransactionId));
> + if (snapshot->xip == NULL)
> + elog(ERROR, "Memory exhausted in GetSnapshotData");
> + }
>
> globalxmin = xmin = GetCurrentTransactionId();
>
> diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
> --- ../base/src/backend/utils/time/tqual.c 2003-04-04 15:45:45.000000000 +0200
> +++ src/backend/utils/time/tqual.c 2003-05-08 21:45:55.000000000 +0200
> @@ -30,6 +30,8 @@
> static SnapshotData SnapshotDirtyData;
> Snapshot SnapshotDirty = &SnapshotDirtyData;
>
> +static SnapshotData QuerySnapshotData;
> +static SnapshotData SerializableSnapshotData;
> Snapshot QuerySnapshot = NULL;
> Snapshot SerializableSnapshot = NULL;
>
> @@ -941,23 +943,16 @@
> /* 1st call in xaction? */
> if (SerializableSnapshot == NULL)
> {
> - SerializableSnapshot = GetSnapshotData(true);
> + SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true);
> QuerySnapshot = SerializableSnapshot;
> Assert(QuerySnapshot != NULL);
> return;
> }
>
> - if (QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - QuerySnapshot = NULL;
> - }
> -
> if (XactIsoLevel == XACT_SERIALIZABLE)
> QuerySnapshot = SerializableSnapshot;
> else
> - QuerySnapshot = GetSnapshotData(false);
> + QuerySnapshot = GetSnapshotData(&QuerySnapshotData, false);
>
> Assert(QuerySnapshot != NULL);
> }
> @@ -1003,19 +998,11 @@
> void
> FreeXactSnapshot(void)
> {
> - if (QuerySnapshot != NULL && QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - }
> -
> + /*
> + * We do not free(QuerySnapshot->xip);
> + * or free(SerializableSnapshot->xip);
> + * they will be reused soon
> + */
> QuerySnapshot = NULL;
> -
> - if (SerializableSnapshot != NULL)
> - {
> - free(SerializableSnapshot->xip);
> - free(SerializableSnapshot);
> - }
> -
> SerializableSnapshot = NULL;
> }
> diff -ruN ../base/src/include/utils/tqual.h src/include/utils/tqual.h
> --- ../base/src/include/utils/tqual.h 2003-04-04 15:45:50.000000000 +0200
> +++ src/include/utils/tqual.h 2003-05-08 21:40:04.000000000 +0200
> @@ -113,7 +113,7 @@
> extern HTSV_Result HeapTupleSatisfiesVacuum(HeapTupleHeader tuple,
> TransactionId OldestXmin);
>
> -extern Snapshot GetSnapshotData(bool serializable);
> +extern Snapshot GetSnapshotData(Snapshot snapshot, bool serializable);
> extern void SetQuerySnapshot(void);
> extern Snapshot CopyQuerySnapshot(void);
> extern void FreeXactSnapshot(void);
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> After lots of discussion, it seems this is to be applied.
I'm still concerned that this will create problems for nested
transactions, while saving only an insignificant number of cycles per
transaction. I would suggest putting the idea on hold until the
dust has settled from nested transactions. If it's still workable
after that feature is complete, we can shave cycles then.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > After lots of discussion, it seems this is to be applied. > > I'm still concerned that this will create problems for nested > transactions, while saving only an insignificant number of cycles per > transaction. I would suggest putting the idea on hold until the > dust has settled from nested transactions. If it's still workable > after that feature is complete, we can shave cycles then. My feeling was that Manfred/Alvero are dealing with nested transactions, so if they both want the patch applied, we apply it. If they want it back, they can grab it from CVS. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks.
---------------------------------------------------------------------------
Manfred Koizar wrote:
> Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
> free'd for every transaction or statement, respectively. This patch
> puts these data structures into static memory, thus saving a few CPU
> cycles and two malloc calls per transaction or (in isolation level
> READ COMMITTED) per query.
>
> Servus
> Manfred
> diff -ruN ../base/src/backend/storage/ipc/sinval.c src/backend/storage/ipc/sinval.c
> --- ../base/src/backend/storage/ipc/sinval.c 2003-04-04 15:45:37.000000000 +0200
> +++ src/backend/storage/ipc/sinval.c 2003-05-08 21:39:15.000000000 +0200
> @@ -305,9 +305,8 @@
> *----------
> */
> Snapshot
> -GetSnapshotData(bool serializable)
> +GetSnapshotData(Snapshot snapshot, bool serializable)
> {
> - Snapshot snapshot = (Snapshot) malloc(sizeof(SnapshotData));
> SISeg *segP = shmInvalBuffer;
> ProcState *stateP = segP->procState;
> TransactionId xmin;
> @@ -316,18 +315,29 @@
> int index;
> int count = 0;
>
> - if (snapshot == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + Assert(snapshot != NULL);
>
> /*
> * Allocating space for MaxBackends xids is usually overkill;
> * lastBackend would be sufficient. But it seems better to do the
> * malloc while not holding the lock, so we can't look at lastBackend.
> + *
> + * if (snapshot->xip != NULL)
> + * no need to free and reallocate xip;
> + *
> + * We can reuse the old xip array, because MaxBackends does not change
> + * at runtime.
> */
> - snapshot->xip = (TransactionId *)
> - malloc(MaxBackends * sizeof(TransactionId));
> if (snapshot->xip == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + {
> + /*
> + * First call for this snapshot
> + */
> + snapshot->xip = (TransactionId *)
> + malloc(MaxBackends * sizeof(TransactionId));
> + if (snapshot->xip == NULL)
> + elog(ERROR, "Memory exhausted in GetSnapshotData");
> + }
>
> globalxmin = xmin = GetCurrentTransactionId();
>
> diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
> --- ../base/src/backend/utils/time/tqual.c 2003-04-04 15:45:45.000000000 +0200
> +++ src/backend/utils/time/tqual.c 2003-05-08 21:45:55.000000000 +0200
> @@ -30,6 +30,8 @@
> static SnapshotData SnapshotDirtyData;
> Snapshot SnapshotDirty = &SnapshotDirtyData;
>
> +static SnapshotData QuerySnapshotData;
> +static SnapshotData SerializableSnapshotData;
> Snapshot QuerySnapshot = NULL;
> Snapshot SerializableSnapshot = NULL;
>
> @@ -941,23 +943,16 @@
> /* 1st call in xaction? */
> if (SerializableSnapshot == NULL)
> {
> - SerializableSnapshot = GetSnapshotData(true);
> + SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true);
> QuerySnapshot = SerializableSnapshot;
> Assert(QuerySnapshot != NULL);
> return;
> }
>
> - if (QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - QuerySnapshot = NULL;
> - }
> -
> if (XactIsoLevel == XACT_SERIALIZABLE)
> QuerySnapshot = SerializableSnapshot;
> else
> - QuerySnapshot = GetSnapshotData(false);
> + QuerySnapshot = GetSnapshotData(&QuerySnapshotData, false);
>
> Assert(QuerySnapshot != NULL);
> }
> @@ -1003,19 +998,11 @@
> void
> FreeXactSnapshot(void)
> {
> - if (QuerySnapshot != NULL && QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - }
> -
> + /*
> + * We do not free(QuerySnapshot->xip);
> + * or free(SerializableSnapshot->xip);
> + * they will be reused soon
> + */
> QuerySnapshot = NULL;
> -
> - if (SerializableSnapshot != NULL)
> - {
> - free(SerializableSnapshot->xip);
> - free(SerializableSnapshot);
> - }
> -
> SerializableSnapshot = NULL;
> }
> diff -ruN ../base/src/include/utils/tqual.h src/include/utils/tqual.h
> --- ../base/src/include/utils/tqual.h 2003-04-04 15:45:50.000000000 +0200
> +++ src/include/utils/tqual.h 2003-05-08 21:40:04.000000000 +0200
> @@ -113,7 +113,7 @@
> extern HTSV_Result HeapTupleSatisfiesVacuum(HeapTupleHeader tuple,
> TransactionId OldestXmin);
>
> -extern Snapshot GetSnapshotData(bool serializable);
> +extern Snapshot GetSnapshotData(Snapshot snapshot, bool serializable);
> extern void SetQuerySnapshot(void);
> extern Snapshot CopyQuerySnapshot(void);
> extern void FreeXactSnapshot(void);
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073