[PATCH] Derby 424 - Queryplan for a query using SESSION schema view is incorrectly put in statement cache. This could cause incorrect plan getting executed later if a temp. table is created with that name.

[Mamta Satoor <ms...@gmail.com>]

Hi,
 I have attached a review package for this bug to JIRA, hopefully, in time
for 10.1.2 release.

The files affected by this change are
M java\engine\org\apache\derby\impl\sql\GenericStatement.java
M java\engine\org\apache\derby\impl\sql\compile\FromList.java
M java\engine\org\apache\derby\impl\sql\GenericPreparedStatement.java
M
java\testing\org\apache\derbyTesting\functionTests\tests\lang\declareGlobalTempTableJava.java
M
java\testing\org\apache\derbyTesting\functionTests\master\declareGlobalTempTableJava.out

The changes for this fix are very localized, affecting only 3 files in Derby
engine. Basically, the problem is that, during the compile phase of views,
the reference to the view gets replaced by the view definition, which causes
us to loose the information that the view might have belonged in SESSION
schema. In order to fix this, during the bind phase in FromList, before the
view gets replaced by its definition, I find out if the view is from SESSION
schema, If yes, then I save this information in FromList and this gets used
by FromList when it is asked if it has any items referencing SESSION schema
objects. This information is again lost during the optimization and generate
phase and hence I moved the check for SESSION schema reference to right
after the bind phase in GenericStatement. If there is a reference to SESSION
schema object, GenericStatement will remove the statement from the cache.
I have put in quite a big of comments in the code which hopefully will make
the patch easier to understand. I have added a new test for this and have
run all the tests with no failures using Sun's jdk142 on Windows XP machine.
 thanks,
Mamta

Re: [PATCH] Derby 424 - Queryplan for a query using SESSION schema view is incorrectly put in statement cache. This could cause incorrect plan getting executed later if a temp. table is created with that name.

[Mamta Satoor <ms...@gmail.com>]

Thanks for committing this.
 I think it will be good to port this to 10.1.2 since this bug can lead into
incorrect data returned from a query.

 On 10/20/05, Satheesh Bandaram <sa...@sourcery.org> wrote:
>
> Thanks for more explanation. Patch looks good and is committed. Should I
> port this to 10.1.2 branch?
>
> Sending
> java\engine\org\apache\derby\impl\sql\GenericPreparedStatement.java
> Sending java\engine\org\apache\derby\impl\sql\GenericStatement.java
> Sending java\engine\org\apache\derby\impl\sql\compile\FromList.java
> Sending
> java\testing\org\apache\derbyTesting\functionTests\master\declareGlobalTempTableJava.out
> Sending
> java\testing\org\apache\derbyTesting\functionTests\tests\lang\declareGlobalTempTableJava.java
> Transmitting file data .....
> Committed revision 326959.
>
> Mamta Satoor wrote:
>
> Hi Satheesh,
> First of all, thanks for reviewing the patch for me. Here are the answers
> to your questions 1 and 3.
>  1)If you look at the GenericStatement's prepMinion method, towards the
> beginning(line 167 onwards in the review package that I sent for
> GenericStatement), we look for the statement in the cache. If it is found
> there, we set foundInCache to true. After that, we check if the statement
> found in the cache might be referencing SESSION schema object and if yes,
> then we do not want to use the statement plan from the cache, instead we
> want to build it again. The check to see if statement references SESSION
> schema is done by following code in GenericStatement's prepMinion method
> if (foundInCache) {
> if (preparedStmt.referencesSessionSchema()) {
> // cannot use this state since it is private to a connection.
> // switch to a new statement.
> foundInCache = false;
> preparedStmt = new GenericPreparedStatement(this);
> break;
> }
> }
> GenericPreparedStatement or GenericStatement at the time of the check
> above donot have the query tree object and hence we can't simply call
> qt.referencesSessionSchema. For this reason, I had to add
> public boolean referencesSessionSchema(QueryTreeNode qt) to
> GenericPreparedStatement(which gets called by the compile phase after the qt
> object is constructed) and the method saves the qt object's
> referencesSessionSchema() status in it's own local variable
> referencesSessionSchema. This local information is what will be used by the
> other referencesSessionSchema method to determine if statement found in
> cache should be discarded and compile phase should be re-executed.
>  So, to summarize, the new method after the bind phase, extracts the
> referencesSessionSchema information from the passed qt object and saves it
> in the local variable.
>  public boolean referencesSessionSchema(QueryTreeNode qt)
> throws StandardException {
> //If the query references a SESSION schema table (temporary or permanent),
> then mark so in this statement
> referencesSessionSchema = qt.referencesSessionSchema();
> return(referencesSessionSchema);
> }
>  And, the existing referencesSessionSchema() method as shown below, uses
> the local variable (which was filled correctly last time the statement was
> compiled) to let the compile phase know if it should ignore the plan found
> in the cache and reconstruct the plan because the old plan had references to
> SESSION schema objects.
>  public boolean referencesSessionSchema()
> {
> return referencesSessionSchema;
> }
>
> 3)At the end of the bind phase for select * from session.st1;
> GenericStatement's qt (QueryTreeNode object which in this case is
> CursorNode) object has it's resultSet object as a SelectNode which has a
> fromList object with referencesSessionSchema field set to true because it
> was referencing an object from SESSION schema.
> When the optimize code is called on this bound qt object, the optimizer
> replaces the SelectNode resultSet object with a ProjectRestrictNode and in
> that process, we loose the referencesSessionSchema information which was
> part of the SelectNode's FromList object. Rather than trying to have that
> information some how be transferred to the new ResultSet object, it is more
> efficient to use the information right after bind phase and remove the plan
> from the statement cache.
>  Hope this answers your comments.
> Mamta
>
>  On 10/19/05, Satheesh Bandaram <sa...@sourcery.org> wrote:
> >
> > Thanks Mamta for the patch. I just have some comments and questions...
> > Thanks for commenting the changes well!
> >
> > 1) Why there is a new method in GenericPreparedStatement? This doesn't
> > return any info about the PreparedStatement itself, so, does this belong
> > here? Why not just have the check qt.referencesSessionSchema() in
> > GenericStatement.java ?
> >
> > + public boolean referencesSessionSchema(QueryTreeNode qt)
> > + throws StandardException {
> > + //If the query references a SESSION schema table (temporary or
> > permanent), then mark so in this statement
> > + referencesSessionSchema = qt.referencesSessionSchema();
> > + return(referencesSessionSchema);
> > + }
> >
> > 2) Thanks for changing completeCompile() to NOT return
> > referencesSessionSchema flag... Seems like an ugly way to do it.
> >
> > 3) You also mentioned:
> >
> > This information is again lost during the optimization and generate
> > phase and hence I moved the check for
> > SESSION schema reference to right after the bind phase in
> > GenericStatement.
> >
> > Do you know why this info is lost?
> >
> > Thanks for the good patch.
> >
> > Satheesh
> >
> > Mamta Satoor wrote:
> >
> > Hi,
> >  I have attached a review package for this bug to JIRA, hopefully, in
> > time for 10.1.2 release.
> >
> > The files affected by this change are
> > M java\engine\org\apache\derby\impl\sql\GenericStatement.java
> > M java\engine\org\apache\derby\impl\sql\compile\FromList.java
> > M java\engine\org\apache\derby\impl\sql\GenericPreparedStatement.java
> > M
> > java\testing\org\apache\derbyTesting\functionTests\tests\lang\declareGlobalTempTableJava.java
> > M
> > java\testing\org\apache\derbyTesting\functionTests\master\declareGlobalTempTableJava.out
> >
> > The changes for this fix are very localized, affecting only 3 files in
> > Derby engine. Basically, the problem is that, during the compile phase of
> > views, the reference to the view gets replaced by the view definition, which
> > causes us to loose the information that the view might have belonged in
> > SESSION schema. In order to fix this, during the bind phase in FromList,
> > before the view gets replaced by its definition, I find out if the view is
> > from SESSION schema, If yes, then I save this information in FromList and
> > this gets used by FromList when it is asked if it has any items referencing
> > SESSION schema objects. This information is again lost during the
> > optimization and generate phase and hence I moved the check for SESSION
> > schema reference to right after the bind phase in GenericStatement. If there
> > is a reference to SESSION schema object, GenericStatement will remove the
> > statement from the cache.
> > I have put in quite a big of comments in the code which hopefully will
> > make the patch easier to understand. I have added a new test for this and
> > have run all the tests with no failures using Sun's jdk142 on Windows XP
> > machine.
> >  thanks,
> > Mamta
> >
> >
>

Re: [PATCH] Derby 424 - Queryplan for a query using SESSION schema view is incorrectly put in statement cache. This could cause incorrect plan getting executed later if a temp. table is created with that name.

[Mamta Satoor <ms...@gmail.com>]

Hi Satheesh,
First of all, thanks for reviewing the patch for me. Here are the answers to
your questions 1 and 3.
 1)If you look at the GenericStatement's prepMinion method, towards the
