Thread: Re: [Farrago-developers] Eigenbase perforce change 9342 for review

In our world they are not always sharable. And at this stage in the
process, we don't know whether they are sharable, because whether they
are sharable depends on the implementation chosen.

What we actually needed here is exclsuve-wait option here: if someone
else is preparing the same SQL, don't pin their entry (because it might
be incomplete), and don't go and create your own, but wait until they
have finished preparing it.

Julian

> -----Original Message-----
> From: John V. Sichi [mailto:js...@gm...] 
> Sent: Monday, May 28, 2007 1:52 AM
> To: Julian Hyde
> Subject: Re: Eigenbase perforce change 9342 for review
> 
> I haven't tracked down where the change below originated, but it's 
> wrong.  Executable statements are pure and sharable, so they aren't 
> supposed to be pinned exclusively.  I'll revert it on //open/dev.
> 
> PIC says we're getting very close now...I'm integrating a 
> last batch of 
> changes up soon.
> 
> JVS
> 
> Julian Hyde wrote:
> > http://p4web.eigenbase.org/@md=d&c=6PU@//9342?ac=10
> > 
> > Change 9342 by jh...@jh...rmalade3 on 2007/05/26 00:25:23
> > 
> > 	DEV: Integrate from //open/dt/dev/...@9326
> > ==== 
> //open/dev/farrago/src/net/sf/farrago/db/FarragoDatabase.java#
> 59 (ktext) ====
> > 880c880
> > <             cacheEntry = codeCache.pin(sql, stmtFactory, false);
> > ---
> >>             cacheEntry = codeCache.pin(sql, stmtFactory, true);
> 

Julian Hyde wrote:
> In our world they are not always sharable. And at this stage in the
> process, we don't know whether they are sharable, because whether they
> are sharable depends on the implementation chosen.

In that case, the correct thing to do is to introduce an extension point 
in the session personality SPI, and leave the Farrago default as is 
(sharable).

> What we actually needed here is exclsuve-wait option here: if someone
> else is preparing the same SQL, don't pin their entry (because it might
> be incomplete), and don't go and create your own, but wait until they
> have finished preparing it.

FarragoObjectCache sharable pin behavior is currently to pin the 
existing entry, and if it's still under construction, wait for the 
construction to complete.  For an executable statement, construction is 
actually preparation.  Search for constructionThread inside of 
FarragoObjectCache.pin.

JVS

> FarragoObjectCache sharable pin behavior is currently to pin the 
> existing entry, and if it's still under construction, wait for the 
> construction to complete.  For an executable statement, 
> construction is 
> actually preparation.  Search for constructionThread inside of 
> FarragoObjectCache.pin.

That behavior didn't work for us. Note that the exclusivity is a
property of the pin operation, and is forgotten once the object is
pinned. For what it's worth, for this purpose, the ideal model would be
for an object to be 'pinned exclusive', meaning no one else can pin it,
have other users block until the object has figured out whether it is
going to be sharable, then for the object to make itself non-exclusive.

I guess we'll add a method to the personality api so that we are always
exclusive - two sql statements get different plans even if they are
textually identical.

Julian

Yes, extending the session personality SPI is the simplest way to 
address it for now.

I've added a link to this thread from

http://issues.eigenbase.org/browse/FRG-263

since the existing incorrect behavior there is semi-related to the 
enhancement you describe below.

JVS

Julian Hyde wrote:
>> FarragoObjectCache sharable pin behavior is currently to pin the 
>> existing entry, and if it's still under construction, wait for the 
>> construction to complete.  For an executable statement, 
>> construction is 
>> actually preparation.  Search for constructionThread inside of 
>> FarragoObjectCache.pin.
> 
> That behavior didn't work for us. Note that the exclusivity is a
> property of the pin operation, and is forgotten once the object is
> pinned. For what it's worth, for this purpose, the ideal model would be
> for an object to be 'pinned exclusive', meaning no one else can pin it,
> have other users block until the object has figured out whether it is
> going to be sharable, then for the object to make itself non-exclusive.
> 
> I guess we'll add a method to the personality api so that we are always
> exclusive - two sql statements get different plans even if they are
> textually identical.
> 
> Julian
> 
> 

