Introduce ALTER TABLE query #726

bevzzz · 2022-12-06T16:19:43Z

This PR introduces ALTER TABLE query as part of the effort to bring auto-migration to bun, see #456.
Therefore, only a limited scope of the functionality related to modifying a database schema is supported in this step.

Changes also include:

minor refactor of some unit tests
some typos fixed
tiny formatting changes (my IDE has added those, let me know if they do not fit the style and I should remove them)

… in InsertQuery.Ignore()

Unlike most of the other SQL queries, ALTER TABLE has a number of statements it can branch out to. Handling all of them in one object may become overwhelming very soon. + introduce ifExists type alias for boolean `ifExists`

ALTER COLUMN statements can be chained within the same ALTER TABLE query

…emented interfaces in structs

bevzzz · 2022-12-07T14:04:39Z

Something I can't make my up about is this:
When building ALTER COLUMN SET DATA TYPE new_type, which option should we provide?

// Similarly to Column in SelectQuery, but we pass `typ` as `schema.Safe(typ)` rather than `schema.UnsafeIdent(typ)`
func (q *AlterColumnSubquery) Type(typ string) *AlterColumnSubquery {
    q.modification = schema.QueryWithArgs{
	Query: "SET DATA TYPE ?",
        Args:  []interface{}{schema.Safe(typ)},
    }
}  

// Similarly to ColumnExpr, but we append our SET DATA TYPE in front of the user's `query`
func (q *AlterColumnSubquery) TypeExpr(query string, args ...interface{}) *AlterColumnSubquery {
    	query = fmt.Sprintf("SET DATA TYPE %s", query)
	q.modification = schema.SafeQuery(query, args)
}

