Preserve table_version entry on rename to avoid reuse

bdb/bdb_api.h

@@ -2289,12 +2289,6 @@ int bdb_del_table_sqlalias_tran(const char *tablename, tran_type *tran);
 int bdb_table_version_update(bdb_state_type *bdb_state, tran_type *tran,
                              unsigned long long val, int *bdberr);
 
-/**
- *  Delete the TABLE VERSION ENTRY for table "bdb_state->name"
- *
- */
-int bdb_table_version_delete(bdb_state_type *bdb_state, tran_type *tran,
-                             int *bdberr);
 /**
  *  Select the TABLE VERSION ENTRY for table "bdb_state->name".
  *  If an entry doesn't exist, version 0 is returned

bdb/llmeta.c

@@ -8898,86 +8898,6 @@ int bdb_table_version_update(bdb_state_type *bdb_state, tran_type *tran,
     return bdb_table_version_upsert_int(bdb_state, tran, &val, bdberr);
 }
 
-/**
- *  Delete the TABLE VERSION ENTRY for table "bdb_state->name"
- *
- */
-int bdb_table_version_delete(bdb_state_type *bdb_state, tran_type *tran,
-                             int *bdberr)
-{
-    struct llmeta_sane_table_version schema_version;
-    char *tblname = bdb_state->name;
-    unsigned long long version;
-    char key[LLMETA_IXLEN] = {0};
-    uint8_t *p_buf, *p_buf_end;
-    int len;
-    int rc;
-
-    if (unlikely(!tran || !tblname || !bdberr)) {
-        logmsg(LOGMSG_ERROR, "%s: NULL argument\n", __func__);
-        if (bdberr)
-            *bdberr = BDBERR_BADARGS;
-        return -1;
-    }
-
-    /*fail if the db isn't open*/
-    if (unlikely(!llmeta_bdb_state)) {
-        logmsg(LOGMSG_ERROR, "%s: low level meta table not yet open,"
-                        "you must run bdb_llmeta_open\n",
-                __func__);
-        *bdberr = BDBERR_MISC;
-        return -1;
-    }
-    if (unlikely(bdb_get_type(llmeta_bdb_state) != BDBTYPE_LITE)) {
-        logmsg(LOGMSG_ERROR, "%s: llmeta db not lite\n", __func__);
-        *bdberr = BDBERR_BADARGS;
-        return -1;
-    }
-
-    /* input validation */
-    len = strlen(bdb_state->name) + 1;
-    if (unlikely(len > sizeof(schema_version.tblname))) {
-        logmsg(LOGMSG_ERROR, "%s: tablename too long %zu\n", __func__,
-               strlen(bdb_state->name));
-        *bdberr = BDBERR_BADARGS;
-        return -1;
-    }
-
-    /* find the existing record, if any */
-    rc = bdb_table_version_select(bdb_state->name, tran, &version, bdberr);
-    if (rc) {
-        *bdberr = BDBERR_MISC;
-        return -1;
-    }
-
-    /* add the key type */
-    bzero(&schema_version, sizeof(schema_version));
-    schema_version.file_type = LLMETA_TABLE_VERSION;
-    strncpy0(schema_version.tblname, bdb_state->name,
-             sizeof(schema_version.tblname));
-
-    p_buf = (uint8_t *)key;
-    p_buf_end = p_buf + LLMETA_IXLEN;
-
-    /* put onto buffer */
-    if (!(p_buf = llmeta_sane_table_version_put(&schema_version, p_buf,
-                                                p_buf_end))) {
-        logmsg(LOGMSG_ERROR, "%s: llmeta_sane_table_version_put returns NULL\n",
-                __func__);
-        *bdberr = BDBERR_MISC;
-        return -1;
-    }
-
-    /* delete old entry */
-    rc = bdb_lite_exact_del(llmeta_bdb_state, tran, key, bdberr);
-    if ((rc || *bdberr != BDBERR_NOERROR) && *bdberr != BDBERR_DEL_DTA) {
-        return rc;
-    }
-
-    *bdberr = BDBERR_NOERROR;
-    return 0;
-}
-
 /**
  *  Select the TABLE VERSION ENTRY for table "bdb_state->name".
  *  If an entry doesn't exist, version 0 is returned
@@ -11193,10 +11113,10 @@ int bdb_rename_table_metadata(bdb_state_type *bdb_state, tran_type *tran,
     if (rc)
         return rc;
 
-    /* delete old name's table_version entry */
-    rc = bdb_table_version_delete(bdb_state, tran, bdberr);
-    if (rc)
-        return rc;
+    /* Note: the old name's table_version entry is intentionally left in
+     * place as a tombstone (mirroring drop table), so that if a table is
+     * later created with the vacated old name, its version does not reset
+     * to 0. */
 
     /* rename files finally, with new versions */
     rc = bdb_rename_files(bdb_state, tran, newname, bdberr);

