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

Restore compatibility with SQLServer 2008 R2 in migrations #16627

Merged
merged 6 commits into from
Aug 8, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Aug 5, 2021

This fixes two problems with MSSQL:

  • ALTER TABLE DROP ... IF EXISTS ... is only supported in SQL Server >16.

The IF EXISTS here is a belt-and-braces and does not need to be present. Therefore
can be dropped. Also stop attempting to drop the indexes as constraints as they're indexes!

  • System tables like: sys.indexes should be lowercase not uppercase because of collation issues.

Fix #16625
Fix #13615
Fix #16483

Signed-off-by: Andrew Thornton art27@cantab.net

`ALTER TABLE DROP ... IF EXISTS ...` is only supported in SQL Server >16.

The `IF EXISTS` here is a belt-and-braces and does not need to be present. Therefore
can be dropped.

We need to figure out some way of restricting our SQL syntax against the minimum
version of SQL Server we will support.

My suspicion is that `ALTER DATABASE database_name SET COMPATIBILITY_LEVEL = 100` may
do that but there may be other side-effects so I am not whether to do that.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@lafriks
Copy link
Member

lafriks commented Aug 5, 2021

This seems to break migrations

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 5, 2021
@silverwind
Copy link
Member

We need to figure out some way of restricting our SQL syntax against the minimum version of SQL Server we will support.

Should probably add that minimum version on CI and then run all migrations/tests against it, that should lead to CI failures in case of incompatible syntax. 2012 is the oldest version supported by MS, but I think only 2017 and above are available for Linux so we might need to set minimum version to 2017 to be able to test it.

@zeripath
Copy link
Contributor Author

zeripath commented Aug 5, 2021

yup it does because it seems that we will need to check if the thing is a constraint or an index.

Damn.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #16627 (a0a5cd5) into main (59e6db0) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #16627   +/-   ##
=======================================
  Coverage   45.39%   45.40%           
=======================================
  Files         756      756           
  Lines       85279    85279           
=======================================
+ Hits        38713    38717    +4     
+ Misses      40306    40303    -3     
+ Partials     6260     6259    -1     
Impacted Files Coverage Δ
services/pull/temp_repo.go 30.76% <0.00%> (-3.85%) ⬇️
services/pull/check.go 25.51% <0.00%> (-2.76%) ⬇️
modules/process/manager.go 72.83% <0.00%> (-2.47%) ⬇️
services/pull/patch.go 54.23% <0.00%> (-1.70%) ⬇️
modules/queue/unique_queue_disk_channel.go 47.26% <0.00%> (-1.37%) ⬇️
models/repo_list.go 76.86% <0.00%> (-0.79%) ⬇️
services/pull/pull.go 41.73% <0.00%> (-0.41%) ⬇️
routers/api/v1/repo/pull.go 29.98% <0.00%> (+0.51%) ⬆️
routers/web/repo/view.go 41.52% <0.00%> (+0.57%) ⬆️
modules/queue/workerpool.go 56.87% <0.00%> (+1.90%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07bc380...a0a5cd5. Read the comment docs.

@zeripath
Copy link
Contributor Author

zeripath commented Aug 5, 2021

Ah that seems to be now working

@lunny
Copy link
Member

lunny commented Aug 5, 2021

You can check if the index is exist at first.

@zeripath
Copy link
Contributor Author

zeripath commented Aug 5, 2021

Line 854 only selects the indexes that exist. The IF EXIST is belt and braces and unnecessary. The DROP CONSTRAINT code was only there because of an earlier problem - solved by lines 843-853.

@zeripath zeripath added the backport/done All backports for this PR have been created label Aug 6, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 6, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 8, 2021
@lafriks lafriks merged commit 9c116f2 into go-gitea:main Aug 8, 2021
@zeripath zeripath deleted the fix-16625-drop-if-exists branch August 8, 2021 12:49
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
6 participants