It is currently done in the 1st way, but I dislike it, because it is not being explicit (in the method's name) about the fact that the input is treated as a "safe" expression.
The second way is somewhat similar in the way that Order() is handled in SelectQuery in that it manipulates user's input before wrapping it in schema.QueryWithArgs, so I would be somehow leaning its way.

What do you think?

I'm using SET DATA TYPE as an example here, but there're other cases (such as ADD CONTRAINT or SET DEFAULT) that follow roughly the same pattern.

vmihailenco · 2022-12-08T07:52:37Z

I am not sure, but we can start with collecting examples for different RDBMS.

PostgreSQL:

ALTER TABLE table_name
ALTER COLUMN column_name1 [SET DATA] TYPE new_data_type,
ALTER COLUMN column_name2 [SET DATA] TYPE new_data_type,

MySQL:

ALTER TABLE table_name    
MODIFY COLUMN column_name1 data_type,  
MODIFY COLUMN column_name2 data_type ... ;

MSSQL:

ALTER TABLE table_name
ALTER COLUMN column_name datatype;

SQLite does not support it and requires creating a new table and copying existing data into it.

So to work on all supported RDBMS it should be something like:

db.NewAlterTable().AlterColumnType(columnName, columnType)

At least that is my line of thinking without looking at the PR.

vmihailenco · 2022-12-08T07:54:01Z

internal/dbtest/db_test.go

@@ -224,6 +224,13 @@ func funcName(x interface{}) string {
 	return s
 }

+func skipIfNotHasFeature(tb testing.TB, db *bun.DB, feat feature.Feature, featName string) {


nitpick: skipIfNoFeature

vmihailenco · 2022-12-08T07:55:42Z

internal/dbtest/query_test.go

+			"rename column", needsAlterTable,
+			func(db *bun.DB) schema.QueryAppender {
+				return db.NewAlterTable().Model((*Model)(nil)).
+					RenameColumn().Column("old").To("new")


Any reasons why not simply RenameColumn("old_name", "new_name")?

Ditto: #726 (comment)

vmihailenco · 2022-12-08T07:56:08Z

internal/dbtest/query_test.go

+			"rename table", needsAlterTable,
+			func(db *bun.DB) schema.QueryAppender {
+				return db.NewAlterTable().Model((*Model)(nil)).
+					Rename().To("new_models")


Why not RenameTable("new_name")?

You're right, that's actually what I started with, but I wasn't sure if it wasn't more consistent with the rest of the API to mirror actual the SQL keywords.
I agree that that what you've suggested is much more readable and explicit though.

vmihailenco · 2022-12-08T07:57:18Z

internal/dbtest/query_test.go

+			func(db *bun.DB) schema.QueryAppender {
+				// Change column type with common SQL data type
+				return db.NewAlterTable().Model((*Model)(nil)).
+					AlterColumn().Column("old").Type(sqltype.Blob)


Same suggestion AlterColumnType(colName, colType)

Ditto: #726 (comment)

vmihailenco · 2022-12-08T08:02:35Z

query_alter_table.go

+type ChainableSubquery interface {
+	SubqueryAppender
+	chain()
+}


Looks like you've come with some new design/idea for Bun query builder. It could be a nice idea, but let's describe it first and come to understanding how we will use it with other queries like SELECT/UPDATE/INSERT.

...but I'd rather not design something new just for ALTER TABLE and instead try to solve the task with existing APIs.

Agree.
I've added some more thoughts about that below.

Going to refactor ChainableSubquery away in favor of the existing API 👌.
It can stay in Git history, or I could describe it in a proposal, if you think it could be used in the rest of the lib.

vmihailenco · 2022-12-08T08:03:51Z

query_alter_table.go

+		return b, nil
+	}
+	return append(b, "IF NOT EXISTS "...), nil
+}


Looks like over-engineering to me, but if you insist... :)

Perhaps you're right; I was toying with the concept of custom types here.
I'm rather new to Go, still trying to discover what is and isn't idiomatic.

vmihailenco · 2022-12-08T08:10:09Z

After looking at the PR it looks like it makes sense to split AlterTableQuery into separate queries, e.g.:

db.NewRenameTable().Table("old_table_name").To("new_table_name")
db.NewRenameTable().Model(&model).To("new_table_name")
db.NewAlterColumnType().Model(&model).Column("name1", "type1").Column("name2", "type2")
db.NewDropColumn().Model(&model).Column("col1").Column("col2")

That would make API cleaner and we won't need chainable/parent queries any more. WDYT?

bevzzz · 2022-12-09T18:49:02Z

That would make API cleaner and we won't need chainable/parent queries any more. WDYT?

tldr;
Yes, I definitely agree that it would make sense to handle each modification separately, primarily because of the stark differences in syntax and degree of support for ALTER TABLE in the 4 DBMS.

Syntax and feature support

I am not sure, but we can start with collecting examples for different RDBMS.

Following your suggestion, I've put up the examples for most of the queries from this list in the dialects Bun supports.
I'm posting them here as a convenient summary and also as a reference for future development:

RENAME TABLE

-- PostgresSQL
ALTER TABLE t RENAME TO new;

-- MySQL
ALTER TABLE t RENAME TO new;
-- or, for multiple renamed tables:
RENAME TABLE
	t TO new,
	d TO dew;

-- MSSQL
EXEC sp_rename 't', 'new', 'TABLE';

-- SQLite
ALTER TABLE t RENAME TO new;

RENAME COLUMN

-- PostgresSQL
ALTER TABLE t RENAME COLUMN a TO new_a;

-- MySQL
ALTER TABLE t RENAME COLUMN a TO new_a;

-- MSSQL
EXEC sp_rename 't.a', 'col_a', 'COLUMN'

-- SQLite
ALTER TABLE t RENAME COLUMN a TO new_a;

CHANGE COLUMN TYPE

-- PostgresSQL
ALTER TABLE t
	ALTER COLUMN a SET DATA TYPE new_data_type,
	ALTER COLUMN b SET DATA TYPE new_data_type;

-- MySQL
ALTER TABLE t
	MODIFY a new_data_type,
	MODIFY b new_data_type;

-- MSSQL
ALTER TABLE table_name ALTER COLUMN a new_data_type;
ALTER TABLE table_name ALTER COLUMN b new_data_type;


-- SQLite
Not supported in the standard API, probably something like this:
BEGIN TRANSACTION
ALTER TABLE t ADD COLUMN __temp_a new_data_type_a;
UPDATE t SET __temp_a = a;
ALTER TABLE t DROP COLUMN a;
ALTER TABLE t RENAME COLUMN __temp_a TO a;
COMMIT;

ADD COLUMN

-- PostgresSQL
ALTER TABLE t
	ADD COLUMN a data_type,
	ADD COLUMN b data_type;

-- MySQL
ALTER TABLE t
	ADD COLUMN a data_type,
	ADD COLUMN b data_type;

-- MSSQL
ALTER TABLE t
ADD 
	a data_type,
	b data_type;

-- SQLite
ALTER TABLE t ADD COLUMN a data_type;
ALTER TABLE t ADD COLUMN b data_type;

SET DEFAULT

-- PostgresSQL
ALTER TABLE t 
	ALTER COLUMN a SET DEFAULT 0,
	ALTER COLUMN b SET DEFAULT 0;

-- MySQL
ALTER TABLE t
	ALTER COLUMN a SET DEFAULT 0,
	ALTER COLUMN b SET DEFAULT 0;

-- MSSQL
ALTER TABLE t ADD
	CONSTRAINT my_default_value_a DEFAULT 0 FOR a,
	CONSTRAINT my_default_value_a DEFAULT 0 FOR b; 

-- SQLite
Not supported in the standard API:

BEGIN TRANSACTION
ALTER TABLE t ADD COLUMN __temp_a data_type_a DEFAULT 0;
UPDATE t SET __temp_a = a;
ALTER TABLE t DROP COLUMN a;
ALTER TABLE t RENAME COLUMN __temp_a TO a;
COMMIT;

There are 2 things to notice here:

SQLite has limited support for ALTER TABLE. Rename table, rename/add/drop column are the only 4 queries supported, the rest must be done by moving data to temp columns/tables and back.
Some DMBS allow executing multiple statements within one query. This was the reason I've introduced the ChainableSubquery in the first place -- that should've been a mechanism for e.g., changing data types in 5 columns, adding a new column, and dropping a default constraint from another one within a single SQL statement. However, with that not being the case for all 4 DBMS (PostgresSQL and MySQL the most consistent support for "chainable subqueries"), I think it's not worth breaking our backs trying to accommodate that in the Bun's API in this iteration.
_Different "angles" _. For example, while setting a default is "altering the column definition" for PostgreSQL and MySQL, it is "adding a table constraint" in MSSQL.

In conclusion, I think these APIs for altering DB schema have less commonalities than I have initially hoped for, and in this case it really makes more sense to design Bun's API around specific operations rather than SQL syntactic constructs.

vmihailenco · 2022-12-10T14:07:51Z

really makes more sense to design Bun's API around specific operations rather than SQL syntactic constructs.

Fully agree. Perhaps even ignore AlterTableQuery altogether and just provide minimal API like:

RenameTable(context.Context, oldName, newName string) error
AddColumn(context.Context, *sqlschema.Column) error
DropColumn(context.Context, colName string) error
ChangeColumnType(context.Context, colName, colType string) error

This way you can do whatever you want in dialects, .e.g. start transactions, create temp tables etc.

bevzzz · 2022-12-28T19:57:18Z

After giving it some thought here's roughly the solution I came up with:

~~Define AlterTableQuery, which has methods for individual table-modifications~~
The reason I want to continue with the standard bun-query approach is:
a) familiar syntax
b) the way models are parsed in baseQuery is very helpful and easily reusable

