Merge lp:~tjoneslo/akiban-server/fix-bug-1199111 into lp:~akiban-technologies/akiban-server/trunk

[Thomas Jones-Low]

Fix bug 1199111 - Where an failed alter table followed by a second one on a group table would return an incorrect error, and then fail to allow any further alters on the table(s) in the group.

This bug was a little subtler than described in the bug 1199111 report.

The core change is in the AbstractSchemaManager where the process of updating the table version for the tableVersionMap is deferred until the transaction for the DDL is committed. In this way, if the DDL fails, and the transaction is rolled back, the tableVersionMap is not updated.

The subtle problem is that the table version stored with the table needs to be updated immediately so that when the AIS is frozen and stored, it has the correct table version. But the tableVersionMap (used for lock checking) is now deferred until much later. This mostly isn't a problem because the tables affected by the Alter DDL are locked, meaning nothing else is reading them.

The exception is the tables used in group indexes which affected indirectly by a table change. That is, if there is a group with tables A, B, C, and a group index on all three tables, and the ALTER changes a column in table C which is also in the group index. The group index maintenance is to drop the group index, perform the table change, then recreate the group index. In this case we know that the group index will be re-created on tables A and B (and possibly C), but those tables are not locked during the DDL processing. So don't change the table version on tables A and B while doing the group index maintenance.

AbstractSchemaManager - change the bumpTableVersion() to trackBumpTableVersions(). Add the code for the trackBumpTableVersion. This does it work through the transaction service add callback functionality.

As described above, the dropIndexes() now skips the bumpTableVersion process if this is part of the group index drop/create. Add a flag to the dropIndex to indicate this. This has a refactoring change to the callers.

PersistitStoreSchemaManager - the full version of the trackBumpTableVersion() as this class has access to the TransactionService.

AlterTableHelper - dropAffectedGroupIndexes() has the call to use the skip update table as this is the source of the group index drop indexes call.

test-bug-1199111.yaml - tests of the original problem, with variations.

[Nathan Williams (nwilliams)]

This looks good.

I think we can move the PSSM#trackBumpTableVersion() to the base class though. Just need to pass TransactionService to it. That way the FDBSchemaManager gets in on the fun for free.

Feel free to big-A after that.

review: Approve

[lp:~tjoneslo/akiban-server/fix-bug-1199111]

2713. By tjoneslo on 2013-07-29

Move the storage of the TransactionService to the AbstractSchemaManager. This allows the moving all the bumbTableVersions() code to the ASM. This will now work better with the FDBSchemaManager when that merge occurs.

=== modified file 'src/main/java/com/akiban/server/service/dxl/AlterTableHelper.java'
--- src/main/java/com/akiban/server/service/dxl/AlterTableHelper.java	2013-07-05 21:24:06 +0000
+++ src/main/java/com/akiban/server/service/dxl/AlterTableHelper.java	2013-07-29 15:28:42 +0000
@@ -110,7 +110,7 @@
         for(IndexName name : affectedGroupIndexes.keySet()) {
             groupIndexes.add(origTable.getGroup().getIndex(name.getName()));
         }
-        ddl.schemaManager().dropIndexes(session, groupIndexes);
+        ddl.schemaManager().dropIndexes(session, groupIndexes, true);
     }
 
     public void createAffectedGroupIndexes(Session session, BasicDDLFunctions ddl, UserTable origTable, UserTable newTable, boolean dataChange) {

=== modified file 'src/main/java/com/akiban/server/service/dxl/BasicDDLFunctions.java'
--- src/main/java/com/akiban/server/service/dxl/BasicDDLFunctions.java	2013-07-25 18:32:49 +0000
+++ src/main/java/com/akiban/server/service/dxl/BasicDDLFunctions.java	2013-07-29 15:28:42 +0000
@@ -1143,7 +1143,7 @@
                 }
                 indexes.add(index);
             }
-            schemaManager().dropIndexes(session, indexes);
+            schemaManager().dropIndexes(session, indexes, false);
             store().deleteIndexes(session, indexes);
             for(TableListener listener : listenerService.getTableListeners()) {
                 listener.onDropIndex(session, indexes);
@@ -1195,7 +1195,7 @@
                 }
                 indexes.add(index);
             }
