Обсуждение: SSI implementation question

Is it really necessary for GetSerializableTransactionSnapshotInt to
acquire an empty SERIALIZABLEXACT before it acquires a snapshot?
If so, why?  The proposed synchronized-snapshots feature will mean
that the allegedly-new snapshot actually was taken some time before,
so it seems to me that either this is not necessary or we cannot use
a synchronized snapshot in a serializable xact.

In the same vein, why is it necessary to be holding
SerializableXactHashLock (exclusively, yet) while acquiring the
snapshot?  That seems rather bad from a concurrency standpoint, and
again it's going to be pretty meaningless if we're just installing a
pre-existing snapshot.

The reason these things came to mind is that I want to refactor the code
so that the SSI-specific work in GetSerializableTransactionSnapshotInt
is done by a function that is handed an already-taken snapshot, because
I cannot stomach what Joachim did to the APIs of GetSnapshotData and
allied functions.  But refactor or no refactor, it seems like installing
a pre-existing snapshot may be breaking some assumptions here.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Is it really necessary for GetSerializableTransactionSnapshotInt
> to acquire an empty SERIALIZABLEXACT before it acquires a
> snapshot?  If so, why?  The proposed synchronized-snapshots
> feature will mean that the allegedly-new snapshot actually was
> taken some time before, so it seems to me that either this is not
> necessary or we cannot use a synchronized snapshot in a
> serializable xact.
Hmm.  If the intent is that each serializable transaction sharing
the snapshot is a separate logical transaction, it *might* hold -- I
have to think about that a bit.  If the intent is that the work of
one logical transaction is being split across processes, then SSI
doesn't hold up without somehow tying all of the processes to a
single SERIALIZABLEXACT; and then the direct access to
MySerializableXact falls apart.
> In the same vein, why is it necessary to be holding
> SerializableXactHashLock (exclusively, yet) while acquiring the
> snapshot?  That seems rather bad from a concurrency standpoint,
> and again it's going to be pretty meaningless if we're just
> installing a pre-existing snapshot.
That seems like something which probably could and should be fixed,
especially since SerializableXactHashLock can become a bottleneck in
9.1 and will be much more of a problem with the elimination of other
bottlenecks in 9.2.
> The reason these things came to mind is that I want to refactor
> the code so that the SSI-specific work in
> GetSerializableTransactionSnapshotInt is done by a function that
> is handed an already-taken snapshot, because I cannot stomach what
> Joachim did to the APIs of GetSnapshotData and allied functions. 
> But refactor or no refactor, it seems like installing a
> pre-existing snapshot may be breaking some assumptions here.
I guess the other thing to look out for is whether it could possibly
move PredXact->SxactGlobalXmin backwards.  I would have to check for
other possible problems.
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:
> If the intent is that each serializable transaction sharing
> the snapshot is a separate logical transaction, it *might* hold --
I think the rules have to be that the snapshot provided to a
serializable transaction must be provided by an active serializable
transaction.  That prevents the serializable global xmin from moving
backwards; which is not allowed except during recovery processing of
prepared transactions.  Each transaction using the snapshot is a
logically separate transaction -- they just have a shared view of
the state of the data.
> If the intent is that the work of one logical transaction is being
> split across processes, then SSI doesn't hold up without somehow
> tying all of the processes to a single SERIALIZABLEXACT; and then
> the direct access to MySerializableXact falls apart.
Except, as discussed on a separate, concurrent thread, that a READ
ONLY transaction might find its snapshot to be safe -- at which
point it no longer uses a SERIALIZABLEXACT.
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Is it really necessary for GetSerializableTransactionSnapshotInt
>> to acquire an empty SERIALIZABLEXACT before it acquires a
>> snapshot?  If so, why?  The proposed synchronized-snapshots
>> feature will mean that the allegedly-new snapshot actually was
>> taken some time before, so it seems to me that either this is not
>> necessary or we cannot use a synchronized snapshot in a
>> serializable xact.
> Hmm.  If the intent is that each serializable transaction sharing
> the snapshot is a separate logical transaction, it *might* hold -- I
> have to think about that a bit.  If the intent is that the work of
> one logical transaction is being split across processes, then SSI
> doesn't hold up without somehow tying all of the processes to a
> single SERIALIZABLEXACT; and then the direct access to
> MySerializableXact falls apart.

