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.

Sign up for GitHub

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

Jump to bottom

Fix overlapping index checking #117

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix overlapping index checking #117

wants to merge 2 commits into from

Conversation

Raveline
Copy link
Contributor

@Raveline Raveline commented Oct 9, 2024

  • Ensure we check only overlap on the current table and not the whole schema each time we check a table;
  • Exclude local indexes;
  • Make sure PK are not taken into account;
  • Fix test (it was passing for the wrong reasons);

@Raveline
Fix overlapping index checking
e0e49a6 
- Ensure we check only overlap on the current table
- Exclude local indexes
- Make sure PK are not taken into account
Raveline
Raveline commented Oct 9, 2024
View reviewed changes
src/Database/PostgreSQL/PQTypes/Checks/Util.hs
, " indisunique"
, " AND other_indisunique"
, " AND colindex = colotherindex"
, ");"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marco44 : OK for you ? (Could you please confirm l. 227 and l. 205 seem ok ?)

Copy link

Choose a reason for hiding this comment

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

Seems ok to me

marco44
marco44 approved these changes Oct 9, 2024
View reviewed changes
src/Database/PostgreSQL/PQTypes/Checks/Util.hs
, " indisunique"
, " AND other_indisunique"
, " AND colindex = colotherindex"
, ");"
Copy link

Choose a reason for hiding this comment

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

Seems ok to me

arybczak
arybczak reviewed Oct 10, 2024
View reviewed changes
src/Database/PostgreSQL/PQTypes/Checks/Util.hs Outdated
@@ -190,8 +190,8 @@ objectHasMore otype ptype extra =
arrListTable :: RawSQL () -> Text
arrListTable tableName = " ->" <+> unRawSQL tableName <> ": "

checkOverlappingIndexesQuery :: SQL
checkOverlappingIndexesQuery =
checkOverlappingIndexesQuery :: SQL -> SQL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
checkOverlappingIndexesQuery :: SQL -> SQL
checkOverlappingIndexesQuery :: RawSQL () -> SQL

src/Database/PostgreSQL/PQTypes/Checks.hs Outdated
@@ -508,7 +508,7 @@ checkDBStructure options tables = fmap mconcat . forM tables $ \(table, version)
runQuery_ $ sqlGetForeignKeys table
fkeys <- fetchMany fetchForeignKey
triggers <- getDBTriggers tblName
checkedOverlaps <- checkOverlappingIndexes
checkedOverlaps <- checkOverlappingIndexes (mkSQL . T.pack $ tblNameString table)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
checkedOverlaps <- checkOverlappingIndexes (mkSQL . T.pack $ tblNameString table)
checkedOverlaps <- checkOverlappingIndexes (tblName table)

And pass RawSQL () around and inside checkOverlappingIndexesQuery you can use raw function to convert from RawSQL () to SQL. AFAIR this is the general theme of how using RawSQL in constructing SQLs throughout the library is done, so let's be consistent.

src/Database/PostgreSQL/PQTypes/Checks/Util.hs Outdated
@@ -201,7 +201,8 @@ checkOverlappingIndexesQuery =
, " , ((regexp_match(pg_get_indexdef(indexrelid)"
, " , 'WHERE (.*)$')))[1] AS preddef"
, " FROM pg_index"
, " WHERE indexprs IS NULL)"
, " WHERE indexprs IS NULL"
, " AND indrelid = '" <> tableName <> "'::regclass)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
, " AND indrelid = '" <> tableName <> "'::regclass)"
, " AND indrelid = '" <> raw tableName <> "'::regclass)"

src/Database/PostgreSQL/PQTypes/Checks.hs Outdated
@@ -714,8 +714,8 @@ checkDBStructure options tables = fmap mconcat . forM tables $ \(table, version)
\expected output into source code.)"
]

checkOverlappingIndexes :: MonadDB m => m ValidationResult
checkOverlappingIndexes =
checkOverlappingIndexes :: MonadDB m => SQL -> m ValidationResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
checkOverlappingIndexes :: MonadDB m => SQL -> m ValidationResult
checkOverlappingIndexes :: MonadDB m => RawSQL () -> m ValidationResult

@Raveline
Copy link
Contributor Author

This has been tested against https://github.com/scrive/kontrakcja/pull/7335 with no problems. Are we ok to merge @arybczak ?

arybczak
arybczak approved these changes Oct 21, 2024
View reviewed changes
Copy link
Collaborator

@arybczak arybczak left a comment

Choose a reason for hiding this comment

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

Let's merge, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco44 marco44 marco44 approved these changes

@arybczak arybczak arybczak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Raveline @marco44 @arybczak