-
Notifications
You must be signed in to change notification settings - Fork 457
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
Supports remote backup push from PostgreSQL 15 or later (#1385) #1595
base: master
Are you sure you want to change the base?
Conversation
* In the streamFromPostgres method, CopyData case adds data receiving processing from pgsql version 15 or later. * In the streamFromPostgres method, add NoticeResponse case. * Troubleshoot EOF problems caused by the last zeroBlock message sent from pgsql15 or later. * Add test for Supports remote backup push from PostgreSQL 15 or later Signed-off-by: tangtx <tianxitang@harmonycloud.cn>
Can the code owner help me review the code? THX! |
What needs to be done to get this reviewed and merged? We would like to use the remote backup feature for our PG15+ clusters, and would prefer not to maintain/run a fork. |
@debebantur Can you please take a look on the code? Architecturally the feature seem important. |
I agree with @x4m . The latest wal-g is no longer applicable to the remote backup capability of pgsql above version 15. Please take a look! @Aevin1387 |
Hello, @tangtx3 ! There were some changes in wal-g since pr was last updated. For example some settings moved from internal to internal/config. Could you, please, rebase pr. |
userData, err := internal.UnmarshalSentinelUserData(userDataRaw) | ||
tracelog.ErrorLogger.FatalfOnError("Failed to unmarshal the provided UserData: %s", err) | ||
|
||
folder, err := internal.ConfigureFolder() |
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.
Seems, that this err value is unused
fullBackup, storeAllCorruptBlocks || viper.GetBool(internal.StoreAllCorruptBlocksSetting), | ||
tarBallComposerType, NewRegularDeltaBackupConfigurator(deltaBaseSelector), | ||
userData, withoutFilesMetadata) | ||
|
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.
And here err is not checked too
@sebasmannem Hi Sebas! |
@x4m @tangtx3 @sebasmannem I started looking into bringing this up to date and fixing the test. Unfortunately, this particular test requires PG actually be running, so it's better to cover with this integration test, but that requires either updating the docker image to PG15+ and fixing all the tests for the changes between PG10 and PG15 (or later), or splitting the tests, either of which is quite complicated. |
ugh... we definitely should upgrade from PG10... maybe gradually, PG11, PG12, PG13 etc. |
Database name
PostgreSQL
Pull request description
Supports remote backup push from PostgreSQL 15 or later (#1385)
Describe what this PR fix
Please provide steps to reproduce (if it's a bug)
wal-g backup-push
ERROR: 2023/11/16 10:37:36.274535 unexpected response: &{%!t(string=ERROR) %!t(string=ERROR) %!t(string=42601) %!t(string=syntax error) %!t(string=) %!t(string=) %!t(int32=0) %!t(int32=0) %!t(string=) %!t(string=) %!t(string=) %!t(string=) %!t(string=) %!t(string=) %!t(string=) %!t(string=repl_scanner.l) %!t(int32=247) %!t(string=replication_yyerror) map[]}
Please add config and wal-g stdout/stderr logs for debug purpose
test case!
AWS_ACCESS_KEY_ID=minio;
AWS_ENDPOINT=http://127.0.0.1:32009;
AWS_S3_FORCE_PATH_STYLE=true;
AWS_SECRET_ACCESS_KEY=123456;
PGHOST=127.0.0.1;
PGPASSWORD=123456;
PGPORT=5432;
PGUSER=postgres;
WALE_S3_PREFIX=s3://test-pg/pg15-1116