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

Unique key violation on changes.script_hash not handled #630

Closed
maudrid opened this issue May 17, 2022 · 10 comments · Fixed by #764
Closed

Unique key violation on changes.script_hash not handled #630

maudrid opened this issue May 17, 2022 · 10 comments · Fixed by #764
Assignees

Comments

@maudrid
Copy link

maudrid commented May 17, 2022

Hi.
Having an issue when I try to deploy and I am able to recreate it on all our environments.
When I run sqitch deploy I get this error in the database:

2022-05-17 07:12:38.045 UTC [172] ERROR:  duplicate key value violates unique constraint "tags_project_tag_key"
2022-05-17 07:12:38.045 UTC [172] DETAIL:  Key (project, tag)=(affectli, @V1.6.0) already exists.
2022-05-17 07:12:38.045 UTC [172] STATEMENT:
                   INSERT INTO tags (
                          tag_id
                        , tag
                        , project
                        , change_id
                        , note
                        , committer_name
                        , committer_email
                        , planned_at
                        , planner_name
                        , planner_email
                   )
                   SELECT tid, tg, proj, chid, n, name, email, at, pname, pemail FROM ( VALUES
               ($1, $2, $3, $4, $5, $6, $7, $8::timestamptz, $9, $10)
                   ) i(tid, tg, proj, chid, n, name, email, at, pname, pemail)
                     LEFT JOIN tags ON i.tid = tags.tag_id
                    WHERE tags.tag_id IS NULL

The steps I followed to create this problem:

  1. Have some existing sqitch project
  2. Add a new tag
  3. Run sqitch deploy without adding any other changes
  4. Now do a sqitch rework
  5. Then do a sqitch deploy again:
$ sqitch deploy
ERROR:  duplicate key value violates unique constraint "tags_project_tag_key"
DETAIL:  Key (project, tag)=(myProject, @V1.6.0) already exists.

This is how I worked around this problem:

  1. After getting this error: sqitch revert @HEAD^
Revert changes to schemas/myProject/publications/iam from db:pg://postgres:@localhost:15432/myProject? [Yes]
  - schemas/myProject/tables/myTable @V1.6.0 .. ok
  1. Then Deploy again:
$ sqitch deploy
Deploying changes to db:pg://postgres:@localhost:15432/myProject
  + schemas/myProject/tables/myTable @V1.6.0 .. ok
  + schemas/myProject/tables/myTable .......... ok
@theory
Copy link
Collaborator

theory commented May 21, 2022

Odd, I am not able to replicate this issue. I

  1. Deployed a plan
  2. Added a tag
  3. deployed again (it says everything is up-to-date, but adds a tag record)
  4. Reworked the most recent change
  5. Deployed again and it deployed successfully

Out of curiosity, I diffed a dump of a database where the tag was deployed at the same time as the last change, and another where it was deployed after the last change, as in your example. The only difference was that the sqitch.events record for that last change did not have the tag. This is purely informational and for display, and clearly not related to the issue you're seeing.

Would you mind sharing an example of

  1. A plan without the tag
  2. The plan with the tag added
  3. The plan with the reworked change

@theory theory self-assigned this Jun 20, 2022
@theory theory added the waiting Waiting on feedback label Jun 20, 2022
@maudrid
Copy link
Author

maudrid commented Nov 29, 2022

Hi. Please see this project on how to recreate the issue:
https://github.com/maudrid/sqitch-bug-630
Read the code and then execute the test.sh to see the problem reproduced.

@theory theory removed the waiting Waiting on feedback label Nov 29, 2022
@theory
Copy link
Collaborator

theory commented Dec 10, 2022

Thanks for that. This is because there is no change to the deploy script. It works with this change:

--- a/test.sh
+++ b/test.sh
@@ -7,4 +7,5 @@ docker exec -ti -u999 sqitch-test bash -c 'cd /tmp/s630/ && sqitch tag myTag -m
 docker exec -ti -u999 sqitch-test bash -c 'cd /tmp/s630/ && sqitch deploy'
 docker exec -ti -u999 sqitch-test bash -c 'cd /tmp/s630/ && sqitch status'
 docker exec -ti -u999 sqitch-test bash -c 'cd /tmp/s630/ && sqitch rework test1 -m "reworking"'
+docker exec -ti -u999 sqitch-test bash -c 'cd /tmp/s630/ && echo >> deploy/test1.sql'
 docker exec -ti -u999 sqitch-test bash -c 'cd /tmp/s630/ && sqitch deploy'

This is on the assumption that you will change the deploy script — because otherwise why rework it?

@theory theory closed this as completed Dec 10, 2022
@theory
Copy link
Collaborator

theory commented Dec 10, 2022

Mean to say thanks a million for taking the time to build such a terrific test case. Made it easy for me to replicate and see what was going on --- even if I did first go down a rabbit hole to install v1.3.1 to make sure the issue remained (I think the apt repo installs v1.1.0).

