go-migrate implementation broken

[senekor]

Version

1.10.0

What happened?

go-migrate files are executed in the wrong order. file 10_foo.up.sql is executed before 9_bar.up.sql. go-migrate specifies that the files are to be executed in order of the version number, not alphabetically.
https://github.com/golang-migrate/migrate/blob/v3.5.4/MIGRATIONS.md

The issue seems to be in internal/sql/sqlpath/read.go, where the migration files are read from the file system in alphabetical order without parsing the go-migrate version number at all.

I suggest sorting the files according to the go-migrate version number before executing them. Alternatively, one could adjust the documentation to clearly state this limitation.

Relevant log output

No response

Database schema

No response

SQL queries

No response

Configuration

No response

Playground URL

No response

What operating system are you using?

Linux

What database engines are you using?

PostgreSQL

What type of code are you generating?

Go

[StevenACoffman]

👋 This is not actually a bug, and is the behavior of golang-migrate, so has nothing to do with sqlc.

Please Note: the files in your golang-migrate migration directory need to be named so that the migrations can be ordered lexicographically. This is accomplished by using a numbered prefix like 001_something.up.sql and 001_something.down.sql

The example in the documentation here in sqlc https://docs.sqlc.dev/en/latest/howto/ddl.html?highlight=migration#golang-migrate uses 20060102.up.sql and 20060102.down.sql but does not explain why this example naming is important.

More information is available in the golang-migrate documentation here: https://github.com/golang-migrate/migrate/blob/master/MIGRATIONS.md#migration-filename-format

[senekor]

Thanks for the reply!

I interpret the go-migrate docs differently, I think the version part of the migration file name should be interpreted numerically and not lexicographically. I've found at least one part of the go-migrate source code indicating that as well.

About the docs first: The second link you provided points to the same document as the link I provided earlier. One sentence from that doc states:

Versions of migrations may be represented as any 64 bit unsigned integer. All migrations are applied upward in order of increasing version number, and downward by decreasing version number.

This clearly indicates to me that the first part of the migration file name is interpreted as a number. The numbers "1" and "01" are the same, so I would expect these two file name prefixes to be treated the same.

Also, please have a look at the following example from the same document:

1_initialize_schema.down.sql
1_initialize_schema.up.sql
2_add_table.down.sql
2_add_table.up.sql
...

Since the developers are surely aware many of their users need more than 9 migrations, it would be short sighted if not negligent to provide this example if the ordering was in fact lexicographic. Adding a tenth migration in this manner would result in a bug. Not so if the ordering is numeric.

The code I found is located in migrate/source/parse.go on line 27. Here, the first capture group of the regex describing the migration file name, which corresponds to the version number, is passed to strconv.ParseUint which disregards leading zeroes, as one would expect.

That being said, I haven't confirmed that no other part of golang-migrate interprets the file names differently. If that was the case, that would clearly be a bug on the golang-migrate side.

Can you tell me which part of the documentation or the source code of golang-migrate indicates to you that the version part of the migration file names are to be interpreted lexicographically instead of numerically?

Thanks for taking the time!

[StevenACoffman]

Great! I am not a maintainer of sqlc, but if you put up a pull request, my experience has been they've been pretty responsive!

[senekor]

My apologies, I haven't addressed the other part of your comment. I don't believe sqlc is actually calling code from golang-migrate. It does not appear in the mod file, nor anywhere else in the code when searching directly. It seems to me that sqlc implements the behavior of golang-migrate itself, although imperfectly, which is the issue at hand.

In the description of the issue, I referenced sqlc/internal/sql/sqlpath/read.go. Specifically, the migration file names seem to be read from the file system on line 22. The resulting list of file names returned by the enclosing Glob is lexicographically ordered, as stated by the documentation of os.ReadDir. This list of file names is used in two places in sqlc/internal/compiler/compile.go: 1st, 2nd. Both of these places simply iterate over the lexicographically ordered list of file names without sorting it first.

I maintain that this is a discrepancy between the behavior of sqlc on one side and both the behavior and documentation of golang-migrate on the other side. This discrepancy is what I consider to be the bug. Hence my recommendation: Either bring the bevahior of sqlc in line with the behavior & documentation of golang-migrate or adjust the documenation of sqlc to clearly state how it behaves differently than golang-migrate, even though it claims to support it without any caveats.

I hope I was able to make things more clear and I'd love to get a response from the maintainers of sqlc.

[senekor]

I don't know enough about the internal workings of sqlc to modify things without running the risk of introducing some other bug. Especially since some other migration approaches are supported as well, which might require some different behavior. If the maintainers decide not to fix this, I'd be happy to make a pull request adjusting the documentation accordingly.