No, the intention of the synchronized-snapshots feature is just to be
able to start multiple transactions using exactly the same snapshot.
They're independent after that.  The aspect of it that is worrying me
is that if xact A starts, gets a snapshot and publishes it, and then
xact B starts and wants to adopt that snapshot, then

(1) other transactions may have started or ended meanwhile; does that
break any of SSI's assumptions?

(2) as things stand, xact A need not be running in serializable mode ---
if B is serializable, does *that* break any assumptions?

We already have to have an interlock to ensure that GlobalXmin doesn't
go backwards, by means of requiring A to still be running at the instant
B adopts the snapshot and sets its MyProc->xmin accordingly.  However,
there is not currently anything that guarantees that A is still running
by the time B does GetSerializableTransactionSnapshotInt, shortly later.
So if your answer to question (1) involves an assumption that A is still
running, we're going to have to figure out how to arrange that without
deadlocking on ProcArrayLock vs SerializableXactHashLock.  Which might
be another good reason for changing predicate.c so that we don't hold
the latter while taking a snapshot ...
        regards, tom lane

On Wed, Oct 19, 2011 at 12:56:58PM -0400, Tom Lane wrote:
> Is it really necessary for GetSerializableTransactionSnapshotInt to
> acquire an empty SERIALIZABLEXACT before it acquires a snapshot?
> If so, why? 

*That* isn't necessary, no. It is necessary, however, to acquire the
snapshot while SerializableXactHashLock is held. There are a couple
reasons for this: the sxact's lastCommitBeforeSnapshot needs to match
the snapshot, SxactGlobalXmin needs to be set to the correct value,
etc. That's why the call to GetSnapshotData happens from where it does

> The proposed synchronized-snapshots feature will mean
> that the allegedly-new snapshot actually was taken some time before,
> so it seems to me that either this is not necessary or we cannot use
> a synchronized snapshot in a serializable xact.

There are definitely potential problems here. If the original snapshot
doesn't belong to an active serializable transaction, we may have
discarded the state we need to do SSI, e.g. we might have already
cleaned up SIREAD locks from concurrent committed transactions.

I assume the answer here is going to have to be to either refuse to
start a serializable transaction if that's the case, or make saving a
snapshot inhibit some of the SSI cleanup.

> In the same vein, why is it necessary to be holding
> SerializableXactHashLock (exclusively, yet) while acquiring the
> snapshot?  That seems rather bad from a concurrency standpoint, and
> again it's going to be pretty meaningless if we're just installing a
> pre-existing snapshot.

Yes, it's bad. I'm working on a design to address
SerializableXactHashLock contention, but there needs to be some locking
here for the reasons I mentioned above. I think the best we can do here
is to acquire a lock in shared mode when registering a serializable
transaction and in exclusive mode when committing. (Which is what you'd
expect, I guess; it's the same story as ProcArrayLock, and for most of
the same reasons.) Obviously, we'll also want to minimize the amount of
work we're doing while holding that lock.

Dan

-- 
Dan R. K. Ports              MIT CSAIL                http://drkp.net/

Dan Ports <drkp@csail.mit.edu> writes:
> On Wed, Oct 19, 2011 at 12:56:58PM -0400, Tom Lane wrote:
>> The proposed synchronized-snapshots feature will mean
>> that the allegedly-new snapshot actually was taken some time before,
>> so it seems to me that either this is not necessary or we cannot use
>> a synchronized snapshot in a serializable xact.

> There are definitely potential problems here. If the original snapshot
> doesn't belong to an active serializable transaction, we may have
> discarded the state we need to do SSI, e.g. we might have already
> cleaned up SIREAD locks from concurrent committed transactions.

> I assume the answer here is going to have to be to either refuse to
> start a serializable transaction if that's the case, or make saving a
> snapshot inhibit some of the SSI cleanup.

We can easily mark an exported snapshot with the IsoLevel of the
transaction that made it, and then for example refuse to adopt a
less-than-serializable snapshot into a serializable transaction.
So that aspect can be had if we need it.  But we still have a race
condition with the patch as it stands, because there is a window for the
original xact to terminate before GetSerializableTransactionSnapshotInt
runs.  It sounds like we have to fix that.

>> In the same vein, why is it necessary to be holding
>> SerializableXactHashLock (exclusively, yet) while acquiring the
>> snapshot?  That seems rather bad from a concurrency standpoint, and
>> again it's going to be pretty meaningless if we're just installing a
>> pre-existing snapshot.

