Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(postgres): allow running multiple statements in no tx migrations #3694

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

adriangb
Copy link

Closes #3693

Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #3574 for previous discussion.

I would prefer if the style matched the existing -- no-transaction annotation.

To make the annotation more selective, I would require it to match on a full line. That way someone can write a comment talking about it without accidentally introducing a statement break where they didn't intend to.

I also don't see much benefit in namespacing the annotations. I don't see much harm, but I also don't see much benefit.

Since the existing annotation isn't namespaced, adding a namespaced annotation (as well as using a different comment style) would just be confusing to the user.

@titaniumtraveler
Copy link
Contributor

I also don't see much benefit in namespacing the annotations. I don't see much harm, but I also don't see much benefit.

My argument to add namespacing was mostly about making it clearer to the user that the annotation is in-fact an annotation. (-- no-transaction might get confused with a regular comment, -- sqlx:no-transaction definitely NOT.)

Also it implicitly communicates that the annotation comes from sqlx, which makes it easier to find out why it is there and what it does.

The last point is that I am reluctant about whitespaces in user interfaces, because they are far too easy to screw up and might lead to hard to diagnose errors.

Like when writing an annotation a user might accidentally add an tab character instead of space, or have two spaces instead of one. That is especially the case when dealing with a system that has relatively little insight into what is happening as this.

Since the existing annotation isn't namespaced, adding a namespaced annotation (as well as using a different comment style) would just be confusing to the user.

But yeah. In the end all those points don't have too much impact and just a few things I tend to be cautious of.

@adriangb
Copy link
Author

@abonander any other changes other than renaming and matching the full line?

Do you generally think this implementation is more viable?

@adriangb adriangb requested a review from abonander January 20, 2025 13:48
@adriangb
Copy link
Author

@abonander I've renamed as per your request and made it only match a full line, including adding a test for the latter

CREATE INDEX CONCURRENTLY test_table_x_idx ON test_table (x);
-- split-migration
INSERT INTO test_table (x) VALUES (1);
-- prove that you can have a comment that won't split -- split-migration DROP TABLE does_not_exist;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that if I remove the \ns from the string split this gets split such that DROP TABLE does_not_exist; becomes runnable SQL that then fails to run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support running multiple statements per migrations file
3 participants