After studying the FarragoObjectCache/FarragoDatabase interactions some 
more, I think I can fix all of the issues raised in FRG-263, including 
yours (along the lines of what you wrote below).  Let me know if anyone 
sees problems with the proposal below.  (Besides correctness, think 
about whether the proposed interface changes would break any red-zone 
dependencies.)

For sharability:  we already have the "wait and see" logic inside of 
FarragoObjectCache.pin (via the "constructionThread" flag).  What I will 
do is change the CachedObjectFactory.initializeMethod to return a 
boolean.  If it returns false, it will indicate that the value turned 
out not to be cacheable.  FarragoObjectCache will see this and flag the 
entry accordingly, and also detach it immediately.  The original caller 
to pin can check this flag and know that they got back a detached entry 
(meaning no need to unpin/detach; this will allow us to get rid of the 
public detach call).  Likewise, other callers who were waiting on the 
object construction results will see the flag and give up on that entry; 
I'll make them restart the attempt from the very top of pin in that case.

For staleness:  I will add an isStale(Object) method to 
CachedObjectFactory.  During the initial map lookup of pin, when 
FarragoObjectCache gets a hit, it will call isStale to test the existing 
object.  If the object is not stale, all proceeds as normal.  If the 
object is stale, AND the entry's pin count is 0, then FarragoObjectCache 
will immediately discard the stale entry and proceed as if the lookup 
had been a miss.  Otherwise, the object is stale, but someone else has 
it pinned; leave the entry alone, but proceed as if the lookup had been 
a miss.  For FarragoDatabase, higher-level locking should prevent this 
last case from ever occurring; the reason something like it currently 
comes up in FRG-263 is that the two threads simultaneously pin the stale 
entry, and then the first one discards it, hitting the assertion because 
the other one still has it pinned.  Once we move the staleness test 
inside of the synchronized map lookup, this shouldn't happen.  One 
assumption here is that a newly initialized object should never be 
stale-on-arrival, which should be the case for FarragoDatabase, again 
due to higher-level locking.  This will allow us to get rid of the 
staleness retry loop in FarragoDatabase.

Yes, FarragoObjectCache really needs its own unit test.  Lots of 
potential ways I can get the synchronization wrong here.

JVS

John V. Sichi wrote:
> Yes, extending the session personality SPI is the simplest way to 
> address it for now.
> 
> I've added a link to this thread from
> 
> http://issues.eigenbase.org/browse/FRG-263
> 
> since the existing incorrect behavior there is semi-related to the 
> enhancement you describe below.
> 
> JVS
> 
> Julian Hyde wrote:
>>> FarragoObjectCache sharable pin behavior is currently to pin the 
>>> existing entry, and if it's still under construction, wait for the 
>>> construction to complete.  For an executable statement, construction 
>>> is actually preparation.  Search for constructionThread inside of 
>>> FarragoObjectCache.pin.
>>
>> That behavior didn't work for us. Note that the exclusivity is a
>> property of the pin operation, and is forgotten once the object is
>> pinned. For what it's worth, for this purpose, the ideal model would be
>> for an object to be 'pinned exclusive', meaning no one else can pin it,
>> have other users block until the object has figured out whether it is
>> going to be sharable, then for the object to make itself non-exclusive.
>>
>> I guess we'll add a method to the personality api so that we are always
>> exclusive - two sql statements get different plans even if they are
>> textually identical.
>>
>> Julian
>>
>>
> 
> 

Fixes are checked in on //open/dev as eigenchange 9448, along with 
spiffy new unit test.  This code is tricky, so it could really use a 
review from anyone with the cycles.  I'm going to run through some more 
SQL-level concurrency tests on a 4-way box.

FarragoObjectCache.pin is enormous now; I did not break it up because I 
wanted to make code reviewing is easier, but I'll follow on with 
breaking it up after review.

The actual changes are slightly different from what I described in the 
message below.  Rather than making CachedObjectFactory.initializeEntry 
return boolean, I changed UninitializedEntry.initialize to take the 
boolean as a third parameter.  Also, instead of detaching the entry and 
returning it to the caller unpinned, I made it return pinned as normal, 
so it will stay around in the cache as junk rather than being 
immediately discarded.  Discard is deferred to the same place where I 
now handle staleness checking.  This simplified things considerably.