@maudrid
Copy link
Author

maudrid commented Jan 16, 2023

So just for me to understand, the issue happens because the hash of the file did not change?
The new file is automatically created, but since the contents is the same, it is giving an issue?

@theory
Copy link
Collaborator

theory commented Jan 17, 2023

Right, it creates the script as a copy of the old one, but expects you to modify it — otherwise why bother reworking it if you don't rework it? :-)

@maudrid
Copy link
Author

maudrid commented Jan 26, 2023

Hi. I found another case that triggers the same error message:

  1. Create a plan that contains a tag.
  2. Deploy all changes
  3. Revert changes until just before the tag (so also revert the tag)
  4. Change the commit message of the tag
  5. Try to deploy all changes.
    Result is the same error message.
    If I then change the commit message back to the original commit message, I can deploy the changes again.
    Also, if I revert to the change that is two lines up from the tag, then the tag is actually reverted. Is this by design?

@theory
Copy link
Collaborator

theory commented Jan 28, 2023

I'm not able to replicate that issue, either :-( Here's what I did:

sqitch init s630 --engine pg --target postgres
sqitch add test1 -m "make a new version"
add test2 -m "make another change version"
sqitch deploy db:pg:try
sqitch status  db:pg:try
sqitch revert --to test1 db:pg:try
# edit sqitch.conf, append ` (edited)` to the `@myTag` line
sqitch deploy db:pg:try

@theory
Copy link
Collaborator

theory commented Jan 28, 2023

Re-opening, because I now realize that the raw SQL error is a bug. The code needs to catch unique key violations and return a more meaningful error.

@theory theory reopened this Jan 28, 2023
@theory theory changed the title Possible bug with tags when deploying Unique key violation on changes.script_hash not handled Jan 28, 2023
@maudrid
Copy link
Author

maudrid commented Jan 30, 2023

I will try to recreate this using the project submitted before, but I need a bit of time.

theory added a commit that referenced this issue Jun 24, 2023
Originally added in a04be11 and updated in 4d63e2c, the unique
violations have been working fine in most situations, but were confusing
when hit, as they returned as raw SQL errors. So modify
`log_deploy_change` to catch database exceptions, check for unique
constraint violations, and return the more useful error message `Cannot
log change "{change}": The deploy script is not unique`.

Requires adding a new method to each engine, `_unique_error`, which checks error
codes from the DBI to determine whether the most recent error was a
unique violation. The default implementation always returns false for
the database
engines that don't enforce unique constraints (Exasol and Snowflake).

Add a test to DBIEngineTest to trigger the unique violation. This
required fixing some tests that previously always passed when they
shouldn't (thanks to the use of `try` without `catch`) and fixing them
to testing things properly. Also requires a new parameter to
`DBIEngineTest->run()` to disable the unique constraint test for engines
that don't enforce that constraint (Exasol and Snowflake).

While at it, always enable primary key and unique constraints on
Vertica. They previously were enforced only by database-level
configuration, and unique constraints only on Vertica 7.2 and later with
that configuration. Enforcement will only be enabled by default for new
registry databases. Drop support for versions of Vertica prior to 7.2,
which don't support the syntax to enable constraints.

Also drop support for versions of SQLite prior to 3.8.6, when unique
constraint enforcement was added to SQLite.

Resolves #630.
theory added a commit that referenced this issue Jun 24, 2023
Originally added in a04be11 and updated in 4d63e2c, the unique
violations have been working fine in most situations, but were confusing
when hit, as they returned as raw SQL errors. So modify
`log_deploy_change` to catch database exceptions, check for unique
constraint violations, and return the more useful error message `Cannot
log change "{change}": The deploy script is not unique`.

Requires adding a new method to each engine, `_unique_error`, which checks error
codes from the DBI to determine whether the most recent error was a
unique violation. The default implementation always returns false for
the database
engines that don't enforce unique constraints (Exasol and Snowflake).

Add a test to DBIEngineTest to trigger the unique violation. This
required fixing some tests that previously always passed when they
shouldn't (thanks to the use of `try` without `catch`) and fixing them
to testing things properly. Also requires a new parameter to
`DBIEngineTest->run()` to disable the unique constraint test for engines
that don't enforce that constraint (Exasol and Snowflake).

While at it, always enable primary key and unique constraints on
Vertica. They previously were enforced only by database-level
configuration, and unique constraints only on Vertica 7.2 and later with
that configuration. Enforcement will only be enabled by default for new
registry databases. Drop support for versions of Vertica prior to 7.2,
which don't support the syntax to enable constraints.

Also drop support for versions of SQLite prior to 3.8.6, when unique
constraint enforcement was added to SQLite.

Resolves #630.
@theory theory closed this as completed in 9b4327a Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants