Code review feedback from grandinj

h2/src/main/org/h2/command/Parser.java

@@ -5167,9 +5167,8 @@ public class Parser {
         
         // this WITH statement is not a temporary view - it is part of a persistent view
         // as in CREATE VIEW abc AS WITH my_cte - this auto detects that condition
-        if(session.isParsingView()){
+        if(session.isParsingCreateView()){
             isPersistent = true;
-            System.out.println("getParsingViewName="+session.getParsingViewName());
         }
         
         do {
@@ -5177,6 +5176,10 @@ public class Parser {
         } while (readIf(","));
 
         Prepared p = null;
+        // reverse the order of constructed CTE views - as the destruction order
+        // (since later created view may depend on previously created views - 
+        //  we preserve that dependency order in the destruction sequence )
+        // used in setCteCleanups
         Collections.reverse(viewsCreated);
 
         if(isToken("SELECT")) {
@@ -5224,13 +5227,12 @@ public class Parser {
     }
 
     private TableView parseSingleCommonTableExpression(boolean isPersistent) {
-        Session targetSession = session;
         String cteViewName = readIdentifierWithSchema();
         Schema schema = getSchema();
         Table recursiveTable=null;
         ArrayList<Column> columns = New.arrayList();
         String[] cols = null;
-        Database db = targetSession.getDatabase();
+        Database db = session.getDatabase();
         
         // column names are now optional - they can be inferred from the named
         // query, if not supplied by user
@@ -5245,10 +5247,10 @@ public class Parser {
         
         Table oldViewFound = null;
         if(isPersistent){
-            oldViewFound = getSchema().findTableOrView(targetSession, cteViewName);
+            oldViewFound = getSchema().findTableOrView(session, cteViewName);
         }
         else{
-            oldViewFound = targetSession.findLocalTempTable(cteViewName);
+            oldViewFound = session.findLocalTempTable(cteViewName);
         }
         // this persistent check conflicts with check 10 lines down
         if (oldViewFound != null) {
@@ -5262,11 +5264,11 @@ public class Parser {
                         cteViewName);
             }
             if(isPersistent){
-                oldViewFound.lock(targetSession, true, true);
-                targetSession.getDatabase().removeSchemaObject(targetSession, oldViewFound);                
+                oldViewFound.lock(session, true, true);
+                session.getDatabase().removeSchemaObject(session, oldViewFound);                
                 
             }else{
-                targetSession.removeLocalTempTable(oldViewFound);
+                session.removeLocalTempTable(oldViewFound);
             }
             oldViewFound=null;
         }
@@ -5276,7 +5278,7 @@ public class Parser {
         // to work (its removed after creation in this method)
         // only create table data and table if we don't have a working CTE already
         if(oldViewFound == null){
-            recursiveTable = createShadowTableForRecursiveTableExpression(isPersistent, targetSession, cteViewName,
+            recursiveTable = createShadowTableForRecursiveTableExpression(isPersistent, session, cteViewName,
                     schema, columns, db);
         }
         List<Column> columnTemplateList;
@@ -5286,20 +5288,20 @@ public class Parser {
             read("(");
             Query withQuery = parseSelect();
             if(isPersistent){
-                withQuery.session = targetSession;
+                withQuery.session = session;
             }            
             read(")");
             columnTemplateList = createQueryColumnTemplateList(cols, withQuery, querySQLOutput);
 
         } finally {
-            destroyShadowTableForRecursiveExpression(isPersistent, targetSession, recursiveTable);
+            destroyShadowTableForRecursiveExpression(isPersistent, session, recursiveTable);
         }
 
         TableView view = createCTEView(cteViewName,
                 querySQLOutput[0], columnTemplateList,
                 true/* allowRecursiveQueryDetection */, 
                 true/* add to session */, 
-                isPersistent, targetSession);
+                isPersistent, session);
         
         return view;
     }
@@ -5452,12 +5454,12 @@ public class Parser {
         read("AS");
         try {
             Query query;
-            session.setParsingView(true,viewName);
+            session.setParsingCreateView(true,viewName);
             try {
                 query = parseSelect();
                 query.prepare();
             } finally {
-                session.setParsingView(false,viewName);
+                session.setParsingCreateView(false,viewName);
             }
             command.setSelect(query);
         } catch (DbException e) {

h2/src/main/org/h2/command/ddl/DropView.java

@@ -77,7 +77,6 @@ public class DropView extends SchemaCommand {
             
             view.lock(session, true, true);
             session.getDatabase().removeSchemaObject(session, view);
-            session.getDatabase().unlockMeta(session);
 
             // remove dependent table expressions
             for( Table childTable: copyOfDependencies){
@@ -88,6 +87,8 @@ public class DropView extends SchemaCommand {
                     }
                 }
             }
+            // make sure its all unlocked
+            session.getDatabase().unlockMeta(session);
         }
         return 0;
     }

h2/src/main/org/h2/command/dml/Select.java

@@ -875,7 +875,7 @@ public class Select extends Query {
                 isQuickAggregateQuery = isEverything(optimizable);
             }
         }
-        cost = preparePlan(session.isParsingView());
+        cost = preparePlan(session.isParsingCreateView());
         if (distinct && session.getDatabase().getSettings().optimizeDistinct &&
                 !isGroupQuery && filters.size() == 1 &&
                 expressions.size() == 1 && condition == null) {
@@ -1087,7 +1087,7 @@ public class Select extends Query {
         for (TableFilter f : topFilters) {
             Table t = f.getTable();
             TableView tableView = (t instanceof TableView) ? (TableView) t : null;
-            if (tableView!=null && tableView.isView() && tableView.isRecursive() && tableView.isTableExpression()) {
+            if (tableView!=null && tableView.isRecursive() && tableView.isTableExpression()) {
                 
                 if(tableView.isPersistent()){
                     // skip the generation of plan SQL for this already recursive persistent ctes, since using a with 

h2/src/main/org/h2/engine/Database.java

@@ -1878,7 +1878,6 @@ public class Database implements DataHandler {
         int type = obj.getType();
         if (type == DbObject.TABLE_OR_VIEW) {
             Table table = (Table) obj;
-            //table.setBeingDropped(true);
             if (table.isTemporary() && !table.isGlobalTemporary()) {
                 session.removeLocalTempTable(table);
                 return;
@@ -1918,7 +1917,6 @@ public class Database implements DataHandler {
                 
             }
             removeMeta(session, id);
-            return;
         }
     }
 

h2/src/main/org/h2/engine/Session.java

@@ -6,7 +6,6 @@
 package org.h2.engine;
 
 import java.util.ArrayList;
-import java.util.EmptyStackException;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -229,7 +228,7 @@ public class Session extends SessionWithState {
         return subQueryInfo;
     }
 
-    public void setParsingView(boolean parsingView, String viewName) {
+    public void setParsingCreateView(boolean parsingView, String viewName) {
         // It can be recursive, thus implemented as counter.
         this.parsingView += parsingView ? 1 : -1;
         assert this.parsingView >= 0;
@@ -241,16 +240,14 @@ public class Session extends SessionWithState {
             viewNameStack.pop();
         }        
     }
-    public String getParsingViewName() {
-        try{
-        return viewNameStack.peek();
-        }
-        catch(EmptyStackException e){
+    public String getParsingCreateViewName() {
+        if(viewNameStack.size()==0){
             return null;
         }
+        return viewNameStack.peek();
     }
 
-    public boolean isParsingView() {
+    public boolean isParsingCreateView() {
         assert parsingView >= 0;
         return parsingView != 0;
     }

h2/src/main/org/h2/table/Table.java

@@ -161,6 +161,7 @@ public abstract class Table extends SchemaObjectBase {
      * @param key the primary key
      * @return the row
      */
+    @SuppressWarnings("unused")
     public Row getRow(Session session, long key) {
         return null;
     }

h2/src/main/org/h2/table/TableView.java

@@ -112,11 +112,11 @@ public class TableView extends Table {
 
     private Query compileViewQuery(Session session, String sql, boolean literalsChecked, String viewName) {
         Prepared p;
-        session.setParsingView(true,viewName);
+        session.setParsingCreateView(true,viewName);
         try {
             p = session.prepare(sql, false, literalsChecked);
         } finally {
-            session.setParsingView(false,viewName);
+            session.setParsingCreateView(false,viewName);
         }
         if (!(p instanceof Query)) {
             throw DbException.getSyntaxError(sql, 0);