fix(ddl): replace refreshSnapshotAfterLock with orphan-table cleanup in DropDatabase

[LeftHandCold]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #23766

What this PR does / why we need it:

The previous fix (#23767) called refreshSnapshotAfterLock after acquiring the
exclusive lock on mo_database. This advanced the transaction's SnapshotTS,
which bypassed the workspace tombstone transfer mechanism and caused
duplicate-key errors in restore-cluster scenarios where multiple DDLs run
inside a single transaction.

Root cause:
A concurrent 'data branch create table' can commit between the DropDatabase
transaction's snapshot and its exclusive lock acquisition. The new table is
invisible to the current txn, so DROP TABLE IF EXISTS is a no-op for it,
leaving orphan records in mo_tables/mo_columns.

New approach (two-step, no snapshot advancement):

1. listRelationsAtLatestSnapshot: independent read-only txn (no WithTxn),
   queries mo_tables at the latest committed snapshot to discover all tables
   including those committed by concurrent transactions.
2. listVisibleRelations: uses the current txn (WithTxn) to get tables visible
   to the current snapshot. Captured BEFORE Engine.Delete modifies the
   workspace (tombstones would otherwise make tables invisible).
3. diffStringSlice: orphanTables = allTables - visibleTables.
4. deleteOrphanTableRecords: uses exec.ExecTxn (independent txn) to call
   engine.Database.Delete() for each orphan table. Uses the engine API
   (not raw SQL DELETE) because TN expects the full GenDropTableTuple batch
   format. Cannot use DROP TABLE IF EXISTS (calls lockMoDatabase(Shared),
   deadlocks with parent's Exclusive lock).

Atomicity note:
The orphan-cleanup txn and the parent DropDatabase txn commit separately.
If cleanup succeeds but parent fails: orphan records are deleted but the DB
still exists — harmless, orphans should not exist anyway.
If cleanup fails: DropDatabase returns the error and the parent does not
continue.

Also removes lockMoTableSentinel and all 7 call sites (no longer needed)

[qodo-code-review]

Review Summary by Qodo

Fix DROP DATABASE duplicate-key errors by using snapshot read instead of advancing transaction snapshot

🐞 Bug fix

Walkthroughs

Description

• Replace refreshSnapshotAfterLock with listRelationsAtLatestSnapshot to fix duplicate-key
  errors during DROP DATABASE in restore cluster scenarios
• Use separate read-only transaction at latest snapshot to list relations, avoiding workspace
  tombstone stale rowIDs
• Remove unused imports (moruntime, encoding/hex) and simplify table filtering logic
• Fix variable reference from database.GetDatabaseId to db.GetDatabaseId
• Update tests to verify snapshot is NOT advanced during relation listing

Diagram

flowchart LR
  A["DropDatabase acquires<br/>exclusive lock"] --> B["listRelationsAtLatestSnapshot<br/>reads mo_tables in<br/>separate txn"]
  B --> C["Returns deleteTables<br/>and ignoreTables"]
  C --> D["Current txn snapshot<br/>unchanged"]
  D --> E["Workspace tombstone<br/>transfer works correctly"]
  E --> F["No duplicate-key<br/>errors on commit"]

Loading

File Changes

1. pkg/sql/compile/ddl.go 🐞 Bug fix +57/-64

Replace snapshot refresh with separate-txn relation listing

• Removed refreshSnapshotAfterLock function that advanced transaction snapshot via clock.Now()
 and UpdateSnapshot
• Added listRelationsAtLatestSnapshot function that queries mo_tables in a separate read-only
 transaction at latest snapshot
• Replaced snapshot refresh logic in DropDatabase with call to listRelationsAtLatestSnapshot
• Simplified table filtering by using catalog.IsHiddenTable instead of iterating through table
 definitions
• Fixed database.GetDatabaseId reference to use existing db variable
• Removed unused imports: moruntime and encoding/hex

pkg/sql/compile/ddl.go

---

2. pkg/sql/compile/ddl_test.go 🧪 Tests +43/-41

Update tests for snapshot read approach

• Renamed test from TestDropDatabase_SnapshotRefreshAfterExclusiveLock to
 TestDropDatabase_ListRelationsAtLatestSnapshot
• Updated test comments to reflect new snapshot read approach instead of snapshot refresh
• Changed mock assertion from UpdateSnapshot being called once to NOT being called (Times(0))
• Replaced refreshSnapshotAfterLock stub with listRelationsAtLatestSnapshot stub
• Updated test to verify listRelationsAtLatestSnapshot is invoked with correct database name
• Removed timestamp.Timestamp import and related test setup
• Updated error propagation test to stub listRelationsAtLatestSnapshot instead of
 refreshSnapshotAfterLock

pkg/sql/compile/ddl_test.go

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1)

1. Unescaped dbName literal 🐞 Bug ⛨ Security

Description

listRelationsAtLatestSnapshot interpolates dbName into a single-quoted SQL literal using '%s'
without escaping, which can break the query or be abused if a database name contains
quotes/backslashes. The repo already provides a standard mo_tables-in-DB query format that uses %q
for safe quoting/escaping of the database-name literal.

Code

pkg/sql/compile/ddl.go[R3849-3854]

+	sql := fmt.Sprintf(
+		"select %s from `%s`.`%s` where %s = %d and %s = '%s'",
+		catalog.SystemRelAttr_Name, catalog.MO_CATALOG, catalog.MO_TABLES,
+		catalog.SystemRelAttr_AccID, accountID,
+		catalog.SystemRelAttr_DBName, dbName,
+	)

Evidence

The new code constructs a WHERE predicate using reldatabase = '<dbName>' via string interpolation,
which is unsafe for arbitrary identifier values. The catalog package already defines
MoTablesInDBQueryFormat for the same lookup and uses %%q (Go-escaped, quoted string literal) for the
database name, indicating the expected safe pattern in this codebase.

pkg/sql/compile/ddl.go[3848-3854]
pkg/sql/compile/ddl.go[99-106]
pkg/catalog/types.go[622-635]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`listRelationsAtLatestSnapshot` builds an internal SQL query by interpolating `dbName` into a single-quoted SQL literal (`... reldatabase = '%s'`). This does not escape quotes/backslashes and can lead to malformed SQL or unintended query behavior.
### Issue Context
The codebase already defines `catalog.MoTablesInDBQueryFormat`, which performs the same lookup and uses `%q` for the database-name literal.
### Fix Focus Areas
- pkg/sql/compile/ddl.go[3848-3863]
- pkg/catalog/types.go[622-635]
### Suggested change
- Replace the ad-hoc `fmt.Sprintf("... reldatabase = '%s'", ..., dbName)` with `fmt.Sprintf(catalog.MoTablesInDBQueryFormat, accountID, dbName)` (or equivalent safe quoting/escaping for the literal).
- (Optional) Add/adjust a unit test to cover db names containing `'` / `\\` so this doesn’t regress.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. listRelationsAtLatestSnapshot may read stale ☑ 📎 Requirement gap ⛯ Reliability

Description

listRelationsAtLatestSnapshot creates a new transaction but does not ensure the snapshot is
logtail-applied at the time of the read, so it may still miss concurrently committed mo_tables
rows. That can cause DROP DATABASE to skip newly created tables and leave orphan records in
mo_tables under the concurrent CLONE/CREATE TABLE scenario.

Code

pkg/sql/compile/ddl.go[R3848-3864]

+	// Query mo_tables in a fresh transaction to see all committed data.
+	sql := fmt.Sprintf(
+		"select %s from `%s`.`%s` where %s = %d and %s = '%s'",
+		catalog.SystemRelAttr_Name, catalog.MO_CATALOG, catalog.MO_TABLES,
+		catalog.SystemRelAttr_AccID, accountID,
+		catalog.SystemRelAttr_DBName, dbName,
+	)
+
+	exec := c.getInternalSQLExecutor()
+	ctx := c.proc.Ctx
+	opts := executor.Options{}.
+		WithDatabase(c.db).
+		WithTimeZone(c.proc.GetSessionInfo().TimeZone).
+		WithDisableIncrStatement()
+
+	res, err := exec.Exec(ctx, sql, opts)
+	if err != nil {

Evidence

PR Compliance ID 1 requires DROP DATABASE to reliably observe and clean up tables created
concurrently so no orphan mo_tables rows remain. The new code runs an internal SQL query without
waiting for logtail application or otherwise pinning the read to a known-applied timestamp, which
can still allow a stale view of mo_tables in a lagging-CN scenario.

Prevent orphan mo_tables records during concurrent CLONE (CREATE TABLE) and DROP DATABASE
pkg/sql/compile/ddl.go[3848-3864]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`listRelationsAtLatestSnapshot` is intended to read `mo_tables` at the latest snapshot to avoid missing concurrently created tables, but it currently executes an internal SQL query without ensuring the chosen snapshot timestamp is logtail-applied/visible on the CN. This can still lead to a stale `mo_tables` view and orphan `mo_tables` records in the concurrent CLONE/CREATE TABLE vs DROP DATABASE scenario.
## Issue Context
Compliance requires DROP DATABASE to not miss newly created tables under concurrency. The previous implementation explicitly waited for logtail application at a target timestamp; the new implementation should provide an equivalent visibility guarantee while still avoiding advancing the current transaction's SnapshotTS.
## Fix Focus Areas
- pkg/sql/compile/ddl.go[3848-3866]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

1. Unescaped dbname literal 🐞 Bug ⛨ Security

listRelationsAtLatestSnapshot interpolates dbName into a single-quoted SQL literal using '%s'
without escaping, which can break the query or be abused if a database name contains
quotes/backslashes. The repo already provides a standard mo_tables-in-DB query format that uses %q
for safe quoting/escaping of the database-name literal.

Agent Prompt

### Issue description
`listRelationsAtLatestSnapshot` builds an internal SQL query by interpolating `dbName` into a single-quoted SQL literal (`... reldatabase = '%s'`). This does not escape quotes/backslashes and can lead to malformed SQL or unintended query behavior.

### Issue Context
The codebase already defines `catalog.MoTablesInDBQueryFormat`, which performs the same lookup and uses `%q` for the database-name literal.

### Fix Focus Areas
- pkg/sql/compile/ddl.go[3848-3863]
- pkg/catalog/types.go[622-635]

### Suggested change
- Replace the ad-hoc `fmt.Sprintf("... reldatabase = '%s'", ..., dbName)` with `fmt.Sprintf(catalog.MoTablesInDBQueryFormat, accountID, dbName)` (or equivalent safe quoting/escaping for the literal).
- (Optional) Add/adjust a unit test to cover db names containing `'` / `\\` so this doesn’t regress.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools