Add an additional commit for CREATE DATABASE in some cases

2026-05-12 11:29:01 -05:00 · 2024-09-10 15:59:57 -07:00
parent a21836217e
commit ec621a67b9
2 changed files with 104 additions and 17 deletions
@@ -411,7 +411,44 @@ func (p *DoltDatabaseProvider) CreateDatabase(ctx *sql.Context, name string) err
 	return p.CreateCollatedDatabase(ctx, name, sql.Collation_Default)
 }

+func commitTransaction(ctx *sql.Context, dSess *dsess.DoltSession, rsc *doltdb.ReplicationStatusController) error {
+	currentTx := ctx.GetTransaction()
+
+	err := dSess.CommitTransaction(ctx, currentTx)
+	if err != nil {
+		return err
+	}
+	newTx, err := dSess.StartTransaction(ctx, sql.ReadWrite)
+	if err != nil {
+		return err
+	}
+	ctx.SetTransaction(newTx)
+
+	if rsc != nil {
+		dsess.WaitForReplicationController(ctx, *rsc)
+	}
+
+	return nil
+}
+
 func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name string, collation sql.CollationID) (err error) {
+	exists, isDir := p.fs.Exists(name)
+	if exists && isDir {
+		return sql.ErrDatabaseExists.New(name)
+	} else if exists {
+		return fmt.Errorf("Cannot create DB, file exists at %s", name)
+	}
+
+	sess := dsess.DSessFromSess(ctx.Session)
+	var rsc doltdb.ReplicationStatusController
+
+	// before we create a new database, we need to implicitly commit any current transaction, because we'll begin a new
+	// one after we create the new DB
+	err = commitTransaction(ctx, sess, &rsc)
+	if err != nil {
+		return err
+	}
+
 	p.mu.Lock()
 	needUnlock := true
 	defer func() {
@@ -420,13 +457,6 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str
 		}
 	}()

-	exists, isDir := p.fs.Exists(name)
-	if exists && isDir {
-		return sql.ErrDatabaseExists.New(name)
-	} else if exists {
-		return fmt.Errorf("Cannot create DB, file exists at %s", name)
-	}
-
 	err = p.fs.MkDirs(name)
 	if err != nil {
 		return err
@@ -445,7 +475,6 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str
 	}

 	// TODO: fill in version appropriately
-	sess := dsess.DSessFromSess(ctx.Session)
 	newEnv := env.Load(ctx, env.GetCurrentUserHomeDir, newFs, p.dbFactoryUrl, "TODO")

 	newDbStorageFormat := types.Format_Default
@@ -454,6 +483,8 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str
 		return err
 	}

+	updatedCollation, updatedSchemas := false, false
+
 	// Set the collation
 	if collation != sql.Collation_Default {
 		workingRoot, err := newEnv.WorkingRoot(ctx)
@@ -471,6 +502,8 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str
 		if err = newEnv.UpdateStagedRoot(ctx, newRoot); err != nil {
 			return err
 		}
+
+		updatedCollation = true
 	}

 	// If the search path is enabled, we need to create our initial schema object (public and pg_catalog are available
@@ -500,6 +533,8 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str
 		if err = newEnv.UpdateStagedRoot(ctx, workingRoot); err != nil {
 			return err
 		}
+
+		updatedSchemas = true
 	}

 	err = p.registerNewDatabase(ctx, name, newEnv)
@@ -507,21 +542,36 @@ func (p *DoltDatabaseProvider) CreateCollatedDatabase(ctx *sql.Context, name str
 		return err
 	}

-	if resolve.UseSearchPath {
+	needsDoltCommit := updatedSchemas || updatedCollation
+	if needsDoltCommit {
 		// Need to unlock the provider early to avoid a deadlock with the commit
 		needUnlock = false
 		p.mu.Unlock()

-		// After creating these schemas, commit them as well so that any newly created branches have them
-		// TODO: it would be better if there weren't a commit for this database where these schemas didn't exist, but
+		// Since we just created this database, we need a new transaction to inspect its roots
+		err = commitTransaction(ctx, sess, &rsc)
+		if err != nil {
+			return err
+		}
+
+		// After making changes to the working set for the DB, create a new dolt commit so that any newly created
+		// branches have those changes
+		// TODO: it would be better if there weren't a commit for this database where these changes didn't exist, but
 		//  we always create an empty commit as part of initializing a repo right now.
 		roots, ok := sess.GetRoots(ctx, name)
 		if !ok {
 			return fmt.Errorf("unable to get roots for database %s", name)
 		}

+		t := ctx.QueryTime()
+		userName := ctx.Client().User
+		userEmail := fmt.Sprintf("%s@%s", ctx.Client().User, ctx.Client().Address)
+
 		pendingCommit, err := sess.NewPendingCommit(ctx, name, roots, actions.CommitStagedProps{
 			Message: "CREATE DATABASE",
+			Date:    t,
+			Name:    userName,
+			Email:   userEmail,
 		})
 		if err != nil {
 			return err
@@ -104,15 +104,52 @@ func TestSchemaOverrides(t *testing.T) {

 // Convenience test for debugging a single query. Unskip and set to the desired query.
 func TestSingleScript(t *testing.T) {
-	t.Skip()
+	// t.Skip()

 	var scripts = []queries.ScriptTest{
 		{
-			Name:        "",
-			SetUpScript: []string{},
-			Assertions:  []queries.ScriptTestAssertion{},
-		},
-	}
+			Name: "CREATE DATABASE error handling",
+			Assertions: []queries.ScriptTestAssertion{
+				{
+					Query:       "create database `abc/def`",
+					ExpectedErr: sql.ErrInvalidDatabaseName,
+				},
+				{
+					Query:    "CREATE DATABASE newtestdb CHARACTER SET utf8mb4 ENCRYPTION='N'",
+					Expected: []sql.Row{{gmstypes.NewOkResult(1)}},
+				},
+				{
+					SkipResultCheckOnServerEngine: true, // tracking issue here, https://github.com/dolthub/dolt/issues/6921. Also for when run with prepares, the warning is added twice
+					Query:                         "SHOW WARNINGS /* 1 */",
+					Expected:                      []sql.Row{{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"}},
+				},
+				{
+					Query:    "CREATE DATABASE newtest1db DEFAULT COLLATE binary ENCRYPTION='Y'",
+					Expected: []sql.Row{{gmstypes.NewOkResult(1)}},
+				},
+				{
+					SkipResultCheckOnServerEngine: true, // tracking issue here, https://github.com/dolthub/dolt/issues/6921.
+					// TODO: There should only be one warning (the warnings are not clearing for create database query) AND 'PREPARE' statements should not create warning from its query
+					Query: "SHOW WARNINGS /* 2 */",
+					Expected: []sql.Row{
+						{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"},
+						{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"},
+					},
+				},
+				{
+					Query:       "CREATE DATABASE mydb",
+					ExpectedErr: sql.ErrDatabaseExists,
+				},
+				{
+					Query:    "CREATE DATABASE IF NOT EXISTS mydb",
+					Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: 1}}},
+				},
+				{
+					Query:    "SHOW WARNINGS /* 3 */",
+					Expected: []sql.Row{{"Note", 1007, "Can't create database mydb; database exists "}},
+				},
+			},
+		}}

 	for _, script := range scripts {
 		harness := newDoltHarness(t)