[#933] Read a catalog table another session created while this one was crea...

Valery Kharseko

11 hours ago 8ad97160e90f47e5fffe0b8a8c65ee421fa0903c

[#933] Read a catalog table another session created while this one was creating it (#1005)

 opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

@@ -4724,8 +4724,9 @@
         * <p>
         * Serialized on the storage, so that two transactions opening trees at the same time cannot both
         * find the table absent and both go on to create it. It serializes this storage and nothing
         * else, which is why the create tolerates a table that turned up while it was being made: an
         * offline tool beside a running server is a pair no lock of one process can order. The stamp -
         * else, which is why the create tolerates a table that turned up while it was being made - an
         * offline tool beside a running server is a pair no lock of one process can order - and why what
         * it tolerated is then read like any other catalog that was already there. The stamp -
         * the one thing under it that is nobody's dependency - is issued outside it.
         * <p>
         * Two things are kept out of the lock because they are the slow ones. The flag is read before
@@ -4760,11 +4761,24 @@
                if (!lostTheRace) {
                    if (isExistsTable(catalog)) {
                        readEnrolledTrees(catalog);
                    } else {
                        createCatalogTable(catalog);
                        // nothing to read from a table that has just been created, and nothing this open
                        // enrols may be skipped as already recorded
                    } else if (!createCatalogTable(catalog)) {
                        // the table was there after all: another session created it while this one was
                        // creating it, and a table this session did not create is a catalog with rows in it
                        // like any other - the branch above is what those rows are for. The flag below is
                        // raised over what this storage knows the catalog records, and raised over none of
                        // it, every tree of this open is enrolled again against a catalog already naming
                        // them: one upsert and one commit each, and the same for every later write of this
                        // open that names a tree (#933). It reads on the session the failed create reset,
                        // which is a connection rolled back or, where even that failed, one given up for
                        // the read to establish again: either is a session a select may be asked of. That
                        // re-establish is the one connect of this class made under the lock: it needs a
                        // create cut by a dead catalog connection, a table another session made meanwhile,
                        // and a database refusing logins, and it is bounded by the deadline of a borrow
                        // like the connect made outside it
                        readEnrolledTrees(catalog);
                    }
                    // and where the create really did create it, there is nothing to read: a table just
                    // made holds no row, and nothing this open enrols may be skipped as already recorded
                    catalogTableOpened=true;
                }
            }
@@ -4856,8 +4870,12 @@
         * line. What is left is the privilege the account is missing. An engine this backend does not
         * know has no number a lock could be told by ({@code isLockTimeout()} answers false on a null
         * dialect), and gets the privilege line there as it did.
         *
         * @return whether this session created the table. {@code false} says another session created it
         *         while this one was creating it, which is a table full of rows this storage has not
         *         read: what {@link #openCatalog} does with that answer is read them (#933).
         */
        void createCatalogTable(TreeName catalog) {
        boolean createCatalogTable(TreeName catalog) {
            final String tableName=getTableName(catalog);
            Dialect dialect=null; // read inside the try, and asked again by the catch, which tells a lock by the engine's own number
            try {
@@ -4878,6 +4896,7 @@
                    catalogCon.commit();
                    return null;
                });
                return true;
            } catch (SQLException | RuntimeException e) {
                // the unchecked one as well, for the reason enrolInCatalog() takes it: what the statement
                // left behind has to be rolled back whatever class the failure arrived in, this connection
@@ -4901,7 +4920,7 @@
                if (alreadyThere) {
                    logger.debug(LocalizableMessage.raw("jdbc: table %s was created by another session while this one was creating it: %s",
                        tableName, stackTraceToSingleLineString(e)));
                    return;
                    return false; // read by the caller, the rows being another session's and not this one's
                }
                // A lock another session holds is not a privilege the account lacks, whichever bound ended
                // the wait for it: asked of the engine's own number on every road, since gaveUpOnTheLock()

 opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java

@@ -37,6 +37,10 @@
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
@@ -2200,6 +2204,181 @@
    }

    /**
     * A catalog table another session created while this one was creating it has to be read like any
     * other catalog that is already there. The create tolerates that race - an offline tool beside a
     * running server is a pair no lock of one process can order - and what it must not do is leave the
     * storage believing that table records nothing (#933): a storage believing that writes the row of
     * every tree it opens again, one upsert and one commit each, for the whole of that open.
     * <p>
     * The tree the case asks for afterwards is one the adopting write never opened, which is what
     * tells the two apart: a storage that read the table knows the catalog names it already, while one
     * that only wrote its way through the trees of that write knows those and nothing else.
     * <p>
     * What says which of the two it is, is the connection. The catalog of a transaction is opened at
     * the first row that transaction has to write - see {@link JDBCStorage.CatalogSession} - so a
     * write whose trees the catalog already names opens none at all, and one that has to record them
     * again opens one to write the rows that are already there.
     */
    @Test
    public void testAnOpenThatAdoptsTheCatalogTableOfAnotherSessionEnrolsNothingAgain() throws Exception {
        final TreeName opened = new TreeName("testAdoptedCatalog", "opened");
        final TreeName recorded = new TreeName("testAdoptedCatalog", "recorded");
        final JDBCStorage owner = new JDBCStorage(createBackendCfg(getBackendId() + "_adopted"), null);
        // the two are one backend, so the clear below drops what either of them left behind, and it is
        // constructed here so that a failure of the half that fills the catalog reaches that clear too:
        // nothing but @BeforeClass ever drops the tables a case leaves standing
        final AdoptingStorage racing = new AdoptingStorage(createBackendCfg(getBackendId() + "_adopted"));
        try {
            try {
                owner.open(AccessMode.READ_WRITE);
                owner.write(new WriteOperation() {
                    @Override
                    public void run(WriteableTransaction txn) throws Exception {
                        txn.openTree(opened, true); // the catalog table, and a row naming each of these trees
                        txn.openTree(recorded, true);
                    }
                });
            } finally {
                owner.close();
            }

            racing.open(AccessMode.READ_WRITE);
            // armed after the open and not before it: what this models is the lookup openCatalog() makes,
            // and anything asking about the same table earlier would spend the one answer it has
            racing.hideOnce(racing.getTableName(racing.getCatalogTree()));
            racing.write(new WriteOperation() {
                @Override
                public void run(WriteableTransaction txn) throws Exception {
                    txn.openTree(opened, true);
                }
            });
            assertTrue(racing.hidTheCatalogTable(), "the case never reached the create this is about");
            final int connectsOfTheAdoptingWrite = racing.catalogConnects();

            racing.write(new WriteOperation() {
                @Override
                public void run(WriteableTransaction txn) throws Exception {
                    txn.openTree(recorded, true); // named by the catalog already, so there is no row to write
                }
            });
            assertEquals(racing.catalogConnects(), connectsOfTheAdoptingWrite,
                "a transaction whose tree the catalog already names opened a connection to write that row"
                    + " again: the open which adopted the table another session created read nothing of what"
                    + " that table records");
            assertTrue(racing.listTrees().contains(recorded), "the read of the adopted rows rewrote or removed them");
        } finally {
            clearQuietly(racing);
        }
    }

    /**
     * The road above pins the "adopted" answer of {@code createCatalogTable()}; this one pins the
     * "created" answer against the same mistake: an open that made the table itself has nothing of
     * its own to read back, the table it just made holding no row, and reading it anyway would be
     * one select and one commit spent on an empty catalog for every such open.
     */
    @Test
    public void testAnOpenThatCreatesTheCatalogTableReadsNothing() throws Exception {
        final AdoptingStorage fresh = new AdoptingStorage(createBackendCfg(getBackendId() + "_created"));
        try {
            fresh.open(AccessMode.READ_WRITE);
            fresh.write(new WriteOperation() {
                @Override
                public void run(WriteableTransaction txn) throws Exception {
                    txn.openTree(new TreeName("testCreatedCatalog", "opened"), true);
                }
            });
            assertEquals(fresh.catalogReads(), 0, "the open that created the catalog table read it back");
        } finally {
            clearQuietly(fresh);
        }
    }

    /**
     * The adopt road again, on the arm where the failed create could not even be rolled back: the
     * session is given up rather than left holding a connection the database has dropped, and the
     * read of the rows that table already holds has to establish it again - the one connect this
     * class makes under {@code catalogLock}, which the invariant of {@code openCatalog()} names as
     * its exception.
     * <p>
     * A rollback refused once is the second state no case can reach from outside, beside the hidden
     * lookup it is armed with: a create that fails because its own connection is gone fails its
     * rollback with it. What says the read was made on a session established for it is the count of
     * connections the adopting write opened - the one the create ran on, and the one the read had to
     * make - and what says that read filled the memo all the same is the write after it, whose tree
     * the catalog already names and which therefore opens none.
     * <p>
     * The attempt is counted beside them, because {@code write()} would otherwise answer for this
     * road itself: a storage that gave the session up without letting go of it reads on a closed
     * connection, which is classified as a dropped connection and replayed, and the replay makes the
     * very table it failed to make usable. The counts above are then the counts of an attempt that
     * never met the race, so the case asks for the one attempt as well.
     */
    @Test
    public void testAnOpenThatAdoptsTheCatalogTableReadsItOnASessionItHadToEstablishAgain() throws Exception {
        final TreeName opened = new TreeName("testRefusedRollbackCatalog", "opened");
        final TreeName recorded = new TreeName("testRefusedRollbackCatalog", "recorded");
        final JDBCStorage owner = new JDBCStorage(createBackendCfg(getBackendId() + "_refused"), null);
        // one backend in two storages, as in the case above: the clear at the end drops what either of
        // them left behind, and constructing it here reaches that clear whichever half failed
        final AdoptingStorage racing = new AdoptingStorage(createBackendCfg(getBackendId() + "_refused"));
        try {
            try {
                owner.open(AccessMode.READ_WRITE);
                owner.write(new WriteOperation() {
                    @Override
                    public void run(WriteableTransaction txn) throws Exception {
                        txn.openTree(opened, true); // the catalog table, and a row naming each of these trees
                        txn.openTree(recorded, true);
                    }
                });
            } finally {
                owner.close();
            }

            racing.open(AccessMode.READ_WRITE);
            // armed after the open, for the reason the case above arms its lookup there: the catalog of a
            // transaction is established at the first row it has to write, which is inside the write below
            racing.hideOnce(racing.getTableName(racing.getCatalogTree()));
            racing.refuseNextRollback();
            final int connectsBeforeTheAdoptingWrite = racing.catalogConnects();
            final AtomicInteger attemptsOfTheAdoptingWrite = new AtomicInteger();
            racing.write(new WriteOperation() {
                @Override
                public void run(WriteableTransaction txn) throws Exception {
                    attemptsOfTheAdoptingWrite.incrementAndGet();
                    txn.openTree(opened, true);
                }
            });
            assertTrue(racing.hidTheCatalogTable(), "the case never reached the create this is about");
            // a session the storage gave up and did not let go of is a select on a closed connection, which
            // write() classifies as a dropped connection and replays: the road recovers, and the counts
            // below are the counts of the attempt that replaced it. This is what says the read was made
            // inside the attempt that met the race, at the cost of the one connect and no replay at all
            assertEquals(attemptsOfTheAdoptingWrite.get(), 1,
                "the write which adopted the table was replayed: the read of the adopted rows has to be"
                    + " made on a session established inside that attempt, not by spending another one");
            assertEquals(racing.catalogConnects(), connectsBeforeTheAdoptingWrite + 2,
                "the session the refused rollback gave up was not established again for the read of the"
                    + " adopted rows: this road costs one connection for the create and one for that read");

            racing.write(new WriteOperation() {
                @Override
                public void run(WriteableTransaction txn) throws Exception {
                    txn.openTree(recorded, true); // named by the catalog already, so there is no row to write
                }
            });
            assertEquals(racing.catalogConnects(), connectsBeforeTheAdoptingWrite + 2,
                "a transaction whose tree the catalog already names opened a connection to write that row"
                    + " again: the read made on the re-established session recorded nothing of what the"
                    + " adopted table holds");
            assertTrue(racing.listTrees().contains(recorded), "the read of the adopted rows rewrote or removed them");
        } finally {
            clearQuietly(racing);
        }
    }

    /**
     * A row of the catalog whose table is not there any more must not fail the clear, and must not
     * stop it dropping the rest. Nothing of the backend leaves such a row behind - deleteTree() takes
     * it out in the commit that drops the table - but a table dropped by hand, or a catalog restored
@@ -3012,4 +3191,120 @@
            fail(whatWentUnsaid + "; the clear reported: " + reported());
        }
    }

    /**
     * A storage which answers once that its catalog table is not there while it is, and counts the
     * connections its catalog is read and written on.
     * <p>
     * That answer models the one state no case can reach from outside: a session whose lookup ran a
     * moment before another session's "create table" committed, which goes on to create a table that
     * is already there and has to make what it finds usable all the same (#933). Every lookup after
     * it is the database's own answer, the way {@link ReportingStorage} lets its own go.
     * <p>
     * The count is what says whether the storage remembered what that table records: the catalog of a
     * transaction is opened at the first row the transaction has to write, so a write whose trees the
     * catalog already names opens no connection at all - see {@link JDBCStorage.CatalogSession}.
     * <p>
     * It refuses one rollback where a case asks for it, which is the other state no case reaches from
     * outside: the create of a session the database has dropped fails its rollback with it, and the
     * read of the adopted rows is then made on a session established again for it.
     */
    protected static final class AdoptingStorage extends JDBCStorage {
        private volatile String tableToHide;
        private volatile boolean hidden;
        private volatile boolean refuseNextRollback;
        private final AtomicInteger catalogConnects = new AtomicInteger();
        private final AtomicInteger catalogReads = new AtomicInteger();

        AdoptingStorage(JDBCBackendCfg cfg) {
            super(cfg, null);
        }

        /** Answers the next lookup of this table with "not there", whatever the database holds. */
        void hideOnce(String tableName) {
            tableToHide = tableName;
        }

        /** Whether that answer was given, so that a case cannot pass without having reached the race. */
        boolean hidTheCatalogTable() {
            return hidden;
        }

        /**
         * Hands the next catalog connection out with its first rollback refused: the state of a
         * session the database has dropped, whose create fails and whose rollback of that create fails
         * with it, which is what leaves {@code reset()} giving the session up instead of rolling it
         * back.
         */
        void refuseNextRollback() {
            refuseNextRollback = true;
        }

        /** How many connections this storage has opened its catalog on since it was constructed. */
        int catalogConnects() {
            return catalogConnects.get();
        }

        /** How many times this storage has read the catalog into its memo since it was constructed. */
        int catalogReads() {
            return catalogReads.get();
        }

        @Override
        boolean isExistsTable(Connection con, JDBCStorage.TableScope scope, String tableName) {
            final String hiding = tableToHide;
            if (hiding != null && hiding.equalsIgnoreCase(tableName)) {
                tableToHide = null; // once: the create it sends is answered by the table the database holds
                hidden = true;
                return false;
            }
            return super.isExistsTable(con, scope, tableName);
        }

        @Override
        Connection newCatalogConnection(long budgetDeadline) throws SQLException {
            catalogConnects.incrementAndGet();
            final Connection real = super.newCatalogConnection(budgetDeadline);
            if (!refuseNextRollback) {
                return real;
            }
            refuseNextRollback = false;
            return refusingItsFirstRollback(real);
        }

        /**
         * The connection above with one rollback of its own refused, and everything else the driver's.
         * The rollback of a whole transaction alone: a rollback to a savepoint is what the bound of a
         * DDL takes back where its setting failed ({@code withDdlLockBound}), and refusing that one
         * would arm a road other than the one this case is about.
         */
        private static Connection refusingItsFirstRollback(final Connection real) {
            return (Connection) Proxy.newProxyInstance(Connection.class.getClassLoader(),
                new Class<?>[] { Connection.class }, new InvocationHandler() {
                    private boolean refused;

                    @Override
                    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
                        if (!refused && "rollback".equals(method.getName()) && method.getParameterCount() == 0) {
                            refused = true;
                            throw new SQLException("the case refused the rollback of this catalog connection");
                        }
                        try {
                            return method.invoke(real, args);
                        } catch (InvocationTargetException e) {
                            throw e.getCause(); // the driver's own failure, and not a wrapper of the call
                        }
                    }
                });
        }

        // the only caller of the two-argument overload is readEnrolledTrees(): catalogTables() (a
        // clear, or listTrees()) takes the three-argument one, so this counts a read of the memo and
        // nothing a clear does
        @Override
        Map<TreeName,String> readCatalogRows(Connection con, String catalogTable) throws SQLException {
            catalogReads.incrementAndGet();
            return super.readCatalogRows(con, catalogTable);
        }
    }
}