Edit: Provide minimal API, as suggested here.

Although baseQuery does a very good job at parsing the models, it's important to remember that at the time any of the ALTER TABLE functions are called, the bun models are likely already modified and the database schema is not yet modified.
Eventually, when auto-migration is introduced, there shouldn't be a need for the higher-level AlterTableQuery at all, as migrations can be done with simple SyncModels((*Model)(nil)).

Delegate building the query to individual dialects. Because of the differences in the syntax, they seem like the best place to handle that

type Dialect interface {
    RenameTable(...) []QueryWithArgs
    AddColumn(...)   []QueryWithArgs
}

These methods return a slice of queries, so that, for instance, SQLite could build several queries for something like SET DEFAULT (which is not natively supported).

Execute multiple raw queries in a transaction. The "exec" step belongs to bun, because that's the part of the program that holds the db connection.

package bun


func DropDefault(ctx context.Context, db *DB, colName string) {
    // Build query
    ...
    // Do
    if len(q.mod) > 1 {
        // execute raw query at q.mod[0]
    }
    // start a transaction and execute each one of the queries in q.mod
}

Will add implementation for that tomorrow, let's see how that works out.

vmihailenco · 2023-01-05T10:27:48Z

Delegate building the query to individual dialects. Because of the differences in the syntax, they seem like the best place to handle that

The idea sounds right, but returning []QueryWithArgs looks very weird. It should be much simpler to just execute the query instead of constructing some weird constructs like []QueryWithArgs.

I guess you are worried about not having access to bun.DB in dialects, but we can introduce a separate sub-package that works with bun.DB and does some dispatching for different dialects internally...

bevzzz · 2023-01-08T22:13:47Z

You're right, I too thought that returning []QueryWithArgs felt awkward. The reason I would like the dialects to only build the query (doesn't have to be a "bun query") itself and to handle its execution in a separate package is something I've mentioned in one of my earlier comments:

Once we've got auto-migrations figured out, we could leverage the fact that all bun's queries implement Stringer and create "up" and "down" SQL-migration files for the user.

