Remove SQL Server upgrade scripts and code - #307
Conversation
|
@bbimber most of this is straightforward and mechanical, but please see my notes about the CLR. No rush to review. I restored the "include" upon review since it was part of the index name (if not actually impacting the index itself on PG) |
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
Looks good to me. Just a couple small suggestions.
Not sure why we don't have EHR test check on the PRs in this repo.
| @@ -536,54 +524,12 @@ public void exec(ResultSet object) throws SQLException | |||
|
|
|||
| private void getDBSize(Container c, User u, final StringBuilder msg, final StringBuilder alerts, Map<String, String> saved, Map<String, String> toSave) | |||
There was a problem hiding this comment.
Looks like toSave parameter is no longer used.
There was a problem hiding this comment.
Many unused parameters in this file. I pruned most and deleted some unused methods too. Don't want to go overboard on unrelated code cleanup.
There was a problem hiding this comment.
I will make a note to look at this later. A lot of this code is quite old. if i had time some day i'd like to go through a cull a lot of things...
| } | ||
|
|
||
| if (!ti.getSqlDialect().isPostgreSQL() && !ti.getSqlDialect().isSqlServer()) | ||
| if (!ti.getSqlDialect().isPostgreSQL()) |
There was a problem hiding this comment.
Do we need this at all? Doesn't seem consistent with other table customizers.
There was a problem hiding this comment.
I think it's extremely unlikely this would ever run on another external schema (mysql / oracle (if you support that anymore); however, it's it theoretically possible?
There was a problem hiding this comment.
Probably not but it's possible this is configured against external DBs somewhere. I didn't want to chase that down so I left it in.
|
|
||
| msg.append("<br><b>The top 20 largest tables, by row count:</b><br><br>"); | ||
| msg.append("<table border=1 style='border-collapse: collapse;'><tr style='font-weight:bold;'><td>Schema</td><td>Table</td><td># of Rows</td><td>Previous Value</td><td>% Change</td></tr>"); | ||
| msg.append("<br><b>The top 20 largest tables, by row count:</b><br><br>"); |
There was a problem hiding this comment.
@labkey-jeckels : you do happen to know if any of the changes LK added to address the AI security review are in develop, or are they in a branch? I dont recall if they touched this, but they did touch a few places to convert string HTML into DOM(). I bring this up to head off merge conflicts
There was a problem hiding this comment.
We're continuing to do scans and address the findings based on severity and choose the appropriate branch for the fix. That said, we should be current here and avoid conflicts since we have the regularly weekly merges to pull all changes from release branches forward to develop.
I saw that. These indexes are one very old feature that should be reviewed. I'm not sure I want to carry them forward into the postgres migration. PRIMe is where more of these things could potentially matter (a more DB-intensive application), then my other servers. Your changes are probably the right thing. |
Thanks for taking a look. Yeah, I'd like to assess the indices based on real query perf in Postgres so I'm trying not to get too carried away with changes as part of bulk refactors like this one. A lot has changed since they were added. |
|
@labkey-jeckels, yes, most of the custom index stuff in LDK and EHR dates back to ~2013-2014-ish, when PRIMe was having substantial issues. With hindsight, I would address it differently, but some of those ideas are also non-trivial changes (like maintaining a cached table with some of the common values currently calculated on-the-fly in SQL). That's not something that can happen here. |
3 participants
Rationale
We no longer support SQLServer as a primary DB
Changes
SupportedDatabasesinmodule.propertiesPostgresOnlyTest