From a83c0e1e861a68778ef216fa69a228c8e74eb0cb Mon Sep 17 00:00:00 2001
From: Xiang Gu <xiang@cockroachlabs.com>
Date: Tue, 28 Mar 2023 14:03:20 -0400
Subject: [PATCH] schemachanger: Annotate tables if ALTER TABLE IF EXISTS is
 short-circuited

Previously, if table `t` does not exist, `ALTER TABLE IF EXISTS t` will
only mark `t` as non-existent. This is inadequate because for stmt
like `ALTER TABLE IF EXISTS t ADD FOREIGN KEY REFERENCES t_other` we
will not touch `t_other` and the validation logic will later complain
that `t_other` is not fully resolved nor marked as non-existent. This
commit fixes it by marking all tables in this ALTER TABLE stmt as
non-existent if the `t` is non-existent, so we can pass the validation.
---
 pkg/sql/logictest/testdata/logic_test/alter_table      | 10 ++++++++++
 .../scbuild/internal/scbuildstmt/alter_table.go        |  7 ++++++-
 pkg/sql/sem/tree/format.go                             |  2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table
index 7c118ee11401..2aa1bb5192d8 100644
--- a/pkg/sql/logictest/testdata/logic_test/alter_table
+++ b/pkg/sql/logictest/testdata/logic_test/alter_table
@@ -3184,3 +3184,13 @@ INSERT INTO t_98269 VALUES (0);
 
 statement error pgcode 0A000 .* cluster_logical_timestamp\(\): nil txn in cluster context
 ALTER TABLE t_98269 ADD COLUMN j DECIMAL NOT NULL DEFAULT cluster_logical_timestamp();
+
+# In #99185, we saw test failures that result from adding a foreign key
+# constraint to a non-existent table with IF EXISTS because we require all tables
+# in the stmt should be marked as non-existent or fully resolved. We didn't do
+# anything to the referenced table name, so the validation logic complained that
+# referenced table name is not fully resolved nor marked as non-existent.
+subtest alter_non_existent_table_with_if_exists
+
+statement ok
+ALTER TABLE IF EXISTS t_non_existent_99185 ADD FOREIGN KEY (i) REFERENCES t_other_99185 (i);
diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go
index c7357fadee60..ecc6d0683f51 100644
--- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go
+++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go
@@ -146,7 +146,12 @@ func AlterTable(b BuildCtx, n *tree.AlterTable) {
 	})
 	_, target, tbl := scpb.FindTable(elts)
 	if tbl == nil {
-		b.MarkNameAsNonExistent(&tn)
+		// Mark all table names (`tn` and others) in this ALTER TABLE stmt as non-existent.
+		tree.NewFmtCtx(tree.FmtSimple, tree.FmtReformatTableNames(func(
+			ctx *tree.FmtCtx, name *tree.TableName,
+		) {
+			b.MarkNameAsNonExistent(name)
+		})).FormatNode(n)
 		return
 	}
 	if target != scpb.ToPublic {
diff --git a/pkg/sql/sem/tree/format.go b/pkg/sql/sem/tree/format.go
index 3e079220e133..4cdca9090c65 100644
--- a/pkg/sql/sem/tree/format.go
+++ b/pkg/sql/sem/tree/format.go
@@ -306,7 +306,7 @@ func FmtPlaceholderFormat(placeholderFn func(_ *FmtCtx, _ *Placeholder)) FmtCtxO
 	}
 }
 
-// FmtReformatTableNames modifies FmtCtx to to substitute the printing of table
+// FmtReformatTableNames modifies FmtCtx to substitute the printing of table
 // naFmtParsable using the provided function.
 func FmtReformatTableNames(tableNameFmt func(*FmtCtx, *TableName)) FmtCtxOption {
 	return func(ctx *FmtCtx) {