db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mamta Satoor <msat...@gmail.com>
Subject 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.
Date Thu, 20 Oct 2005 18:00:07 GMT
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 <satheesh@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 <satheesh@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
> >
> >
>

Mime
View raw message