beginning(line 167 onwards in the review package that I sent for
GenericStatement), we look for the statement in the cache. If it is found
there, we set foundInCache to true. After that, we check if the statement
found in the cache might be referencing SESSION schema object and if yes,
then we do not want to use the statement plan from the cache, instead we
want to build it again. The check to see if statement references SESSION
schema is done by following code in GenericStatement's prepMinion method
if (foundInCache) {
if (preparedStmt.referencesSessionSchema()) {
// cannot use this state since it is private to a connection.
// switch to a new statement.
foundInCache = false;
preparedStmt = new GenericPreparedStatement(this);
break;
}
}
GenericPreparedStatement or GenericStatement at the time of the check above
donot have the query tree object and hence we can't simply call
qt.referencesSessionSchema. For this reason, I had to add
public boolean referencesSessionSchema(QueryTreeNode qt) to
GenericPreparedStatement(which gets called by the compile phase after the qt
object is constructed) and the method saves the qt object's
referencesSessionSchema() status in it's own local variable
referencesSessionSchema. This local information is what will be used by the
other referencesSessionSchema method to determine if statement found in
cache should be discarded and compile phase should be re-executed.
 So, to summarize, the new method after the bind phase, extracts the
referencesSessionSchema information from the passed qt object and saves it
in the local variable.
 public boolean referencesSessionSchema(QueryTreeNode qt)
throws StandardException {
//If the query references a SESSION schema table (temporary or permanent),
then mark so in this statement
referencesSessionSchema = qt.referencesSessionSchema();
return(referencesSessionSchema);
}
 And, the existing referencesSessionSchema() method as shown below, uses the
local variable (which was filled correctly last time the statement was
compiled) to let the compile phase know if it should ignore the plan found
in the cache and reconstruct the plan because the old plan had references to
SESSION schema objects.
 public boolean referencesSessionSchema()
{
return referencesSessionSchema;
}

3)At the end of the bind phase for select * from session.st1;
GenericStatement's qt (QueryTreeNode object which in this case is
CursorNode) object has it's resultSet object as a SelectNode which has a
fromList object with referencesSessionSchema field set to true because it
was referencing an object from SESSION schema.
When the optimize code is called on this bound qt object, the optimizer
replaces the SelectNode resultSet object with a ProjectRestrictNode and in
that process, we loose the referencesSessionSchema information which was
part of the SelectNode's FromList object. Rather than trying to have that
information some how be transferred to the new ResultSet object, it is more
efficient to use the information right after bind phase and remove the plan
from the statement cache.
 Hope this answers your comments.
Mamta

 On 10/19/05, Satheesh Bandaram <sa...@sourcery.org> wrote:
>
> Thanks Mamta for the patch. I just have some comments and questions...
> Thanks for commenting the changes well!
>
> 1) Why there is a new method in GenericPreparedStatement? This doesn't
> return any info about the PreparedStatement itself, so, does this belong
> here? Why not just have the check qt.referencesSessionSchema() in
> GenericStatement.java?
>
> + public boolean referencesSessionSchema(QueryTreeNode qt)
> + throws StandardException {
> + //If the query references a SESSION schema table (temporary or
> permanent), then mark so in this statement
> + referencesSessionSchema = qt.referencesSessionSchema();
> + return(referencesSessionSchema);
> + }
>
> 2) Thanks for changing completeCompile() to NOT return
> referencesSessionSchema flag... Seems like an ugly way to do it.
>
> 3) You also mentioned:
>
> This information is again lost during the optimization and generate phase
> and hence I moved the check for
> SESSION schema reference to right after the bind phase in
> GenericStatement.
>
> Do you know why this info is lost?
>
> Thanks for the good patch.
>
> Satheesh
>
> Mamta Satoor wrote:
>
> Hi,
>  I have attached a review package for this bug to JIRA, hopefully, in time
> for 10.1.2 release.
>
> The files affected by this change are
> M java\engine\org\apache\derby\impl\sql\GenericStatement.java
> M java\engine\org\apache\derby\impl\sql\compile\FromList.java
> M java\engine\org\apache\derby\impl\sql\GenericPreparedStatement.java
> M
> java\testing\org\apache\derbyTesting\functionTests\tests\lang\declareGlobalTempTableJava.java
> M
> java\testing\org\apache\derbyTesting\functionTests\master\declareGlobalTempTableJava.out
>
> The changes for this fix are very localized, affecting only 3 files in
> Derby engine. Basically, the problem is that, during the compile phase of
> views, the reference to the view gets replaced by the view definition, which
> causes us to loose the information that the view might have belonged in
> SESSION schema. In order to fix this, during the bind phase in FromList,
> before the view gets replaced by its definition, I find out if the view is
> from SESSION schema, If yes, then I save this information in FromList and
> this gets used by FromList when it is asked if it has any items referencing
> SESSION schema objects. This information is again lost during the
> optimization and generate phase and hence I moved the check for SESSION
> schema reference to right after the bind phase in GenericStatement. If there
> is a reference to SESSION schema object, GenericStatement will remove the
> statement from the cache.
> I have put in quite a big of comments in the code which hopefully will
> make the patch easier to understand. I have added a new test for this and
> have run all the tests with no failures using Sun's jdk142 on Windows XP
> machine.
>  thanks,
> Mamta
>
>