After reading the code, you might think that a further simplification is 
possible:  why not treat non-reusability as a special case of staleness? 
  The reason is that, at least in the SQL statement context we're 
interested in, reusability is static and can only be determined from the 
FarragoPreparingStmt, whereas staleness is time-varying and can be 
determined from FarragoExecutableStmt.  Hence, we flag for the former 
(so that we can release the memory-hungry FarragoPreparingStmt 
reference), whereas we check dynamically for the latter.  The 
alternative would be to lie and say that a non-resuable statement is 
stale on arrival, but that seems muddy to me.

JVS

John V. Sichi wrote:
> After studying the FarragoObjectCache/FarragoDatabase interactions some 
> more, I think I can fix all of the issues raised in FRG-263, including 
> yours (along the lines of what you wrote below).  Let me know if anyone 
> sees problems with the proposal below.  (Besides correctness, think 
> about whether the proposed interface changes would break any red-zone 
> dependencies.)
> 
> For sharability:  we already have the "wait and see" logic inside of 
> FarragoObjectCache.pin (via the "constructionThread" flag).  What I will 
> do is change the CachedObjectFactory.initializeMethod to return a 
> boolean.  If it returns false, it will indicate that the value turned 
> out not to be cacheable.  FarragoObjectCache will see this and flag the 
> entry accordingly, and also detach it immediately.  The original caller 
> to pin can check this flag and know that they got back a detached entry 
> (meaning no need to unpin/detach; this will allow us to get rid of the 
> public detach call).  Likewise, other callers who were waiting on the 
> object construction results will see the flag and give up on that entry; 
> I'll make them restart the attempt from the very top of pin in that case.
> 
> For staleness:  I will add an isStale(Object) method to 
> CachedObjectFactory.  During the initial map lookup of pin, when 
> FarragoObjectCache gets a hit, it will call isStale to test the existing 
> object.  If the object is not stale, all proceeds as normal.  If the 
> object is stale, AND the entry's pin count is 0, then FarragoObjectCache 
> will immediately discard the stale entry and proceed as if the lookup 
> had been a miss.  Otherwise, the object is stale, but someone else has 
> it pinned; leave the entry alone, but proceed as if the lookup had been 
> a miss.  For FarragoDatabase, higher-level locking should prevent this 
> last case from ever occurring; the reason something like it currently 
> comes up in FRG-263 is that the two threads simultaneously pin the stale 
> entry, and then the first one discards it, hitting the assertion because 
> the other one still has it pinned.  Once we move the staleness test 
> inside of the synchronized map lookup, this shouldn't happen.  One 
> assumption here is that a newly initialized object should never be 
> stale-on-arrival, which should be the case for FarragoDatabase, again 
> due to higher-level locking.  This will allow us to get rid of the 
> staleness retry loop in FarragoDatabase.
> 
> Yes, FarragoObjectCache really needs its own unit test.  Lots of 
> potential ways I can get the synchronization wrong here.
> 
> JVS
> 
> John V. Sichi wrote:
>> Yes, extending the session personality SPI is the simplest way to 
>> address it for now.
>>
>> I've added a link to this thread from
>>
>> http://issues.eigenbase.org/browse/FRG-263
>>
>> since the existing incorrect behavior there is semi-related to the 
>> enhancement you describe below.
>>
>> JVS
>>
>> Julian Hyde wrote:
>>>> FarragoObjectCache sharable pin behavior is currently to pin the 
>>>> existing entry, and if it's still under construction, wait for the 
>>>> construction to complete.  For an executable statement, construction 
>>>> is actually preparation.  Search for constructionThread inside of 
>>>> FarragoObjectCache.pin.
>>>
>>> That behavior didn't work for us. Note that the exclusivity is a
>>> property of the pin operation, and is forgotten once the object is
>>> pinned. For what it's worth, for this purpose, the ideal model would be
>>> for an object to be 'pinned exclusive', meaning no one else can pin it,
>>> have other users block until the object has figured out whether it is
>>> going to be sharable, then for the object to make itself non-exclusive.
>>>
>>> I guess we'll add a method to the personality api so that we are always
>>> exclusive - two sql statements get different plans even if they are
>>> textually identical.
>>>
>>> Julian
>>>
>>>
>>
>>
> 
>