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

change parameter_file Value to longtext #7392

Merged
merged 4 commits into from
Jun 7, 2021

Conversation

ycAbout
Copy link
Contributor

@ycAbout ycAbout commented Mar 15, 2021

Brief summary of changes

Add a sql patch to change parameter_file Value to longtext for upgrade.

Testing instructions (if applicable)

  1. On a upgrade instance (upgraded from v21), check parameter_file table Value is text datatype (see also 21.0_to_22.0 upgrade)
  2. Source mysql file on a upgrade instance (upgraded from v21)
  3. Verify parameter_file table Value is longtext datatype, and your existing data is OK.

Link(s) to related issue(s)

cc
@gdevenyi

Copy link
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

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

ALTER TABLE parameter_file MODIFY Value TEXT;

21.0_To_22.0_upgrade.sql has the issue, so it is better to modify the patch file directly.

@kongtiaowang kongtiaowang self-requested a review March 16, 2021 14:02
Copy link
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

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

need works

@driusan
Copy link
Collaborator

driusan commented Mar 16, 2021

Why is this being changed?

@ycAbout
Copy link
Contributor Author

ycAbout commented Mar 16, 2021

ALTER TABLE parameter_file MODIFY Value TEXT;

OK I see your point. @kongtiaowang Your point would help the the following 2. I can add it later today.

  1. For existing instance upgraded from v21 already, when they upgrade to v24, this new patch will do the work.
  2. For new instance that is going to upgrade from v21 to v22 or v23 (but not v24), change 21.0_To_22.0_upgrade.sql would prevent the issue if they download the v24 (this patch goes into v24), also the new patch won't do any harm if they later upgrade to v24.

@ycAbout
Copy link
Contributor Author

ycAbout commented Mar 16, 2021

Why is this being changed?

Hi @driusan Please specifically see LORIS_MRI issue# 589, where I investigated the history of this issue.
Resolves #7391
Also resolves LORIS_MRI issue# 589

@ycAbout ycAbout requested a review from kongtiaowang March 16, 2021 19:26
@@ -35,7 +35,7 @@ SELECT 'Running: SQL/Archive/22.0/2019-07-04-remove_header_row_from_parameter_fi

DELETE FROM parameter_file WHERE ParameterTypeID=(SELECT ParameterTypeID FROM parameter_type WHERE Name='header' AND SourceFrom='parameter_file');
DELETE FROM parameter_type WHERE Name='header' AND SourceFrom='parameter_file';
ALTER TABLE parameter_file MODIFY Value TEXT;

Copy link
Contributor

@kongtiaowang kongtiaowang Mar 17, 2021

Choose a reason for hiding this comment

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

Hi @ycAbout SQL/New_patches/2021-03-15_change_parameter_file_Value_longtext.sql is good.
But "ALTER TABLE parameter_file MODIFY Value LONGTEXT;" should be here.
Just running one release patch file once,if user only wants to update Loris from 21 to 22.
release patch needs a new patch to update, that doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kongtiaowang , I see what you mean. Without this line, user doesn't need to apply a v24 release patch to upgrade to v22. The reason to remove this line is that v18 already has this line handled. On the other hand, do the way you suggested won't do much harm. I can change it.

@ycAbout ycAbout requested a review from kongtiaowang March 17, 2021 15:03
Copy link
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

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

LGTM

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Mar 18, 2021
Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

This should not modify the release patch for a release that already exists.

@ycAbout
Copy link
Contributor Author

ycAbout commented May 3, 2021

This should not modify the release patch for a release that already exists.

OK,
It seems that both @kongtiaowang and @driusan have a point. Given that Dave is more senior, I think we should follow @driusan, what do you think, @kongtiaowang ?

@ycAbout
Copy link
Contributor Author

ycAbout commented Jun 7, 2021

The change has been reverted, so there is no modification of released patches. @driusan

@driusan driusan merged commit 6859c08 into aces:main Jun 7, 2021
@ycAbout ycAbout deleted the fixMriParameter_fileLongtext branch June 9, 2021 18:36
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
Change parameter_file Value to longtext. This was previously modified in the schema but the patch for existing projects was missed.
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SQL] parameter_file table inconsistency between new install and upgrade
4 participants