-
Notifications
You must be signed in to change notification settings - Fork 11
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
Upgrade to PG17 in Github actions #2114
Conversation
…saction_timeout = 0 setting
…the pg_dump/pg_restore as per version
.github/workflows/pg-17-migtests.yml
Outdated
yes | ./install-yb-voyager --install-from-local-source --only-pg-support | ||
sudo apt-get -y install postgresql-17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we start shipping PG 17 via installer script now?
Because
If we are gonna detect till PG17, then installing older version during setup is of no use.
It will be a manual step for the user unnecessarily.
Lets discuss this in the standup tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is already in plan. I believe there is already ticket for that https://yugabyte.atlassian.net/browse/DB-14593
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, just add a TODO here saying that we need to remove this once installer script picks up 17
stmt = strings.ToUpper(stmt) | ||
|
||
// pg_dump generate `SET client_min_messages = 'warning';`, but we want to get | ||
// NOTICE severity as well (which is the default), hence skipping this. | ||
if strings.Contains(stmt, "SET CLIENT_MIN_MESSAGES") { | ||
//pg_dump 17 gives this SET transaction_timeout = 0; | ||
if strings.Contains(stmt, "SET CLIENT_MIN_MESSAGES") || strings.Contains(stmt, "SET TRANSACTION_TIMEOUT") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constants ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
.github/workflows/pg-17-migtests.yml
Outdated
@@ -108,27 +110,27 @@ jobs: | |||
psql "postgresql://yugabyte@yb-tserver-n1:5433/yugabyte" -c "SELECT version();" | |||
|
|||
- name: "TEST: PG sample schemas (sakila)" | |||
if: ${{ !cancelled() && matrix.test_group == 'offline' }} | |||
if: ${{ !cancelled() && matrix.test_group == 'offline'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: be consistent in formatting here(wrt the spaces before/after braces used):
${{ !cancelled() && matrix.test_group == 'offline' }}
or
${{!cancelled() && matrix.test_group == 'offline'}}
Describe the changes in this pull request
PG 17 upgrade, Major differences -
- Validation fix only
- Going in Uncategorized.sql, must be considered while extracting the schema from pg_dump.
- We must skip setting this as it is not supported in YB yet.
Fixed Validation files for the VIEW/MVIEW's select statements
Skipping the
SET transaction_timeout = 0;
in the import schema to be not executed on YB.Fixes #2121
Describe if there are any user-facing changes
No
How was this pull request tested?
https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline-test/1073
https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline-test/1074
Does your PR have changes that can cause upgrade issues?