schemachange/sc_rename_table.c

@@ -167,11 +167,28 @@ int finalize_rename_table(struct ireq *iq, struct schema_change_type *s,
         goto recover_memory;
     }
 
-    /* set table version for the renamed name */
-    rc = table_version_set(tran, newname, db->tableversion + 1);
+    /* Set table version for the renamed name.  The destination name may
+     * carry a tombstone left behind by a previous incarnation that was
+     * dropped or renamed away; that value is already one past anything the
+     * previous incarnation reported, so never go below it -- otherwise we
+     * re-issue version numbers this name has already handed out.  A name
+     * that was never used selects as 0.  finalize_add_table() adopts the
+     * same tombstone when a table is created under a vacated name. */
+    unsigned long long tombstone = 0;
+    rc = bdb_table_version_select(newname, tran, &tombstone, &bdberr);
+    if (rc) {
+        sc_errf(s, "Failed fetching table version for %s bdberr %d\n", newname, bdberr);
+        goto recover_memory;
+    }
+
+    unsigned long long newversion = db->tableversion + 1;
+    if (newversion < tombstone)
+        newversion = tombstone;
+
+    rc = table_version_set(tran, newname, newversion);
     if (rc) {
         sc_errf(s, "Failed to set table version for %s\n", db->tablename);
-        goto tran_error;
+        goto recover_memory;
     }
 
     gbl_sc_commit_count++;
@@ -184,8 +201,12 @@ int finalize_rename_table(struct ireq *iq, struct schema_change_type *s,
     return rc;
 
 recover_memory:
-    /* backout memory changes */
-    rename_db(db, oldname);
+    /* backout memory changes; rename_db took ownership of newname, so put
+     * oldname back before freeing it.  If the backout fails, db->tablename
+     * still references newname -- leak it rather than leave a dangling
+     * pointer behind. */
+    if (rename_db(db, oldname) == 0)
+        free(newname);
     return rc;
 
 tran_error:

tests/rename_populate.test/runit

@@ -1,7 +1,10 @@
 #!/usr/bin/env bash
 # Verify table rename correctness:
-# 1) bdb_rename_table_metadata must delete orphaned table_version entries so
-#    a fresh table created with the old name starts at version 0.
+# 1) bdb_rename_table_metadata must preserve (not delete) the old name's
+#    table_version entry as a tombstone, so that a table later created (or
+#    renamed into) that vacated name never reuses a version number that was
+#    already handed out to a previous incarnation of that name -- in
+#    particular it must never reset back to 0.
 # 2) reload_rename_table (replicant scdone callback) must refresh file versions
 #    so dtavers[]/ixvers[] reflect the renamed files.
 
@@ -60,18 +63,21 @@ echo "Step 5: Rename t2 back to t1"
 cdb2sql ${CDB2_OPTIONS} $dbnm default "alter table t2 rename to t1" || failexit "rename t2→t1"
 assertcnt t1 5 "t1 should have 5 rows after rename-back"
 
-# Step 6: Create a FRESH t2 — this is where the bug manifests.
-# The old table_version entry for "t2" (orphaned by step 5's rename)
-# is still in LLMETA. The new table picks it up instead of starting at 0.
+# Step 6: Create a FRESH t2. The name "t2" was previously used and renamed
+# away from twice (steps 3 and 5), so a tombstone table_version entry for
+# "t2" should still be sitting in LLMETA. The new table must pick that up
+# instead of resetting to 0 -- resetting to 0 would mean re-issuing a version
+# number ("0") that a prior incarnation of "t2" already reported to clients.
 echo "Step 6: Create fresh t2"
 cdb2sql ${CDB2_OPTIONS} $dbnm default "create table t2(id int)" || failexit "create fresh t2"
 
 v2=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $dbnm default "select table_version('t2')")
-echo "  Fresh t2 table_version = $v2 (should be 0 for a brand-new table)"
+echo "  Fresh t2 table_version = $v2 (must not be 0 -- name 't2' was already used)"
+[ -z "$v2" ] && failexit "no table_version returned for fresh t2"
 