> Yes, it's bad. I'm working on a design to address
> SerializableXactHashLock contention, but there needs to be some locking
> here for the reasons I mentioned above. I think the best we can do here
> is to acquire a lock in shared mode when registering a serializable
> transaction and in exclusive mode when committing. (Which is what you'd
> expect, I guess; it's the same story as ProcArrayLock, and for most of
> the same reasons.) Obviously, we'll also want to minimize the amount of
> work we're doing while holding that lock.

I wonder whether it would be prudent to set the synchronized-snapshots
patch aside until you've finished that work (assuming you're actively
working on it).  It's evidently going to require some nontrivial changes
in predicate.c, and I don't think the feature should take precedence
over SSI performance improvement.
        regards, tom lane

On Wed, Oct 19, 2011 at 04:36:41PM -0400, Tom Lane wrote:
> No, the intention of the synchronized-snapshots feature is just to be
> able to start multiple transactions using exactly the same snapshot.
> They're independent after that.  The aspect of it that is worrying me
> is that if xact A starts, gets a snapshot and publishes it, and then
> xact B starts and wants to adopt that snapshot, then
> 
> (2) as things stand, xact A need not be running in serializable mode ---
> if B is serializable, does *that* break any assumptions?

[taking these in opposite order]

Yes, I think that's going to be a problem. The obvious case where it's
clearly not going to work is if A is older than the oldest active
serializable xact (i.e. SxactGlobalXmin would have to move backwards).
It's probably possible to make it work when that's not the case, but I
think it's better to require A to be serializable -- if nothing else,
it's a far simpler rule to document!

There is another case that could be problematic, if A was READ ONLY,
and B isn't. It sounds to me like that would also be a reasonable thing
to forbid.

> (1) other transactions may have started or ended meanwhile; does that
> break any of SSI's assumptions?

Mostly, no, if A is still running. There's one case that needs to be
handled a bit carefully, but shouldn't be a problem: if A was
SERIALIZABLE READ ONLY, and its snapshot was found to be safe, then
it's actually running (safely) at REPEATABLE READ. If we start a new
read-only transaction at the same snapshot, we need to make it run at
REPEATABLE READ if it requests SERIALIZABLE.

> We already have to have an interlock to ensure that GlobalXmin doesn't
> go backwards, by means of requiring A to still be running at the instant
> B adopts the snapshot and sets its MyProc->xmin accordingly.  However,
> there is not currently anything that guarantees that A is still running
> by the time B does GetSerializableTransactionSnapshotInt, shortly later.
> So if your answer to question (1) involves an assumption that A is still
> running, we're going to have to figure out how to arrange that without
> deadlocking on ProcArrayLock vs SerializableXactHashLock.

Yep, I think we're going to have to do that. I haven't had a chance to
look at the synchronized snapshots patch yet, so I can't (yet) offer
any suggestions about how to implement it.

> Which might
> be another good reason for changing predicate.c so that we don't hold
> the latter while taking a snapshot ...

It'd be great if we could do that, but I don't really see it being
possible...

Dan

-- 
Dan R. K. Ports              MIT CSAIL                http://drkp.net/

Dan Ports <drkp@csail.mit.edu> wrote:
> the sxact's lastCommitBeforeSnapshot needs to match the snapshot,
> SxactGlobalXmin needs to be set to the correct value, etc. That's
> why the call to GetSnapshotData happens from where it does
Oh, right.  I knew I was forgetting something.  What if that was
captured as part of building a snapshot?  That seems like it would
be a trivial cost compared to other snapshot-building activity, and
might give us a way to get this out from under the
SerializableXactHashLock locking.
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Dan Ports <drkp@csail.mit.edu> wrote:
>> the sxact's lastCommitBeforeSnapshot needs to match the snapshot,
>> SxactGlobalXmin needs to be set to the correct value, etc. That's
>> why the call to GetSnapshotData happens from where it does
> Oh, right.  I knew I was forgetting something.  What if that was
> captured as part of building a snapshot?  That seems like it would
> be a trivial cost compared to other snapshot-building activity, and
> might give us a way to get this out from under the
> SerializableXactHashLock locking.

