diff --git a/db/osqlsqlthr.c b/db/osqlsqlthr.c index 0eda3a8475..0fffc2c077 100644 --- a/db/osqlsqlthr.c +++ b/db/osqlsqlthr.c @@ -32,6 +32,7 @@ */ #include +#include #include #include #include "sql.h" @@ -170,11 +171,12 @@ static inline int osql_should_restart(struct sqlclntstate *clnt, int rc, } \ } \ if (rc) { \ - logmsg(LOGMSG_ERROR, \ - "%s: error writting record to master in offload mode " \ - "rc=%d!\n", \ - __func__, rc); \ - if (rc != SQLITE_TOOBIG && rc != ERR_SC) \ + if (rc != SQLITE_DDL_MISUSE) \ + logmsg(LOGMSG_ERROR, \ + "%s: error writting record to master in offload mode " \ + "rc=%d!\n", \ + __func__, rc); \ + if (rc != SQLITE_TOOBIG && rc != ERR_SC && rc != SQLITE_DDL_MISUSE) \ rc = SQLITE_INTERNAL; \ } else { \ rc = SQLITE_OK; \ @@ -1428,6 +1430,33 @@ int osql_sock_abort(struct sqlclntstate *clnt, int type) * ***********************************************/ int gbl_reject_mixed_ddl_dml = 1; +static int check_for_overlap(struct sqlclntstate *clnt, struct schema_change_type *sc) +{ + if (!clnt->ddl_tables) + return SQLITE_OK; + + if (hash_find_readonly(clnt->ddl_tables, sc->tablename)) + return SQLITE_DDL_MISUSE; + + /* sqlite can't hold a tablename and an aliasname that collide, so filter + * out introduced-name conflicts here before sending to master for SC_ALIASTABLE as well. */ + const char *newname = NULL; + if (sc->kind == SC_ADDTABLE) { + newname = sc->tablename; + } else if ((sc->kind == SC_RENAMETABLE || sc->kind == SC_ALIASTABLE) && + strcasecmp(sc->tablename, sc->newtable) != 0) { + newname = sc->newtable; + } + if (newname && clnt->ddl_new_tables && hash_find_readonly(clnt->ddl_new_tables, newname)) + return SQLITE_DDL_MISUSE; + + hash_add(clnt->ddl_tables, strdup(sc->tablename)); + if (newname && clnt->ddl_new_tables) + hash_add(clnt->ddl_new_tables, strdup(newname)); + + return SQLITE_OK; +} + static int osql_send_usedb_logic_int(char *tablename, struct sqlclntstate *clnt, int nettype) { @@ -1897,10 +1926,8 @@ int osql_schemachange_logic(struct schema_change_type *sc, int usedb) } } - if (clnt->dml_tables && - hash_find_readonly(clnt->dml_tables, sc->tablename)) { - return SQLITE_DDL_MISUSE; - } + if ((rc = check_for_overlap(clnt, sc)) != SQLITE_OK) + return rc; if (clnt->remsql_set.is_remsql == IS_REMCREATE) { /* this is a distributed create for a partition, creating individual shard here, info passed from @@ -1930,13 +1957,6 @@ int osql_schemachange_logic(struct schema_change_type *sc, int usedb) } } - if (clnt->ddl_tables) { - if (hash_find_readonly(clnt->ddl_tables, sc->tablename)) { - return SQLITE_DDL_MISUSE; - } else - hash_add(clnt->ddl_tables, strdup(sc->tablename)); - } - sc->usedbtablevers = comdb2_table_version(sc->tablename); if (thd->clnt->dbtran.mode == TRANLEVEL_SOSQL) { diff --git a/db/sql.h b/db/sql.h index 1b84d2b471..5cf095e688 100644 --- a/db/sql.h +++ b/db/sql.h @@ -949,6 +949,11 @@ struct sqlclntstate { hash_t *ddl_tables; hash_t *dml_tables; hash_t *ddl_contexts; + /* Names introduced as brand-new tables in this transaction (CREATE target + * and RENAME target). Used to reject two statements introducing the same + * name (e.g. "rename a to c; create c"), which otherwise deadlocks the + * abort handler at finalize. */ + hash_t *ddl_new_tables; int statement_query_effects; diff --git a/db/sqlinterfaces.c b/db/sqlinterfaces.c index 352ffb6ff4..cee0d3732a 100644 --- a/db/sqlinterfaces.c +++ b/db/sqlinterfaces.c @@ -1610,10 +1610,11 @@ static void sql_update_usertran_state(struct sqlclntstate *clnt) clnt->in_client_trans = 1; - assert(clnt->ddl_tables == NULL && clnt->dml_tables == NULL && - clnt->ddl_contexts == NULL); + assert(clnt->ddl_tables == NULL && clnt->dml_tables == NULL && clnt->ddl_contexts == NULL && + clnt->ddl_new_tables == NULL); clnt->ddl_tables = hash_init_strcase(0); clnt->dml_tables = hash_init_strcase(0); + clnt->ddl_new_tables = hash_init_strcase(0); clnt->ddl_contexts = hash_init_strcaseptr(offsetof(struct clnt_ddl_context, name)); } } else if (meta == TSMC_COMMIT) { @@ -2438,8 +2439,10 @@ int handle_sql_commitrollback(struct sqlthdstate *thd, destroy_hash(clnt->ddl_tables, free_it); destroy_hash(clnt->dml_tables, free_it); + destroy_hash(clnt->ddl_new_tables, free_it); clnt->ddl_tables = NULL; clnt->dml_tables = NULL; + clnt->ddl_new_tables = NULL; destroy_hash(clnt->ddl_contexts, free_clnt_ddl_context); clnt->ddl_contexts = NULL; @@ -5414,8 +5417,10 @@ void cleanup_clnt(struct sqlclntstate *clnt) destroy_hash(clnt->ddl_tables, free_it); destroy_hash(clnt->dml_tables, free_it); + destroy_hash(clnt->ddl_new_tables, free_it); clnt->ddl_tables = NULL; clnt->dml_tables = NULL; + clnt->ddl_new_tables = NULL; destroy_hash(clnt->ddl_contexts, free_clnt_ddl_context); clnt->ddl_contexts = NULL; diff --git a/tests/renametable.test/reqoutput.txt b/tests/renametable.test/reqoutput.txt index 849d83acda..2f45511ad7 100644 --- a/tests/renametable.test/reqoutput.txt +++ b/tests/renametable.test/reqoutput.txt @@ -113,6 +113,8 @@ (name='$1_5DD5C692') (name='$2_E5D0B3CA') (name='talias2') +[alter table ra rename to ra2] rc 0 +[alter table ra2 rename to ra] rc 0 [begin] rc 0 [drop table t] rc 0 [alter table t2 rename to t] rc 0 @@ -127,3 +129,19 @@ [alter table t rename to dummy] rc 0 [alter table t2 rename to t] rc 0 [commit] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[create table tr2 {schema{int i}}] rc 0 +[alter table tr rename to tr2] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[alter table tr rename to tr2] rc 0 +[create table tr2(i int)] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[alter table tr rename to tr2] rc 0 +[truncate tr] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[truncate tr] rc 0 +[alter table tr rename to tr2] rc 0 diff --git a/tests/renametable.test/reqoutput_lightweight.txt b/tests/renametable.test/reqoutput_lightweight.txt index dc79569dd3..dd40829e9d 100644 --- a/tests/renametable.test/reqoutput_lightweight.txt +++ b/tests/renametable.test/reqoutput_lightweight.txt @@ -113,6 +113,8 @@ (name='$1_5DD5C692') (name='$2_E5D0B3CA') (name='talias2') +[alter table ra rename to ra2] rc 0 +[alter table ra2 rename to ra] rc 0 [commit] failed with rc -3 New table name already exists [begin] rc 0 [drop table t] rc 0 @@ -127,3 +129,19 @@ [begin] rc 0 [alter table t rename to dummy] rc 0 [alter table t2 rename to t] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[create table tr2 {schema{int i}}] rc 0 +[alter table tr rename to tr2] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[alter table tr rename to tr2] rc 0 +[create table tr2(i int)] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[alter table tr rename to tr2] rc 0 +[truncate tr] rc 0 +[commit] failed with rc -5 Transactional DDL Error: Overlapping Tables +[begin] rc 0 +[truncate tr] rc 0 +[alter table tr rename to tr2] rc 0 diff --git a/tests/renametable.test/runit b/tests/renametable.test/runit index 2cbd622c49..d7aaee3dd4 100755 --- a/tests/renametable.test/runit +++ b/tests/renametable.test/runit @@ -107,6 +107,17 @@ if (( $? != 0 )) ; then exit 1 fi +# Tests "removing an alias" exception in sqlite3AlterRenameTable +# (comdb2build.c): get_dbtable_by_name(target) finds the same table via its +# physical name, which must NOT be flagged as a collision. +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists ra" +cdb2sql ${CDB2_OPTIONS} $dbnm default "create table ra(i int)" +cat <> $OUT 2>&1 +alter table ra rename to ra2\$\$ +alter table ra2 rename to ra\$\$ +EOF +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists ra" + # Rename after dropping table with target name in txn cdb2sql ${CDB2_OPTIONS} $dbnm default "create table t2(i int)" cat <> $OUT 2>&1 @@ -141,6 +152,53 @@ alter table t2 rename to t\$\$ commit EOF +# TEST TRAN: CREATE TR2; RENAME TR->TR2 (expected: fail, must not deadlock) +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr" +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr2" +cdb2sql ${CDB2_OPTIONS} $dbnm default "create table tr {schema{int i}}" +cat <> $OUT 2>&1 +begin +create table tr2 {schema{int i}}\$\$ +alter table tr rename to tr2\$\$ +commit +EOF + +# TEST TRAN: RENAME TR->TR2; CREATE TR2 (expected: fail, must not deadlock) +# should fail, check comment in osqlsqlthr.c:check_for_overlap +# this creates a sqlaliasname tr2 for tr, but when we create tr2, sqlite will err +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr" +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr2" +cdb2sql ${CDB2_OPTIONS} $dbnm default "create table tr {schema{int i}}" +cat <> $OUT 2>&1 +begin +alter table tr rename to tr2\$\$ +create table tr2(i int)\$\$ +commit +EOF + +# TEST TRAN: RENAME TR->TR2; TRUNCATE TR (DDL on introduced name) (expected: fail, must not deadlock) +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr" +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr2" +cdb2sql ${CDB2_OPTIONS} $dbnm default "create table tr {schema{int i}}" +cat <> $OUT 2>&1 +begin +alter table tr rename to tr2\$\$ +truncate tr +commit +EOF + +# TEST TRAN: TRUNCATE TR; RENAME TR->TR2 (DDL on introduced name) (expected: fail, must not deadlock) +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr" +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tr2" +cdb2sql ${CDB2_OPTIONS} $dbnm default "create table tr {schema{int i}}" +cat <> $OUT 2>&1 +begin +truncate tr +alter table tr rename to tr2\$\$ +commit +EOF + + df=$(diff $OUT $EXP) if [ $? -ne 0 ] ; then echo " ^^^^^^^^^^^^" diff --git a/tests/sc_transactional.test/runit b/tests/sc_transactional.test/runit index eaa0b83823..dc829935c8 100755 --- a/tests/sc_transactional.test/runit +++ b/tests/sc_transactional.test/runit @@ -3,7 +3,7 @@ bash -n "$0" | exit 1 source ${TESTSROOTDIR}/tools/runit_common.sh #PS4='$(date +%Y%m%d-%H:%M:%S): ' -set -x + dbnm=$1 @@ -317,4 +317,34 @@ do_verify t1 do_verify t2 do_verify t3 +# skip this test if rowlocks, cause this will set the isolation level to SNAPISOL and +# so this will skip SOSQL DDL/DML conflict detection step +if [[ x"$dbnm" != x*"rowlocks"* ]]; then + +echo "TEST TRAN: TRUNCATE T; INSERT T (same table, DDL then DML) (expected: fail, must not deadlock)" +cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table if exists tdml" +cdb2sql ${CDB2_OPTIONS} $dbnm default "create table tdml(i int)" +cdb2sql ${CDB2_OPTIONS} $dbnm default "insert into tdml values(1)" +assertcnt tdml 1 + +# ddl-dml overlap +out=$(timeout --kill-after=5s 60s cdb2sql ${CDB2_OPTIONS} $dbnm default - <<"EOF" 2>&1 +begin +truncate tdml +insert into tdml values(2) +commit +EOF +) +echo "$out" +if ! echo "$out" | grep -q "Transactional DDL Error"; then + failexit "TRUNCATE T; INSERT T did not report a transactional DDL error: $out" +fi +# table must survive with original row intact +assertcnt tdml 1 +do_verify tdml + +else + echo "TEST TRAN: TRUNCATE T; INSERT T skipped (rowlocks/SNAPISOL mode bypasses SOSQL DDL/DML conflict detection)" +fi + echo "Success"