The only alternative that comes to my mind atm is to have the dialects return []byte like other QueryAppenders do, but it's only a slight improvement over []QueryWithArgs. WDYT? Is there a more elegant solution?

I guess you are worried about not having access to bun.DB in dialects, but we can introduce a separate sub-package that works with bun.DB and does some dispatching for different dialects internally...

Yes, if we put ALTER TABLE functionality in a sub-separate package, we could just pass bun.DB as an argument.

vmihailenco · 2023-01-09T07:37:14Z

build the query (doesn't have to be a "bun query") itself and to handle its execution in a separate package

I've missed that, but it would be nice to have. It is not mandatory though and we could start with Go-only migrations.

Perhaps we could achieve that by having some special *sql.DB abstracted under an interface that would record queries instead of executing them. For example, https://github.com/DATA-DOG/go-sqlmock records queries for testing, but we could write them into a file instead.

bevzzz added 5 commits December 5, 2022 01:11

chore: fix typos in comments

d605c42

refactor: db.HasFeature is a shortcut for db.fmter.HasFeature, use it…

911ce6a

… in InsertQuery.Ignore()

test: add helper function to skip tests that do not support a feature

a6ce183

feat: allow renaming columns with ALTER TABLE query

bcb8135

feat: add "rename table" action to AlterTable

f8ff602

bevzzz changed the title ~~Feat/alter table query~~ Introduce ALTER TALBE query Dec 6, 2022

bevzzz changed the title ~~Introduce ALTER TALBE query~~ Introduce ALTER TABLE query Dec 6, 2022

bevzzz added 6 commits December 6, 2022 22:54

feat: change column type in ALTER COLUMN subquery

99743c9

ALTER COLUMN statements can be chained within the same ALTER TABLE query

test: add human-readable names to test cases

e409c36

refactor: do not expand package's API, use existing schema.QueryWithArgs

234f34f

feat: add DROP/ADD COLUMN subqueries to ALTER TABLE

8abcf63

refactor: unify code style, use existing patterns, and highlight impl…

533383a

…emented interfaces in structs

bevzzz force-pushed the feat/alter_table_query branch from acbf516 to 533383a Compare December 6, 2022 21:56

vmihailenco reviewed Dec 8, 2022

View reviewed changes

Merge branch 'uptrace:master' into feat/alter_table_query

7c926a4

bevzzz mentioned this pull request Nov 1, 2023

AutoMigrate: Rename/Create/Drop tables #926

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ALTER TABLE query #726

Introduce ALTER TABLE query #726

bevzzz commented Dec 6, 2022

bevzzz commented Dec 7, 2022 •

edited

vmihailenco commented Dec 8, 2022

vmihailenco Dec 8, 2022

vmihailenco Dec 8, 2022

bevzzz Dec 9, 2022

vmihailenco Dec 8, 2022

bevzzz Dec 9, 2022 •

edited

vmihailenco Dec 8, 2022

bevzzz Dec 9, 2022

vmihailenco Dec 8, 2022

vmihailenco Dec 8, 2022

bevzzz Dec 9, 2022

vmihailenco Dec 8, 2022

bevzzz Dec 9, 2022

vmihailenco commented Dec 8, 2022 •

edited

bevzzz commented Dec 9, 2022 •

edited

vmihailenco commented Dec 10, 2022

bevzzz commented Dec 28, 2022 •

edited

vmihailenco commented Jan 5, 2023

bevzzz commented Jan 8, 2023 •

edited

vmihailenco commented Jan 9, 2023 •

edited

Introduce ALTER TABLE query #726

Are you sure you want to change the base?

Introduce ALTER TABLE query #726

Conversation

bevzzz commented Dec 6, 2022

bevzzz commented Dec 7, 2022 • edited

vmihailenco commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bevzzz Dec 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmihailenco commented Dec 8, 2022 • edited

bevzzz commented Dec 9, 2022 • edited

vmihailenco commented Dec 10, 2022

bevzzz commented Dec 28, 2022 • edited

vmihailenco commented Jan 5, 2023

bevzzz commented Jan 8, 2023 • edited

vmihailenco commented Jan 9, 2023 • edited

bevzzz commented Dec 7, 2022 •

edited

bevzzz Dec 9, 2022 •

edited

vmihailenco commented Dec 8, 2022 •

edited

bevzzz commented Dec 9, 2022 •

edited

bevzzz commented Dec 28, 2022 •

edited

bevzzz commented Jan 8, 2023 •

edited

vmihailenco commented Jan 9, 2023 •

edited