-            schemaManager().dropIndexes(session, indexes);
+            schemaManager().dropIndexes(session, indexes, false);
             store().deleteIndexes(session, indexes);
             for(TableListener listener : listenerService.getTableListeners()) {
                 listener.onDropIndex(session, indexes);

=== modified file 'src/main/java/com/akiban/server/store/AbstractSchemaManager.java'
--- src/main/java/com/akiban/server/store/AbstractSchemaManager.java	2013-07-25 16:10:58 +0000
+++ src/main/java/com/akiban/server/store/AbstractSchemaManager.java	2013-07-29 15:28:42 +0000
@@ -62,6 +62,7 @@
 import com.akiban.server.service.security.SecurityService;
 import com.akiban.server.service.session.Session;
 import com.akiban.server.service.session.SessionService;
+import com.akiban.server.service.transaction.TransactionService;
 import com.akiban.server.util.ReadWriteMap;
 import com.akiban.util.ArgumentValidation;
 import com.persistit.exception.PersistitException;
@@ -100,15 +101,19 @@
 
     protected final SessionService sessionService;
     protected final ConfigurationService config;
+    protected final TransactionService txnService;
+
     protected SecurityService securityService;
     protected int maxAISBufferSize;
     protected SerializationType serializationType = SerializationType.NONE;
     protected ReadWriteMap<Integer,Integer> tableVersionMap;
     protected Map<TableName,MemoryTableFactory> memoryTableFactories;
 
-    protected AbstractSchemaManager(ConfigurationService config, SessionService sessionService) {
+    protected AbstractSchemaManager(ConfigurationService config, SessionService sessionService,
+            TransactionService txnService) {
         this.config = config;
         this.sessionService = sessionService;
+        this.txnService = txnService;
     }
 
 
@@ -255,6 +260,12 @@
 
     @Override
     public Collection<Index> createIndexes(Session session, Collection<? extends Index> indexesToAdd, boolean keepTree) {
+        
+        // Per the interface specification, if the index list is empty do nothing
+        // Avoid doing an empty merge too. 
+        if (indexesToAdd.isEmpty()) { 
+            return Collections.emptyList();
+        }
         AISMerge merge = AISMerge.newForAddIndex(getNameGenerator(), getAis(session));
         Set<String> schemas = new HashSet<>();
 
@@ -271,14 +282,14 @@
         }
         merge.merge();
         AkibanInformationSchema newAIS = merge.getAIS();
-        bumpTableVersions(newAIS, tableIDs);
+        trackBumpTableVersion(session, newAIS, tableIDs);
 
         saveAISChangeWithRowDefs(session, newAIS, schemas);
         return newIndexes;
     }
 
     @Override
-    public void dropIndexes(Session session, final Collection<? extends Index> indexesToDrop) {
+    public void dropIndexes(Session session, final Collection<? extends Index> indexesToDrop, boolean temporary) {
         final AkibanInformationSchema newAIS = AISCloner.clone(
                 getAis(session),
                 new ProtobufWriter.TableSelector() {
@@ -293,12 +304,13 @@
                     }
         });
 
-        Collection<Integer> tableIDs = new ArrayList<>(indexesToDrop.size());
-        for(Index index : indexesToDrop) {
-            tableIDs.addAll(index.getAllTableIDs());
+        if (!temporary) {
+            Collection<Integer> tableIDs = new ArrayList<>(indexesToDrop.size());
+            for(Index index : indexesToDrop) {
+                tableIDs.addAll(index.getAllTableIDs());
+            }
+            trackBumpTableVersion(session, newAIS, tableIDs);
         }
-        bumpTableVersions(newAIS, tableIDs);
-
         final Set<String> schemas = new HashSet<>();
         for(Index index : indexesToDrop) {
             schemas.add(DefaultNameGenerator.schemaNameForIndex(index));
@@ -338,7 +350,7 @@
         AISMerge merge = AISMerge.newForModifyTable(getNameGenerator(), getAis(session), alteredTables);
         merge.merge();
         AkibanInformationSchema newAIS = merge.getAIS();
-        bumpTableVersions(newAIS, tableIDs);
+        trackBumpTableVersion(session,newAIS, tableIDs);
 
         // This is hacky. PK trees have to be preserved because there is no way to duplicate
         // accumulator state that shouldn't change. But ordinals are stored in accumulators
@@ -645,7 +657,7 @@
 
         final AkibanInformationSchema oldAIS = getAis(session);
         final AkibanInformationSchema newAIS = removeTablesFromAIS(session, tables, sequences);
-        bumpTableVersions(newAIS, tableIDs);
+        trackBumpTableVersion(session, newAIS, tableIDs);
 
         for(Integer tableID : tableIDs) {
             clearTableStatus(session, oldAIS.getUserTable(tableID));
@@ -728,6 +740,58 @@
         );
     }
 
+    protected void trackBumpTableVersion (Session session, AkibanInformationSchema newAIS, Collection<Integer> allTableIDs)
+    {
+        // Set the new table version  for tables in the NewAIS
+        for(Integer tableID : allTableIDs) {
+            Integer current = tableVersionMap.get(tableID);
+            Integer update = (current == null) ? 1 : current + 1;
+            UserTable table = newAIS.getUserTable(tableID);
+            if(table != null) { // From drop
+                table.setVersion(update);
+            }
+        }
+        // Schedule the update for the tableVersionMap version number on commit.
+        // Replace any existing map as we only should have one at at time.
+        // for one AIS. 
+        // There may be two of these, the first for an alter table, 
+        // the second for group indexes affected by the change. 
+        Map<AkibanInformationSchema,Collection<Integer>> map = session.get(TABLE_VERSIONS);
+        if(map == null) {
+            map = new HashMap<>();
+            session.put(TABLE_VERSIONS, map);
+        }
+        map.put(newAIS, allTableIDs);
+        txnService.addCallback(session, TransactionService.CallbackType.COMMIT, bumpTableVersionCommit);
+        txnService.addCallback(session, TransactionService.CallbackType.END, cleanTableVersion);
+    }
+
+    protected final static Session.MapKey<AkibanInformationSchema, Collection<Integer>> TABLE_VERSIONS = 
+            Session.MapKey.mapNamed("TABLE_VERSIONS");
+
+    // If the Alter table fails, make sure to clean up the TABLE_VERSION change list on end
+    // If the Alter succeeds, the bumpTableVersionCommit process will clean up, and this does nothing. 
+    protected final TransactionService.Callback cleanTableVersion = new TransactionService.Callback() {
+        
+        @Override
+        public void run(Session session, long timestamp) {
+            session.remove(TABLE_VERSIONS);
+        }
+    };
+    
+    
+    protected final TransactionService.Callback bumpTableVersionCommit = new TransactionService.Callback() {
+        @Override
+        public void run(Session session, long timestamp) {
+            Map<AkibanInformationSchema,Collection<Integer>> tableIDs = session.remove(TABLE_VERSIONS);
+            if (tableIDs != null) {
+                for(Map.Entry<AkibanInformationSchema, Collection<Integer>> entry : tableIDs.entrySet()) {
+                    bumpTableVersions(entry.getKey(), entry.getValue());
+                }
+            }
+        }
+    };
+    
     private void bumpTableVersions(AkibanInformationSchema newAIS, Collection<Integer> allTableIDs) {
         for(Integer tableID : allTableIDs) {
             Integer current = tableVersionMap.get(tableID);
@@ -737,10 +801,6 @@
             if(!success) {
                 throw new IllegalStateException("Unexpected concurrent DDL on table: " + tableID);
             }
-            UserTable table = newAIS.getUserTable(tableID);
-            if(table != null) { // From drop
-                table.setVersion(update);
-            }
         }
     }
 

=== modified file 'src/main/java/com/akiban/server/store/AbstractStore.java'
--- src/main/java/com/akiban/server/store/AbstractStore.java	2013-07-11 14:20:38 +0000
+++ src/main/java/com/akiban/server/store/AbstractStore.java	2013-07-29 15:28:42 +0000
@@ -35,7 +35,6 @@
 import com.akiban.qp.operator.GroupCursor;
 import com.akiban.qp.operator.Operator;
 import com.akiban.qp.operator.QueryBindings;
-import com.akiban.qp.operator.QueryBindingsCursor;
 import com.akiban.qp.operator.QueryContext;
 import com.akiban.qp.operator.SimpleQueryContext;
 import com.akiban.qp.operator.StoreAdapter;
@@ -90,7 +89,7 @@
 import java.util.Set;
 
 public abstract class AbstractStore<SDType> implements Store {
-    private static final Logger LOG = LoggerFactory.getLogger(AbstractStore.class.getName());
+    private static final Logger LOG = LoggerFactory.getLogger(AbstractStore.class);
 
     private static final InOutTap WRITE_ROW_TAP = Tap.createTimer("write: write_row");
     private static final InOutTap DELETE_ROW_TAP = Tap.createTimer("write: delete_row");
@@ -1024,10 +1023,10 @@
         final int tableID = rowDef.getRowDefId();
 
         // Since this is called on a per-row basis, we can't rely on re-entrancy.
-        if(lockService.isTableClaimed(session, mode, tableID)) {
+        if(lockService.isTableClaimed(session, mode, tableID) ||
+                lockService.isTableClaimed(session, LockService.Mode.EXCLUSIVE, tableID)) {
             return;
         }
-
         /*
          * No need to retry locks or back off already acquired. Other locker is DDL
          * and it performs needed backoff to prevent deadlocks.

=== modified file 'src/main/java/com/akiban/server/store/PersistitStoreSchemaManager.java'
--- src/main/java/com/akiban/server/store/PersistitStoreSchemaManager.java	2013-07-25 19:53:43 +0000
+++ src/main/java/com/akiban/server/store/PersistitStoreSchemaManager.java	2013-07-29 15:28:42 +0000
@@ -124,7 +124,7 @@
  * </p>
  * </p>
  */
-public class PersistitStoreSchemaManager extends AbstractSchemaManager implements Service {
+public class PersistitStoreSchemaManager extends AbstractSchemaManager {
     private static enum GenValue { NEW, SNAPSHOT }
     private static enum GenMap { PUT_NEW, NO_PUT }
 
@@ -205,7 +205,6 @@
     private static final Logger LOG = LoggerFactory.getLogger(PersistitStoreSchemaManager.class.getName());
 
     private final TreeService treeService;
-    private final TransactionService txnService;
     private RowDefCache rowDefCache;
     private NameGenerator nameGenerator;
     private AtomicLong delayedTreeIDGenerator;
@@ -222,9 +221,8 @@
     @Inject
     public PersistitStoreSchemaManager(ConfigurationService config, SessionService sessionService,
                                        TreeService treeService, TransactionService txnService) {
-        super(config, sessionService);
+        super(config, sessionService, txnService);
         this.treeService = treeService;
-        this.txnService = txnService;
     }
 
     @Override
@@ -912,20 +910,6 @@
         }
     }
 
-    @Override
-    public boolean hasTableChanged(Session session, int tableID) {
-        UserTable table = getAis(session).getUserTable(tableID);
-        if(table == null) {
-            throw new IllegalStateException("Unknown table: " + tableID);
-        }
-        Integer curVer = tableVersionMap.get(tableID);
-        Integer tableVer = table.getVersion();
-        if(curVer == null) {
-            return tableVer != null;
-        }
-        return !curVer.equals(tableVer);
-    }
-
     private Accumulator.SeqAccumulator getGenerationAccumulator(Session session) throws PersistitException {
         // treespace policy could split the _schema_ tree across volumes and give us multiple accumulators, which would
         // be very bad. Work around that with a fake/constant schema name. It isn't a problem if this somehow got changed

=== modified file 'src/main/java/com/akiban/server/store/SchemaManager.java'
--- src/main/java/com/akiban/server/store/SchemaManager.java	2013-07-25 16:10:58 +0000
+++ src/main/java/com/akiban/server/store/SchemaManager.java	2013-07-29 15:28:42 +0000
@@ -118,8 +118,11 @@
      * supported through this interface.
      * @param session Session to operate under.
      * @param indexes List of indexes to drop.
+     * @param index drop is temporary - True means indexes will be recreated in
+     *      the same transaction as part of an alter. False means this is the only
+     *      change. 
      */
-    void dropIndexes(Session session, Collection<? extends Index> indexes);
+    void dropIndexes(Session session, Collection<? extends Index> indexes, boolean temporary);
 
     /**
      * Delete the definition of the table with the given name. Throws

=== modified file 'src/test/java/com/akiban/server/service/is/BasicInfoSchemaTablesServiceImplTest.java'
--- src/test/java/com/akiban/server/service/is/BasicInfoSchemaTablesServiceImplTest.java	2013-07-25 16:10:58 +0000
+++ src/test/java/com/akiban/server/service/is/BasicInfoSchemaTablesServiceImplTest.java	2013-07-29 15:28:42 +0000
@@ -705,7 +705,7 @@
         }
 
         @Override
-        public void dropIndexes(Session session, Collection<? extends Index> indexes) {
+        public void dropIndexes(Session session, Collection<? extends Index> indexes, boolean temporary) {
             throw new UnsupportedOperationException();
         }
 

=== added file 'src/test/resources/com/akiban/sql/pg/yaml/bugs/test-bug-1199111.yaml'
--- src/test/resources/com/akiban/sql/pg/yaml/bugs/test-bug-1199111.yaml	1970-01-01 00:00:00 +0000
+++ src/test/resources/com/akiban/sql/pg/yaml/bugs/test-bug-1199111.yaml	2013-07-29 15:28:42 +0000
@@ -0,0 +1,174 @@
+# Test 1: broken alter followed by a good alter
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NULL;
+---
+- Statement: SELECT * FROM t1;
+- output: [[1, null]]
+---
+- DropTable: t2
+---
+- DropTable: t1
+# Test 2: broken alter t2 followed by a good alter t1
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NULL;
+---
+- Statement: SELECT * FROM t1;
+- output: [[1, null]]
+---
+- DropTable: t2
+---
+- DropTable: t1
+# Test 3: broken alter t1 followed by a good alter t2
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NULL;
+---
+- Statement: SELECT * FROM t2;
+- output: [[1, 1, null]]
+---
+- DropTable: t2
+---
+- DropTable: t1
+# Test 4: broken alter t1 then t2, followed by a good alter on t1
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NULL;
+---
+- Statement: SELECT * FROM t1;
+- output: [[1, null]]
+---
+- DropTable: t2
+---
+- DropTable: t1
+# Test 5: broken alter t2 then t1, followed by a good alter on t1
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NULL;
+---
+- Statement: SELECT * FROM t1;
+- output: [[1, null]]
+---
+- DropTable: t2
+---
+- DropTable: t1
+# Test 6: broken alter t1 then t2, followed by a good alter on t1 and t2
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NOT NULL;
+- error: ['23502']
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NULL;
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NULL;
+---
+- Statement: SELECT * FROM t1;
+- output: [[1, null]]
+---
+- DropTable: t2
+---
+- DropTable: t1
+
+# Test 7: Group Index on t1/t2 , 
+---
+- CreateTable: t1 (id INT NOT NULL PRIMARY KEY);
+---
+- CreateTable: t2(id INT NOT NULL PRIMARY KEY, t1id INT NOT NULL,
+    GROUPING FOREIGN KEY (t1id) references t1 (id));
+---
+- Statement: INSERT INTO t1 VALUES (1);
+---
+- Statement: insert into t2 values (1, 1);
+---
+- Statement: ALTER TABLE t1 ADD COLUMN x INT NULL
+---
+- Statement: ALTER TABLE t2 ADD COLUMN x INT NULL
+---
+- Statement: CREATE INDEX t12 on t1 (t1.x, t2.x) USING LEFT JOIN;
+---
+- Statement: ALTER TABLE t1 ALTER COLUMN x NOT NULL
+- error: ['23502']
+---
+- Statement: UPDATE t1 set x = 1;
+---
+- Statement: ALTER TABLE t1 ALTER COLUMN x NOT NULL
+---
+- Statement: SELECT * FROM t1;
+- output: [[1, 1]]
+---
+- DropTable: t2
+---
+- DropTable: t1