Merge #59675 #59800

59675: sql: disallow virtual columns in FK references r=RaduBerinde a=RaduBerinde

Disallow FK references to or from virtual columns. Note that there is
no big reason why we couldn't support them other than some extra work
and testing surface; this is tracked by #59671.

Informs #57608.

Release note: None

59800: roachprod: extend error reporting for DNS sync r=rail a=rail

Related to #58905

In some cases `roachprod sync` fails updating the crdb.io DNS entries
without giving enough information about the failure. It looks like the
root cause may be the file format passed to the `gcloud dns` command.

Added extra information in the error message in order to get a better
picture when it happens again.

Release note: None

Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>

pkg/cmd/roachprod/vm/gce/utils.go

@@ -18,6 +18,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"strings"
 	"text/template"
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachprod/vm"
@@ -161,19 +162,24 @@ func SyncDNS(vms vm.List) error {
 			fmt.Fprintf(os.Stderr, "removing %s failed: %v", f.Name(), err)
 		}
 	}()
+
+	var zoneBuilder strings.Builder
 	for _, vm := range vms {
 		if len(vm.Name) < 60 {
-			fmt.Fprintf(f, "%s 60 IN A %s\n", vm.Name, vm.PublicIP)
+			zoneBuilder.WriteString(fmt.Sprintf("%s 60 IN A %s\n", vm.Name, vm.PublicIP))
+		} else {
+			fmt.Printf("WARN: not adding `%s' to the zone file because it is longer than 60 characters\n", vm.Name)
 		}
 	}
+	fmt.Fprint(f, zoneBuilder.String())
 	f.Close()
 
 	args := []string{"--project", dnsProject, "dns", "record-sets", "import",
 		"-z", dnsZone, "--delete-all-existing", "--zone-file-format", f.Name()}
 	cmd := exec.Command("gcloud", args...)
 	output, err := cmd.CombinedOutput()
 
-	return errors.Wrapf(err, "Command: gcloud %s\nOutput: %s", args, output)
+	return errors.Wrapf(err, "Command: %s\nOutput: %s\nZone file contents:\n%s", cmd, output, zoneBuilder.String())
 }
 
 // GetUserAuthorizedKeys retreives reads a list of user public keys from the

pkg/sql/alter_table.go

@@ -300,16 +300,6 @@ func (n *alterTableNode) startExec(params runParams) error {
 				}
 
 			case *tree.ForeignKeyConstraintTableDef:
-				for _, colName := range d.FromCols {
-					col, err := n.tableDesc.FindActiveOrNewColumnByName(colName)
-					if err != nil {
-						return err
-					}
-
-					if err := col.CheckCanBeFKRef(); err != nil {
-						return err
-					}
-				}
 				affected := make(map[descpb.ID]*tabledesc.Mutable)
 
 				// If there are any FKs, we will need to update the table descriptor of the

pkg/sql/catalog/descpb/column.go

@@ -45,11 +45,30 @@ func (desc *ColumnDescriptor) ColName() tree.Name {
 	return tree.Name(desc.Name)
 }
 
-// CheckCanBeFKRef returns whether the given column is computed.
-func (desc *ColumnDescriptor) CheckCanBeFKRef() error {
+// CheckCanBeOutboundFKRef returns whether the given column can be on the
+// referencing (origin) side of a foreign key relation.
+func (desc *ColumnDescriptor) CheckCanBeOutboundFKRef() error {
+	if desc.Virtual {
+		return unimplemented.NewWithIssuef(
+			59671, "virtual column %q cannot reference a foreign key",
+			desc.Name,
+		)
+	}
 	if desc.IsComputed() {
 		return unimplemented.NewWithIssuef(
-			46672, "computed column %q cannot be a foreign key reference",
+			46672, "computed column %q cannot reference a foreign key",
+			desc.Name,
+		)
+	}
+	return nil
+}
+
+// CheckCanBeInboundFKRef returns whether the given column can be on the
+// referenced (target) side of a foreign key relation.
+func (desc *ColumnDescriptor) CheckCanBeInboundFKRef() error {
+	if desc.Virtual {
+		return unimplemented.NewWithIssuef(
+			59671, "virtual column %q cannot be referenced by a foreign key",
 			desc.Name,
 		)
 	}

pkg/sql/create_table.go

@@ -807,7 +807,7 @@ func ResolveFK(
 		if err != nil {
 			return err
 		}
-		if err := col.CheckCanBeFKRef(); err != nil {
+		if err := col.CheckCanBeOutboundFKRef(); err != nil {
 			return err
 		}
 		// Ensure that the origin columns don't have duplicates.
@@ -889,6 +889,12 @@ func ResolveFK(
 		return err
 	}
 
+	for i := range referencedCols {
+		if err := referencedCols[i].CheckCanBeInboundFKRef(); err != nil {
+			return err
+		}
+	}
+
 	if len(referencedCols) != len(originCols) {
 		return pgerror.Newf(pgcode.Syntax,
 			"%d columns must reference exactly %d columns in referenced table (found %d)",

pkg/sql/logictest/testdata/logic_test/computed

@@ -358,17 +358,17 @@ CREATE TABLE y (
   r INT AS (q) STORED
 )
 
-statement error computed column "r" cannot be a foreign key reference
+statement error computed column "r" cannot reference a foreign key
 CREATE TABLE y (
   r INT AS (1) STORED REFERENCES x (a)
 )
 
-statement error computed column "r" cannot be a foreign key reference
+statement error computed column "r" cannot reference a foreign key
 CREATE TABLE y (
   r INT AS (1) STORED REFERENCES x
 )
 
-statement error computed column "r" cannot be a foreign key reference
+statement error computed column "r" cannot reference a foreign key
 CREATE TABLE y (
   a INT,
   r INT AS (1) STORED REFERENCES x
@@ -393,14 +393,14 @@ CREATE TABLE xx (
   UNIQUE (a, b)
 )
 
-statement error computed column "y" cannot be a foreign key reference
+statement error computed column "y" cannot reference a foreign key
 CREATE TABLE yy (
   x INT,
   y INT AS (3) STORED,
   FOREIGN KEY (x, y) REFERENCES xx (a, b)
 )
 
-statement error computed column "y" cannot be a foreign key reference
+statement error computed column "y" cannot reference a foreign key
 CREATE TABLE yy (
   x INT,
   y INT AS (3) STORED,
@@ -416,7 +416,7 @@ CREATE TABLE y (
   INDEX (r)
 )
 
-statement error computed column "r" cannot be a foreign key reference
+statement error computed column "r" cannot reference a foreign key
 ALTER TABLE y ADD FOREIGN KEY (r) REFERENCES x (a)
 
 statement ok

pkg/sql/logictest/testdata/logic_test/virtual_columns

@@ -500,3 +500,45 @@ a  b   v   w1  w2
 1  10  11  10  20
 2  20  22  40  40
 3  30  33  90  60
+
+# Verify that FK relations on virtual columns are disallowed.
+statement ok
+CREATE TABLE fk (
+  a INT PRIMARY KEY,
+  b INT,
+  c INT,
+  u INT UNIQUE AS (b+c) VIRTUAL
+)
+
+statement error virtual column "u" cannot be referenced by a foreign key
+CREATE TABLE fk2 (
+  p INT PRIMARY KEY,
+  c INT REFERENCES fk(u)
+)
+
+statement error virtual column "c" cannot reference a foreign key
+CREATE TABLE fk2 (
+  p INT PRIMARY KEY,
+  c INT AS (p+1) VIRTUAL REFERENCES fk(a)
+)
+
+statement error virtual column "u" cannot be referenced by a foreign key
+CREATE TABLE fk2 (
+  p INT PRIMARY KEY,
+  q INT,
+  r INT,
+  CONSTRAINT fk FOREIGN KEY (q,r) REFERENCES fk(a,u)
+)
+
+statement ok
+CREATE TABLE fk2 (
+  x INT PRIMARY KEY,
+  y INT,
+  v INT AS (x+y) VIRTUAL
+)
+
+statement error virtual column "u" cannot be referenced by a foreign key
+ALTER TABLE fk2 ADD CONSTRAINT foo FOREIGN KEY (x) REFERENCES fk(u)
+
+statement error virtual column "v" cannot reference a foreign key
+ALTER TABLE fk2 ADD CONSTRAINT foo FOREIGN KEY (v) REFERENCES fk(a)