But aren't the values you need to fetch protected by
SerializableXactHashLock?  Having to take an additional LWLock is
*not* a "trivial cost".
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>> Dan Ports <drkp@csail.mit.edu> wrote:
>>> the sxact's lastCommitBeforeSnapshot needs to match the
>>> snapshot, SxactGlobalXmin needs to be set to the correct value,
>>> etc. That's why the call to GetSnapshotData happens from where
>>> it does
>  
>> Oh, right.  I knew I was forgetting something.  What if that was
>> captured as part of building a snapshot?  That seems like it
>> would be a trivial cost compared to other snapshot-building
>> activity, and might give us a way to get this out from under the
>> SerializableXactHashLock locking.
> 
> But aren't the values you need to fetch protected by
> SerializableXactHashLock?  Having to take an additional LWLock is
> *not* a "trivial cost".
I was thinking that this would become a more general "commit
sequence number" and could be bundled into the snapshot.  It would
also allow cleaning up the funny double-increment we did as a
workaround for this not being incremented at the actual moment of
commit.  There was actually a patch floated to do it that way near
the end of the 9.1 development cycle.  I imagine that's probably
suffered major bitrot because of Robert's 9.2 work, but the general
idea is the same.
I agree that if it can't fit under existing locks at commit and
snapshot build times, it isn't feasible.
-Kevin

On Wed, Oct 19, 2011 at 05:04:52PM -0400, Tom Lane wrote:
> I wonder whether it would be prudent to set the synchronized-snapshots
> patch aside until you've finished that work (assuming you're actively
> working on it).  It's evidently going to require some nontrivial changes
> in predicate.c, and I don't think the feature should take precedence
> over SSI performance improvement.

I wouldn't hold the patch up on my account. Improving the SSI locking
situation looks to be a fairly substantial project. I've been drawing
up a plan to fix it, but I'm also travelling for most of the next two
weeks and probably won't be able to do any serious hacking on it until
I'm back to the office.

Dan

-- 
Dan R. K. Ports              MIT CSAIL                http://drkp.net/

I think it would be fairly sensible to push some of this into
ProcArray, actually. The commit sequence numbers just involve assigning/
incrementing a global counter when taking a snapshot and finishing a
transaction -- that's not too much work, doesn't require any additional
locking beyond ProcArrayLock, and isn't too tied to SSI. (I could
imagine it being useful for other purposes, though I'm not going to
make that argument too seriously without actually having one in mind.)

SxactGlobalXmin and WritableSxactCount are obviously more SSI-specific,
but I think we can come up with something reasonable to do with them.

The part that's harder is building the list of potential conflicts
that's used to identify safe snapshots for r/o transactions. That
(currently) has to happen atomically taking the snapshot. We'll
probably have to do this in some significantly different way, but I
haven't quite worked out what it is yet.

On the bright side, if we can address these three issues, we shouldn't
need to take SerializableXactHashLock at all when starting a
transaction. (Realistically, we might have to take it or some other
lock shared to handle one of them -- but I really want starting a
serializable xact to not take any exclusive locks.)

Dan

-- 
Dan R. K. Ports              MIT CSAIL                http://drkp.net/

Dan Ports  wrote:
> I think it would be fairly sensible to push some of this into
> ProcArray, actually. The commit sequence numbers just involve
> assigning/incrementing a global counter when taking a snapshot and
> finishing a transaction -- that's not too much work, doesn't
> require any additional locking beyond ProcArrayLock, and isn't too
> tied to SSI. (I could imagine it being useful for other purposes,
> though I'm not going to make that argument too seriously without
> actually having one in mind.)
So far it sounds like we're on the same page.
> SxactGlobalXmin and WritableSxactCount are obviously more
> SSI-specific, but I think we can come up with something reasonable
> to do with them.
Agreed.
> The part that's harder is building the list of potential conflicts
> that's used to identify safe snapshots for r/o transactions. That
> (currently) has to happen atomically taking the snapshot.
That's not obvious to me; could you elaborate on the reasons?  If the
commit sequence number is incremented under cover of an existing
ProcArrayLock, and the current value is assigned to a snapshot under
cover of same, acquiring SerializableXactHashLock after we get the
snapshot seems to work for SxactGlobalXmin and WritableSxactCount,
AFAICS.
> We'll probably have to do this in some significantly different way,
> but I haven't quite worked out what it is yet.
Well, transaction startup needs some rearrangement, clearly.  So far
nothing fundamental has caught my attention beyond the commit
sequence number changes.
> On the bright side, if we can address these three issues, we
> shouldn't need to take SerializableXactHashLock at all when
> starting a transaction. (Realistically, we might have to take it or
> some other lock shared to handle one of them -- but I really want
> starting a serializable xact to not take any exclusive locks.)
Yeah, I don't see how we can avoid taking a LW lock to get a
serializable transaction which needs a SERIALIZABLEXACT set up, but
it might be possible and worthwhile to split the uses of
SerializableXactHashLock so that it's not such a hotspot.
-Kevin

