"serializable" in comments and names
There are many comments in the code which use "serializable
transaction" to mean "transaction snapshot based transaction". This
doesn't matter much as long as REPEATABLE READ and SERIALIZABLE
transaction isolation levels behave identically, but will be
confusing and inaccurate when there is any difference between the
two. In a similar way, the static bool registered_serializable in
snapmgr.c will become misleading, and the IsXactIsoLevelSerializable
macro is bound to lead to confusion.
The patch to implement true serializable transactions using SSI
renames/rewords these things to avoid confusion. However, there are
seven files which are changed only for this reason, and another
where there is one "real" change of two lines hidden in the midst of
dozens of lines of such wording changes. I find it distracting to
have all this mixed in, and I fear that it will be a time-waster for
anyone reviewing the meat of the patch. I'd like to submit a
"no-op" patch to cover these issues in advance of the CF, to get
them out of the way. Joe suggested that I pose the issue to the
-hackers list.
I could knock out a couple other files from the main patch if people
considered it acceptable to enable the SHMQueueIsDetached function
now, which the patch uses in several places within asserts. I would
remove the #ifdef NOT_USED from around the (very short) function,
and add it to the .h file.
The changes to the comments and local variables seem pretty safe.
The change of IsXactIsoLevelSerializable to
IsXactIsoLevelXactSnapshotBased (or whatever name the community
prefers) could break the compile of external code; but the fix would
be pretty obvious. It's hard to see how the change of a local
variable name would present a lot of risk. So I see this as an
extremely low-risk no-op patch to lay the groundwork for the main
patch, so that it is easier to read.
Thoughts?
-Kevin
On Sep 1, 2010, at 11:02 AM, "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
The patch to implement true serializable transactions using SSI
renames/rewords these things to avoid confusion. However, there are
seven files which are changed only for this reason, and another
where there is one "real" change of two lines hidden in the midst of
dozens of lines of such wording changes. I find it distracting to
have all this mixed in, and I fear that it will be a time-waster for
anyone reviewing the meat of the patch. I'd like to submit a
"no-op" patch to cover these issues in advance of the CF, to get
them out of the way. Joe suggested that I pose the issue to the
-hackers list.
+1.
I could knock out a couple other files from the main patch if people
considered it acceptable to enable the SHMQueueIsDetached function
now, which the patch uses in several places within asserts. I would
remove the #ifdef NOT_USED from around the (very short) function,
and add it to the .h file.
-1.
The changes to the comments and local variables seem pretty safe.
The change of IsXactIsoLevelSerializable to
IsXactIsoLevelXactSnapshotBased (or whatever name the community
prefers)
How about IsXactIsoLevelSnapshot? Just to be a bit shorter.
...Robert
Robert Haas <robertmhaas@gmail.com> wrote:
I could knock out a couple other files from the main patch if
people considered it acceptable to enable the SHMQueueIsDetached
function now, which the patch uses in several places within
asserts. I would remove the #ifdef NOT_USED from around the
(very short) function, and add it to the .h file.-1.
OK, I'll leave that part out.
The changes to the comments and local variables seem pretty
safe. The change of IsXactIsoLevelSerializable to
IsXactIsoLevelXactSnapshotBased (or whatever name the community
prefers)How about IsXactIsoLevelSnapshot? Just to be a bit shorter.
I need two macros -- one which has the same definition as the
current IsXactIsoLevelSerializable, to be used everywhere the old
macro name currently is used, which conveys that it is an isolation
level which is based on a transaction snapshot rather than statement
snapshots (i.e., REPEATABLE READ or SERIALIZABLE) and a new macro
(which I was planning to call IsXactIsoLevelFullySerializable) which
conveys that it is the SERIALIZABLE isolation level. Do you feel
that IsXactIsoLevelSnapshot works with
IsXactIsoLevelFullySerializable to convey the right semantics? If
not, what would you suggest?
I'm not attached to any particular names; what matters is that when
people see them, they get the right meanings from them. I have some
concern that IsXactIsoLevelSnapshot might suggest that it excludes
the fully serializable transaction isolation level.
-Kevin
On Thu, Sep 2, 2010 at 11:41 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
How about IsXactIsoLevelSnapshot? Just to be a bit shorter.
I need two macros -- one which has the same definition as the
current IsXactIsoLevelSerializable, to be used everywhere the old
macro name currently is used, which conveys that it is an isolation
level which is based on a transaction snapshot rather than statement
snapshots (i.e., REPEATABLE READ or SERIALIZABLE) and a new macro
(which I was planning to call IsXactIsoLevelFullySerializable) which
conveys that it is the SERIALIZABLE isolation level. Do you feel
that IsXactIsoLevelSnapshot works with
IsXactIsoLevelFullySerializable to convey the right semantics? If
not, what would you suggest?
OK, I see what you were going for. The current definition is:
#define IsXactIsoLevelSerializable (XactIsoLevel >= XACT_REPEATABLE_READ)
...which is certainly a bit odd, since you'd think it would be
comparing against XACT_SERIALIZABLE given the name.
IsXactIsoLevelRepeatableRead()?
XactUsesPerXactSnapshot()?
Or, inverting the sense of it, XactUsesPerStatementSnapshot()?
Just brainstorming...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> wrote:
The current definition is:
#define IsXactIsoLevelSerializable (XactIsoLevel >=
XACT_REPEATABLE_READ)...which is certainly a bit odd, since you'd think it would be
comparing against XACT_SERIALIZABLE given the name.
Precisely why I want to rename it. ;-)
IsXactIsoLevelRepeatableRead()?
Since the SSI implementation of a fully serializable transaction
isolation level needs to do everything that the snapshot isolation
of REPEATABLE READ does, plus a wee bit more, it is convenient to
have a macro with the same semantics; just a less confusing name.
I don't see anywhere in the code where there's a need to test for
*just* REPEATABLE READ -- anything done for that also needs to be
done for SERIALIZABLE.
XactUsesPerXactSnapshot()?
That seems unambiguous. I think I prefer it to
IsXactIsoLevelXactSnapshotBased, so if there are no objections, I'll
switch to XactUsesPerXactSnapshot. The current code uses a macro
without parentheses; are you suggesting that the new code add those?
Or, inverting the sense of it, XactUsesPerStatementSnapshot()?
I don't see anywhere that the code is throwing an exclamation point
in front of the macro name, so inverting it seems like a bad idea.
I'd rather go from:
if (IsXactIsoLevelSerializable)
to:
if (XactUsesPerXactSnapshot)
than:
if (!XactUsesPerStatementSnapshot)
Given the suggested name above, IsXactIsoLevelFullySerializable no
longer seems, well, symmetrical. How do you feel about
XactIsFullySerializable?
Names starting with IsXactIsoLevel seem more technically correct,
but the names get long enough that it seems to me that the meaning
gets a bit lost in the jumble of words -- which is why I like the
shorter suggested name. Any other opinions out there?
-Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
Robert Haas <robertmhaas@gmail.com> wrote:
XactUsesPerXactSnapshot()?
That seems unambiguous. I think I prefer it to
IsXactIsoLevelXactSnapshotBased, so if there are no objections, I'll
switch to XactUsesPerXactSnapshot. The current code uses a macro
without parentheses; are you suggesting that the new code add those?
+1 for adding parens; we might want to make a function of it someday.
Names starting with IsXactIsoLevel seem more technically correct,
but the names get long enough that it seems to me that the meaning
gets a bit lost in the jumble of words -- which is why I like the
shorter suggested name. Any other opinions out there?
I don't much like the "XactUses..." aspect of it; that's just about
meaningless, because almost everything in PG could be said to be "used"
by a transaction. How about IsolationUsesXactSnapshot (versus
IsolationUsesStmtSnapshot)?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1 for adding parens; we might want to make a function of it
someday.
Makes sense; will do.
I don't much like the "XactUses..." aspect of it; that's just
about meaningless, because almost everything in PG could be said
to be "used" by a transaction. How about
IsolationUsesXactSnapshot (versus IsolationUsesStmtSnapshot)?
And IsolationIsSerializable to make that test symmetrical?
Any objections to this plan?
-Kevin
Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1 for adding parens; we might want to make a function of it
someday.
How about IsolationUsesXactSnapshot
Patch attached.
Joe said that he'll review this weekend and probably commit in a day
or two if there are no objections.
-Kevin
Attachments:
sernoop-v1.patchtext/plain; name=sernoop-v1.patchDownload
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
***************
*** 2049,2055 **** IndexCheckExclusion(Relation heapRelation,
*
* After completing validate_index(), we wait until all transactions that
* were alive at the time of the reference snapshot are gone; this is
! * necessary to be sure there are none left with a serializable snapshot
* older than the reference (and hence possibly able to see tuples we did
* not index). Then we mark the index "indisvalid" and commit. Subsequent
* transactions will be able to use it for queries.
--- 2049,2055 ----
*
* After completing validate_index(), we wait until all transactions that
* were alive at the time of the reference snapshot are gone; this is
! * necessary to be sure there are none left with a transaction-based snapshot
* older than the reference (and hence possibly able to see tuples we did
* not index). Then we mark the index "indisvalid" and commit. Subsequent
* transactions will be able to use it for queries.
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 2387,2393 **** ltrmark:;
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 2387,2393 ----
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 1554,1560 **** EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 1554,1560 ----
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
*** a/src/backend/executor/nodeLockRows.c
--- b/src/backend/executor/nodeLockRows.c
***************
*** 130,136 **** lnext:
break;
case HeapTupleUpdated:
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 130,136 ----
break;
case HeapTupleUpdated:
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 328,334 **** ldelete:;
break;
case HeapTupleUpdated:
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 328,334 ----
break;
case HeapTupleUpdated:
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
***************
*** 516,522 **** lreplace:;
break;
case HeapTupleUpdated:
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 516,522 ----
break;
case HeapTupleUpdated:
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
***************
*** 3332,3346 **** ri_PerformCheck(RI_QueryKey *qkey, SPIPlanPtr qplan,
/*
* In READ COMMITTED mode, we just need to use an up-to-date regular
* snapshot, and we will see all rows that could be interesting. But in
! * SERIALIZABLE mode, we can't change the transaction snapshot. If the
! * caller passes detectNewRows == false then it's okay to do the query
* with the transaction snapshot; otherwise we use a current snapshot, and
* tell the executor to error out if it finds any rows under the current
* snapshot that wouldn't be visible per the transaction snapshot. Note
* that SPI_execute_snapshot will register the snapshots, so we don't need
* to bother here.
*/
! if (IsXactIsoLevelSerializable && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
test_snapshot = GetLatestSnapshot();
--- 3332,3346 ----
/*
* In READ COMMITTED mode, we just need to use an up-to-date regular
* snapshot, and we will see all rows that could be interesting. But in
! * xact-snapshot-based modes, we can't change the transaction snapshot. If
! * the caller passes detectNewRows == false then it's okay to do the query
* with the transaction snapshot; otherwise we use a current snapshot, and
* tell the executor to error out if it finds any rows under the current
* snapshot that wouldn't be visible per the transaction snapshot. Note
* that SPI_execute_snapshot will register the snapshots, so we don't need
* to bother here.
*/
! if (IsolationUsesXactSnapshot() && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
test_snapshot = GetLatestSnapshot();
*** a/src/backend/utils/time/snapmgr.c
--- b/src/backend/utils/time/snapmgr.c
***************
*** 37,44 ****
/*
! * CurrentSnapshot points to the only snapshot taken in a serializable
! * transaction, and to the latest one taken in a read-committed transaction.
* SecondarySnapshot is a snapshot that's always up-to-date as of the current
* instant, even on a serializable transaction. It should only be used for
* special-purpose code (say, RI checking.)
--- 37,44 ----
/*
! * CurrentSnapshot points to the only snapshot taken in a xact-snapshot-based
! * transaction; otherwise to the latest one taken.
* SecondarySnapshot is a snapshot that's always up-to-date as of the current
* instant, even on a serializable transaction. It should only be used for
* special-purpose code (say, RI checking.)
***************
*** 97,107 **** static int RegisteredSnapshots = 0;
bool FirstSnapshotSet = false;
/*
! * Remembers whether this transaction registered a serializable snapshot at
* start. We cannot trust FirstSnapshotSet in combination with
! * IsXactIsoLevelSerializable, because GUC may be reset before us.
*/
! static bool registered_serializable = false;
static Snapshot CopySnapshot(Snapshot snapshot);
--- 97,107 ----
bool FirstSnapshotSet = false;
/*
! * Remembers whether this transaction registered a transaction-based snapshot at
* start. We cannot trust FirstSnapshotSet in combination with
! * IsolationUsesXactSnapshot(), because GUC may be reset before us.
*/
! static bool registered_xact_snapshot = false;
static Snapshot CopySnapshot(Snapshot snapshot);
***************
*** 130,150 **** GetTransactionSnapshot(void)
FirstSnapshotSet = true;
/*
! * In serializable mode, the first snapshot must live until end of
! * xact regardless of what the caller does with it, so we must
! * register it internally here and unregister it at end of xact.
*/
! if (IsXactIsoLevelSerializable)
{
CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_serializable = true;
}
return CurrentSnapshot;
}
! if (IsXactIsoLevelSerializable)
return CurrentSnapshot;
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
--- 130,151 ----
FirstSnapshotSet = true;
/*
! * In xact-snapshot-based isolation levels, the first snapshot must
! * live until end of xact regardless of what the caller does with it,
! * so we must register it internally here and unregister it at end of
! * xact.
*/
! if (IsolationUsesXactSnapshot())
{
CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_xact_snapshot = true;
}
return CurrentSnapshot;
}
! if (IsolationUsesXactSnapshot())
return CurrentSnapshot;
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
***************
*** 155,161 **** GetTransactionSnapshot(void)
/*
* GetLatestSnapshot
* Get a snapshot that is up-to-date as of the current instant,
! * even if we are executing in SERIALIZABLE mode.
*/
Snapshot
GetLatestSnapshot(void)
--- 156,162 ----
/*
* GetLatestSnapshot
* Get a snapshot that is up-to-date as of the current instant,
! * even if we are executing in xact-snapshot-based mode.
*/
Snapshot
GetLatestSnapshot(void)
***************
*** 515,527 **** void
AtEarlyCommit_Snapshot(void)
{
/*
! * On a serializable transaction we must unregister our private refcount
! * to the serializable snapshot.
*/
! if (registered_serializable)
UnregisterSnapshotFromOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_serializable = false;
}
--- 516,528 ----
AtEarlyCommit_Snapshot(void)
{
/*
! * On a xact-snapshot-based transaction we must unregister our private
! * refcount to the xact snapshot.
*/
! if (registered_xact_snapshot)
UnregisterSnapshotFromOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_xact_snapshot = false;
}
***************
*** 557,561 **** AtEOXact_Snapshot(bool isCommit)
SecondarySnapshot = NULL;
FirstSnapshotSet = false;
! registered_serializable = false;
}
--- 558,562 ----
SecondarySnapshot = NULL;
FirstSnapshotSet = false;
! registered_xact_snapshot = false;
}
*** a/src/include/access/xact.h
--- b/src/include/access/xact.h
***************
*** 35,41 **** extern int XactIsoLevel;
* We only implement two isolation levels internally. This macro should
* be used to check which one is selected.
*/
! #define IsXactIsoLevelSerializable (XactIsoLevel >= XACT_REPEATABLE_READ)
/* Xact read-only state */
extern bool DefaultXactReadOnly;
--- 35,41 ----
* We only implement two isolation levels internally. This macro should
* be used to check which one is selected.
*/
! #define IsolationUsesXactSnapshot() (XactIsoLevel >= XACT_REPEATABLE_READ)
/* Xact read-only state */
extern bool DefaultXactReadOnly;
Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1 for adding parens; we might want to make a function of it
someday.How about IsolationUsesXactSnapshot
Patch attached.
I find this name confusing :-( Doesn't a READ COMMITTED transaction use
transaction snapshots as well?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010:
How about IsolationUsesXactSnapshot
I find this name confusing :-( Doesn't a READ COMMITTED transaction use
transaction snapshots as well?
AFAIR it doesn't keep the first snapshot around. If it did, most of
your work on snapshot list trimming would have been useless, no?
regards, tom lane
Excerpts from Tom Lane's message of mié sep 08 12:12:31 -0400 2010:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010:
How about IsolationUsesXactSnapshot
I find this name confusing :-( Doesn't a READ COMMITTED transaction use
transaction snapshots as well?AFAIR it doesn't keep the first snapshot around. If it did, most of
your work on snapshot list trimming would have been useless, no?
That's my point precisely. The name "IsolationUsesXactSnapshot" makes
it sound like it applies to any transaction that uses snapshots for
isolation, doesn't it? How about IsolationUses1stXactSnapshot, or
something else that makes it clearer that there's a difference between
that and read committed transactions?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Kevin Grittner's message of vie sep 03 19:06:17 -0400 2010:
How about IsolationUsesXactSnapshot
I find this name confusing :-( Doesn't a READ COMMITTED transaction use
transaction snapshots as well?AFAIR it doesn't keep the first snapshot around. If it did, most of
your work on snapshot list trimming would have been useless, no?
Technically, serializable uses a single transaction snapshot and read
committed uses statement snapshots.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of mié sep 08 12:12:31 -0400 2010:
AFAIR it doesn't keep the first snapshot around. If it did, most of
your work on snapshot list trimming would have been useless, no?
That's my point precisely. The name "IsolationUsesXactSnapshot" makes
it sound like it applies to any transaction that uses snapshots for
isolation, doesn't it?
I don't think so, at least not when compared to the alternative
IsolationUsesStmtSnapshot.
How about IsolationUses1stXactSnapshot
This just seems longer, not really better. In particular, we have
*always* adhered to the phraseology that a "transaction snapshot"
is the first one taken in a transaction, so I don't see exactly
why it's confusing you now.
regards, tom lane
On 09/08/2010 10:02 AM, Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of mié sep 08 12:12:31 -0400 2010:
AFAIR it doesn't keep the first snapshot around. If it did, most of
your work on snapshot list trimming would have been useless, no?That's my point precisely. The name "IsolationUsesXactSnapshot" makes
it sound like it applies to any transaction that uses snapshots for
isolation, doesn't it?I don't think so, at least not when compared to the alternative
IsolationUsesStmtSnapshot.
The attached patch is updated for the various comments, as well as some
wording tweaks by me. If there are no objections I'd like to commit this
in a day or two.
Joe
--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
Attachments:
sernoop-v1.4.patchtext/x-patch; name=sernoop-v1.4.patchDownload
? src/backend/parser/parse.h
Index: src/backend/access/heap/heapam.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.293
diff -c -r1.293 heapam.c
*** src/backend/access/heap/heapam.c 29 Jul 2010 16:14:36 -0000 1.293
--- src/backend/access/heap/heapam.c 9 Sep 2010 16:03:18 -0000
***************
*** 2173,2179 ****
if (crosscheck != InvalidSnapshot && result == HeapTupleMayBeUpdated)
{
! /* Perform additional check for serializable RI updates */
if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
result = HeapTupleUpdated;
}
--- 2173,2179 ----
if (crosscheck != InvalidSnapshot && result == HeapTupleMayBeUpdated)
{
! /* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&tp, crosscheck, buffer))
result = HeapTupleUpdated;
}
***************
*** 2525,2531 ****
if (crosscheck != InvalidSnapshot && result == HeapTupleMayBeUpdated)
{
! /* Perform additional check for serializable RI updates */
if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer))
result = HeapTupleUpdated;
}
--- 2525,2531 ----
if (crosscheck != InvalidSnapshot && result == HeapTupleMayBeUpdated)
{
! /* Perform additional check for transaction-snapshot mode RI updates */
if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer))
result = HeapTupleUpdated;
}
Index: src/backend/catalog/index.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.338
diff -c -r1.338 index.c
*** src/backend/catalog/index.c 13 Aug 2010 20:10:50 -0000 1.338
--- src/backend/catalog/index.c 9 Sep 2010 16:03:18 -0000
***************
*** 2049,2055 ****
*
* After completing validate_index(), we wait until all transactions that
* were alive at the time of the reference snapshot are gone; this is
! * necessary to be sure there are none left with a serializable snapshot
* older than the reference (and hence possibly able to see tuples we did
* not index). Then we mark the index "indisvalid" and commit. Subsequent
* transactions will be able to use it for queries.
--- 2049,2055 ----
*
* After completing validate_index(), we wait until all transactions that
* were alive at the time of the reference snapshot are gone; this is
! * necessary to be sure there are none left with a transaction snapshot
* older than the reference (and hence possibly able to see tuples we did
* not index). Then we mark the index "indisvalid" and commit. Subsequent
* transactions will be able to use it for queries.
Index: src/backend/commands/trigger.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/trigger.c,v
retrieving revision 1.265
diff -c -r1.265 trigger.c
*** src/backend/commands/trigger.c 19 Aug 2010 15:46:18 -0000 1.265
--- src/backend/commands/trigger.c 9 Sep 2010 16:03:18 -0000
***************
*** 2387,2393 ****
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 2387,2393 ----
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
Index: src/backend/executor/execMain.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/execMain.c,v
retrieving revision 1.354
diff -c -r1.354 execMain.c
*** src/backend/executor/execMain.c 5 Aug 2010 14:45:02 -0000 1.354
--- src/backend/executor/execMain.c 9 Sep 2010 16:03:19 -0000
***************
*** 1554,1560 ****
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 1554,1560 ----
case HeapTupleUpdated:
ReleaseBuffer(buffer);
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
Index: src/backend/executor/nodeLockRows.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/nodeLockRows.c,v
retrieving revision 1.6
diff -c -r1.6 nodeLockRows.c
*** src/backend/executor/nodeLockRows.c 28 Jul 2010 17:21:56 -0000 1.6
--- src/backend/executor/nodeLockRows.c 9 Sep 2010 16:03:19 -0000
***************
*** 130,136 ****
break;
case HeapTupleUpdated:
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 130,136 ----
break;
case HeapTupleUpdated:
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
Index: src/backend/executor/nodeModifyTable.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/nodeModifyTable.c,v
retrieving revision 1.9
diff -c -r1.9 nodeModifyTable.c
*** src/backend/executor/nodeModifyTable.c 18 Aug 2010 21:52:24 -0000 1.9
--- src/backend/executor/nodeModifyTable.c 9 Sep 2010 16:03:19 -0000
***************
*** 310,316 ****
* Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
* the row to be deleted is visible to that snapshot, and throw a can't-
* serialize error if not. This is a special-case behavior needed for
! * referential integrity updates in serializable transactions.
*/
ldelete:;
result = heap_delete(resultRelationDesc, tupleid,
--- 310,316 ----
* Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
* the row to be deleted is visible to that snapshot, and throw a can't-
* serialize error if not. This is a special-case behavior needed for
! * referential integrity updates in transaction-snapshot mode transactions.
*/
ldelete:;
result = heap_delete(resultRelationDesc, tupleid,
***************
*** 328,334 ****
break;
case HeapTupleUpdated:
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 328,334 ----
break;
case HeapTupleUpdated:
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
***************
*** 499,505 ****
* Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
* the row to be updated is visible to that snapshot, and throw a can't-
* serialize error if not. This is a special-case behavior needed for
! * referential integrity updates in serializable transactions.
*/
result = heap_update(resultRelationDesc, tupleid, tuple,
&update_ctid, &update_xmax,
--- 499,505 ----
* Note: if es_crosscheck_snapshot isn't InvalidSnapshot, we check that
* the row to be updated is visible to that snapshot, and throw a can't-
* serialize error if not. This is a special-case behavior needed for
! * referential integrity updates in transaction-snapshot mode transactions.
*/
result = heap_update(resultRelationDesc, tupleid, tuple,
&update_ctid, &update_xmax,
***************
*** 516,522 ****
break;
case HeapTupleUpdated:
! if (IsXactIsoLevelSerializable)
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
--- 516,522 ----
break;
case HeapTupleUpdated:
! if (IsolationUsesXactSnapshot())
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.596
diff -c -r1.596 postgres.c
*** src/backend/tcop/postgres.c 12 Aug 2010 23:24:54 -0000 1.596
--- src/backend/tcop/postgres.c 9 Sep 2010 16:03:19 -0000
***************
*** 2802,2808 ****
*
* PROCSIG_RECOVERY_CONFLICT_SNAPSHOT if no snapshots are held
* by parent transactions and the transaction is not
! * serializable
*
* PROCSIG_RECOVERY_CONFLICT_TABLESPACE if no temp files or
* cursors open in parent transactions
--- 2802,2808 ----
*
* PROCSIG_RECOVERY_CONFLICT_SNAPSHOT if no snapshots are held
* by parent transactions and the transaction is not
! * transaction-snapshot mode
*
* PROCSIG_RECOVERY_CONFLICT_TABLESPACE if no temp files or
* cursors open in parent transactions
Index: src/backend/tcop/pquery.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/tcop/pquery.c,v
retrieving revision 1.137
diff -c -r1.137 pquery.c
*** src/backend/tcop/pquery.c 26 Feb 2010 02:01:02 -0000 1.137
--- src/backend/tcop/pquery.c 9 Sep 2010 16:03:19 -0000
***************
*** 1163,1170 ****
* Set snapshot if utility stmt needs one. Most reliable way to do this
* seems to be to enumerate those that do not need one; this is a short
* list. Transaction control, LOCK, and SET must *not* set a snapshot
! * since they need to be executable at the start of a serializable
! * transaction without freezing a snapshot. By extension we allow SHOW
* not to set a snapshot. The other stmts listed are just efficiency
* hacks. Beware of listing anything that can modify the database --- if,
* say, it has to update an index with expressions that invoke
--- 1163,1170 ----
* Set snapshot if utility stmt needs one. Most reliable way to do this
* seems to be to enumerate those that do not need one; this is a short
* list. Transaction control, LOCK, and SET must *not* set a snapshot
! * since they need to be executable at the start of a transaction-snapshot
! * mode transaction without freezing a snapshot. By extension we allow SHOW
* not to set a snapshot. The other stmts listed are just efficiency
* hacks. Beware of listing anything that can modify the database --- if,
* say, it has to update an index with expressions that invoke
Index: src/backend/utils/adt/ri_triggers.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ri_triggers.c,v
retrieving revision 1.120
diff -c -r1.120 ri_triggers.c
*** src/backend/utils/adt/ri_triggers.c 28 Jul 2010 05:22:24 -0000 1.120
--- src/backend/utils/adt/ri_triggers.c 9 Sep 2010 16:03:19 -0000
***************
*** 2784,2793 ****
/*
* Run the plan. For safety we force a current snapshot to be used. (In
! * serializable mode, this arguably violates serializability, but we
! * really haven't got much choice.) We don't need to register the
! * snapshot, because SPI_execute_snapshot will see to it. We need at most
! * one tuple returned, so pass limit = 1.
*/
spi_result = SPI_execute_snapshot(qplan,
NULL, NULL,
--- 2784,2793 ----
/*
* Run the plan. For safety we force a current snapshot to be used. (In
! * transaction-snapshot mode, this arguably violates transaction
! * isolation rules, but we really haven't got much choice.)
! * We don't need to register the snapshot, because SPI_execute_snapshot
! * will see to it. We need at most one tuple returned, so pass limit = 1.
*/
spi_result = SPI_execute_snapshot(qplan,
NULL, NULL,
***************
*** 3332,3346 ****
/*
* In READ COMMITTED mode, we just need to use an up-to-date regular
* snapshot, and we will see all rows that could be interesting. But in
! * SERIALIZABLE mode, we can't change the transaction snapshot. If the
! * caller passes detectNewRows == false then it's okay to do the query
* with the transaction snapshot; otherwise we use a current snapshot, and
* tell the executor to error out if it finds any rows under the current
* snapshot that wouldn't be visible per the transaction snapshot. Note
* that SPI_execute_snapshot will register the snapshots, so we don't need
* to bother here.
*/
! if (IsXactIsoLevelSerializable && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
test_snapshot = GetLatestSnapshot();
--- 3332,3346 ----
/*
* In READ COMMITTED mode, we just need to use an up-to-date regular
* snapshot, and we will see all rows that could be interesting. But in
! * transaction-snapshot mode, we can't change the transaction snapshot.
! * If the caller passes detectNewRows == false then it's okay to do the query
* with the transaction snapshot; otherwise we use a current snapshot, and
* tell the executor to error out if it finds any rows under the current
* snapshot that wouldn't be visible per the transaction snapshot. Note
* that SPI_execute_snapshot will register the snapshots, so we don't need
* to bother here.
*/
! if (IsolationUsesXactSnapshot() && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
test_snapshot = GetLatestSnapshot();
Index: src/backend/utils/time/snapmgr.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/time/snapmgr.c,v
retrieving revision 1.15
diff -c -r1.15 snapmgr.c
*** src/backend/utils/time/snapmgr.c 26 Feb 2010 02:01:15 -0000 1.15
--- src/backend/utils/time/snapmgr.c 9 Sep 2010 16:03:19 -0000
***************
*** 37,46 ****
/*
! * CurrentSnapshot points to the only snapshot taken in a serializable
! * transaction, and to the latest one taken in a read-committed transaction.
* SecondarySnapshot is a snapshot that's always up-to-date as of the current
! * instant, even on a serializable transaction. It should only be used for
* special-purpose code (say, RI checking.)
*
* These SnapshotData structs are static to simplify memory allocation
--- 37,46 ----
/*
! * CurrentSnapshot points to the only snapshot taken in transaction-snapshot
! * mode, and to the latest one taken in a read-committed transaction.
* SecondarySnapshot is a snapshot that's always up-to-date as of the current
! * instant, even in transaction-snapshot mode. It should only be used for
* special-purpose code (say, RI checking.)
*
* These SnapshotData structs are static to simplify memory allocation
***************
*** 97,107 ****
bool FirstSnapshotSet = false;
/*
! * Remembers whether this transaction registered a serializable snapshot at
* start. We cannot trust FirstSnapshotSet in combination with
! * IsXactIsoLevelSerializable, because GUC may be reset before us.
*/
! static bool registered_serializable = false;
static Snapshot CopySnapshot(Snapshot snapshot);
--- 97,107 ----
bool FirstSnapshotSet = false;
/*
! * Remembers whether this transaction registered a transaction snapshot at
* start. We cannot trust FirstSnapshotSet in combination with
! * IsolationUsesXactSnapshot(), because GUC may be reset before us.
*/
! static bool registered_xact_snapshot = false;
static Snapshot CopySnapshot(Snapshot snapshot);
***************
*** 130,150 ****
FirstSnapshotSet = true;
/*
! * In serializable mode, the first snapshot must live until end of
! * xact regardless of what the caller does with it, so we must
* register it internally here and unregister it at end of xact.
*/
! if (IsXactIsoLevelSerializable)
{
CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_serializable = true;
}
return CurrentSnapshot;
}
! if (IsXactIsoLevelSerializable)
return CurrentSnapshot;
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
--- 130,150 ----
FirstSnapshotSet = true;
/*
! * In transaction-snapshot mode, the first snapshot must live until
! * end of xact regardless of what the caller does with it, so we must
* register it internally here and unregister it at end of xact.
*/
! if (IsolationUsesXactSnapshot())
{
CurrentSnapshot = RegisterSnapshotOnOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_xact_snapshot = true;
}
return CurrentSnapshot;
}
! if (IsolationUsesXactSnapshot())
return CurrentSnapshot;
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
***************
*** 155,161 ****
/*
* GetLatestSnapshot
* Get a snapshot that is up-to-date as of the current instant,
! * even if we are executing in SERIALIZABLE mode.
*/
Snapshot
GetLatestSnapshot(void)
--- 155,161 ----
/*
* GetLatestSnapshot
* Get a snapshot that is up-to-date as of the current instant,
! * even if we are executing in transaction-snapshot mode.
*/
Snapshot
GetLatestSnapshot(void)
***************
*** 515,527 ****
AtEarlyCommit_Snapshot(void)
{
/*
! * On a serializable transaction we must unregister our private refcount
! * to the serializable snapshot.
*/
! if (registered_serializable)
UnregisterSnapshotFromOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_serializable = false;
}
--- 515,527 ----
AtEarlyCommit_Snapshot(void)
{
/*
! * In transaction-snapshot mode we must unregister our private refcount
! * to the transaction-snapshot.
*/
! if (registered_xact_snapshot)
UnregisterSnapshotFromOwner(CurrentSnapshot,
TopTransactionResourceOwner);
! registered_xact_snapshot = false;
}
***************
*** 557,561 ****
SecondarySnapshot = NULL;
FirstSnapshotSet = false;
! registered_serializable = false;
}
--- 557,561 ----
SecondarySnapshot = NULL;
FirstSnapshotSet = false;
! registered_xact_snapshot = false;
}
Index: src/include/access/xact.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/access/xact.h,v
retrieving revision 1.103
diff -c -r1.103 xact.h
*** src/include/access/xact.h 26 Feb 2010 02:01:21 -0000 1.103
--- src/include/access/xact.h 9 Sep 2010 16:03:20 -0000
***************
*** 35,41 ****
* We only implement two isolation levels internally. This macro should
* be used to check which one is selected.
*/
! #define IsXactIsoLevelSerializable (XactIsoLevel >= XACT_REPEATABLE_READ)
/* Xact read-only state */
extern bool DefaultXactReadOnly;
--- 35,41 ----
* We only implement two isolation levels internally. This macro should
* be used to check which one is selected.
*/
! #define IsolationUsesXactSnapshot() (XactIsoLevel >= XACT_REPEATABLE_READ)
/* Xact read-only state */
extern bool DefaultXactReadOnly;
On 09/09/2010 09:11 AM, Joe Conway wrote:
The attached patch is updated for the various comments, as well as some
wording tweaks by me. If there are no objections I'd like to commit this
in a day or two.
Committed.
Joe
--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support
Joe Conway <mail@joeconway.com> wrote:
Committed.
Thanks!
I'm pulling together a new version of the main patch, and it is
almost 300 lines shorter and touches five fewer files than the last
version because this went in. It should be easier for people to
scan to understand the substance of the changes without the noop
changes interleaved with "real" ones.
-Kevin