-if [ "$v2" != "0" ]; then
-    echo "  BUG: fresh t2 inherited stale table_version $v2 from orphaned LLMETA entry"
-    failexit "Fresh table t2 has stale table_version $v2 instead of 0"
+if [ "$v2" == "0" ]; then
+    echo "  BUG: fresh t2 reset to version 0 even though 't2' is a reused name"
+    failexit "Fresh table t2 has version 0 despite reusing a previously-versioned name"
 fi
 
 # Step 7: Verify data integrity
@@ -90,4 +96,66 @@ if [ -n "$CLUSTER" ]; then
     done
 fi
 
+# Step 9: The exact reuse scenario -- create, alter (bumps version), rename
+# away, then create a brand-new table with the original name. The new table
+# must continue from (at least) the version the original table last had, and
+# must never come back down to 0.
+echo "Step 9: create/alter/rename/recreate must not reset table_version to 0"
+
+cdb2sql ${CDB2_OPTIONS} $dbnm default "create table t3(id int)" || failexit "create t3"
+v3_created=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $dbnm default "select table_version('t3')")
+echo "  t3 version after create = $v3_created"
+[ -z "$v3_created" ] && failexit "no table_version returned for t3 after create"
+[ "$v3_created" != "0" ] && failexit "freshly created t3 should start at version 0, got $v3_created"
+
+cdb2sql ${CDB2_OPTIONS} $dbnm default "alter table t3 add column val int null" || failexit "alter t3"
+v3_altered=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $dbnm default "select table_version('t3')")
+echo "  t3 version after alter = $v3_altered"
+[ -z "$v3_altered" ] && failexit "no table_version returned for t3 after alter"
+if [ "$v3_altered" -le "$v3_created" ]; then
+    failexit "alter should have bumped t3's version above $v3_created, got $v3_altered"
+fi
+
+cdb2sql ${CDB2_OPTIONS} $dbnm default "alter table t3 rename to t3_renamed" || failexit "rename t3->t3_renamed"
+
+cdb2sql ${CDB2_OPTIONS} $dbnm default "create table t3(id int)" || failexit "recreate t3 under the original name"
+v3_recreated=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $dbnm default "select table_version('t3')")
+echo "  t3 version after rename+recreate = $v3_recreated"
+[ -z "$v3_recreated" ] && failexit "no table_version returned for t3 after rename+recreate"
+
+if [ "$v3_recreated" == "0" ] || [ "$v3_recreated" -le "$v3_altered" ]; then
+    echo "  BUG: recreated t3 reused a stale/lower version number"
+    failexit "recreated t3 has version $v3_recreated, expected something greater than $v3_altered (must not reuse an old version number)"
+fi
+
+assertcnt t3 0 "recreated t3 should be empty"
+
+# Step 10: the other half of the invariant -- renaming a table INTO a name
+# that was previously used must not pull that name's version back down to
+# something a prior incarnation of the name already reported to clients.
+echo "Step 10: rename into a vacated name must not reuse that name's versions"
+
+# push t4's version well ahead of anything a freshly created table reaches,
+# so that a rename that ignores t4's history is visibly detectable
+cdb2sql ${CDB2_OPTIONS} $dbnm default "create table t4(id int)" || failexit "create t4"
+for c in val1 val2 val3; do
+    cdb2sql ${CDB2_OPTIONS} $dbnm default "alter table t4 add column $c int null" || failexit "alter t4 add $c"
+done
+v4_last=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $dbnm default "select table_version('t4')")
+echo "  t4 version before drop = $v4_last"
+[ -z "$v4_last" ] && failexit "no table_version returned for t4 before drop"
+cdb2sql ${CDB2_OPTIONS} $dbnm default "drop table t4" || failexit "drop t4"
+
+# fresh, low-versioned table takes over the vacated name
+cdb2sql ${CDB2_OPTIONS} $dbnm default "create table t5(id int)" || failexit "create t5"
+cdb2sql ${CDB2_OPTIONS} $dbnm default "alter table t5 rename to t4" || failexit "rename t5->t4"
+v4_renamed=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $dbnm default "select table_version('t4')")
+echo "  t4 version after rename t5->t4 = $v4_renamed"
+[ -z "$v4_renamed" ] && failexit "no table_version returned for t4 after rename"
+
+if [ "$v4_renamed" -le "$v4_last" ]; then
+    echo "  BUG: rename into 't4' reused version $v4_renamed, already reported by a prior t4"
+    failexit "renamed-into t4 has version $v4_renamed, expected greater than $v4_last"
+fi
+
 echo "Success"