On Thu, Oct 20, 2011 at 07:33:59AM -0500, Kevin Grittner wrote:
> Dan Ports  wrote:
> > The part that's harder is building the list of potential conflicts
> > that's used to identify safe snapshots for r/o transactions. That
> > (currently) has to happen atomically taking the snapshot.
>  
> That's not obvious to me; could you elaborate on the reasons?  If the
> commit sequence number is incremented under cover of an existing
> ProcArrayLock, and the current value is assigned to a snapshot under
> cover of same, acquiring SerializableXactHashLock after we get the
> snapshot seems to work for SxactGlobalXmin and WritableSxactCount,
> AFAICS.

Well, whenever a r/w transaction commits or aborts, we need to check
whether that caused any concurrent r/o transactions to have a
known-safe or unsafe snapshot. The way that happens now is that, when a
r/o transaction starts, it scans the list of r/w transactions and adds
a pointer to itself in their sxact->possibleUnsafeConflicts. When one
of them commits, it scans the list of possible conflicts and does the
appropriate thing.

That's not ideal because:
 - it requires modifing another transaction's sxact when registering a   serializable transaction, so that means taking
 SerializableXactHashLock exclusive.
 
 - the set of concurrent transactions used to identify the possible   conflicts needs to match the one used to build
thesnapshot.   Otherwise, a transaction might commit between when the snapshot is   take and when we find possible
conflicts.(Holding   SerializableXactHashLock prevents this.)
 

> Yeah, I don't see how we can avoid taking a LW lock to get a
> serializable transaction which needs a SERIALIZABLEXACT set up, but
> it might be possible and worthwhile to split the uses of
> SerializableXactHashLock so that it's not such a hotspot.

Oh, right, one other problem is that the sxact free list is also
protected by SerializableXactHashLock, so allocating from it requires
locking. That one could be fixed by protecting it with its own lock, or
(better yet) eventually moving to a lock-free implementation.

In general, the contention problem is that SerializableXactHashLock
basically protects all SSI state except the predicate locks themselves
(notably, the dependency graph). This isn't partitioned at all, so
looking at or modifying a single sxact requires locking the whole
graph. I'd like to replace this with something finer-grained.

Dan

-- 
Dan R. K. Ports              MIT CSAIL                http://drkp.net/

Dan Ports <drkp@csail.mit.edu> writes:
> On Wed, Oct 19, 2011 at 04:36:41PM -0400, Tom Lane wrote:
>> (2) as things stand, xact A need not be running in serializable mode ---
>> if B is serializable, does *that* break any assumptions?

> [taking these in opposite order]

> Yes, I think that's going to be a problem. The obvious case where it's
> clearly not going to work is if A is older than the oldest active
> serializable xact (i.e. SxactGlobalXmin would have to move backwards).
> It's probably possible to make it work when that's not the case, but I
> think it's better to require A to be serializable -- if nothing else,
> it's a far simpler rule to document!

> There is another case that could be problematic, if A was READ ONLY,
> and B isn't. It sounds to me like that would also be a reasonable thing
> to forbid.

I've committed the synchronized-snapshots patch with those two
restrictions, ie, to import a snapshot into a serializable transaction
(1) the source transaction must be serializable (and must still be
running, of course);
(2) you can't import a read-only transaction's snapshot into a
read-write serializable transaction.

I don't understand the SSI code well enough to tell if this is
sufficient or not, so I hope you guys will take a closer look at the
issue when you have time.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't understand the SSI code well enough to tell if this is
> sufficient or not, so I hope you guys will take a closer look at
> the issue when you have time.
I will definitely give it a look.  Right now we have a "perfect
storm" of time demands due to some recently-passed legislation
combined with the need to wrap up some "Annual Plan" items.  Within
a few weeks I should find time for a more careful review and
testing.
-Kevin