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

PostgresqlConnectionManager SplitScriptIntoCommands isn't splitting the script, causing unexpected transaction errors #11

Open
mgkeen opened this issue Feb 20, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@mgkeen
Copy link

mgkeen commented Feb 20, 2023

I noticed this when trying to create a script with multiple DROP INDEX CONCURRENTLY statements in it. I was getting weird transaction errors even when not running in a transaction.

The regex pattern is ^\\s*;\\s*$. The ^ is causing this to only match if a ; is on a line by itself surrounded by whitespace.

That means the following script is being executed as a single statement when it should be separated into multiple statements. By executing as a single statement you get the error DROP INDEX CONCURRENTLY must be first action in transaction for index_2:

DROP INDEX CONCURRENTLY index_1;
DROP INDEX CONCURRENTLY index_2;
DROP INDEX CONCURRENTLY index_3;

I'm sure there's some edge cases for this as well to avoid picking up ; from within strings etc. I'll put up a PR to resolve the issue as best I can soon.

For anyone else getting this problem right now, as a workaround you can do the following. It looks silly but it will match the regex and cause the script to be split properly:

DROP INDEX CONCURRENTLY individual_full_name_trgm
;
DROP INDEX CONCURRENTLY individual_name_trgm
;
DROP INDEX CONCURRENTLY individual_phone_trgm
;
@mgkeen mgkeen added the bug Something isn't working label Feb 20, 2023
@mgkeen
Copy link
Author

mgkeen commented Feb 20, 2023

Turns out this is pretty gnarly to sort out given the various ways for e.g. creating procedures using things like dollar quoted string constants in postgres.
The other implementations use the SqlCommandReader which gets it most of the way there, but that doesn't understand some of the postgres specific language right now.
I have a branch that is starting on this here but i've run out of time for now to chase down those rabbit holes. Hopefully I'll get some time soon to progress this, but if someone else wants to pick it up feel free!

@sungam3r
Copy link
Member

Separating statements into different files is the most secure approach IMO.

@mgkeen
Copy link
Author

mgkeen commented Feb 21, 2023

Yup could certainly make the argument for that, particularly when there aren't transactions involved which seems to be the only time this matters anyway. It's sometimes nice to group things together for the sake of making changes easier to understand though.

Either way, its current behaviour is somewhat unexpected and there's definitely a bug in the code. So whether that gets fixed or there's better docs or better error messages around what support is I don't mind.

@droyad droyad transferred this issue from DbUp/DbUp Jan 30, 2024
@jeremysimmons
Copy link

Received message

Message: 25001: CREATE INDEX CONCURRENTLY cannot be executed within a pipeline

This worked for me using ; on it's own line to separate CREATE INDEX CONCURRENTLY statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

3 participants