-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add ability to specify custom schema for verification in postgres #139
Conversation
Signed-off-by: Emiliano Suñé <[email protected]>
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.
Looking good so far. A few comments and recommendations ...
The TABLE_SCHEMA
variable has been used to this point to define the schema for verification. It's used by backup.mariadb.plugin
and backup.postgres.plugin
. The downside - It's global, hance this PR.
I think the TABLE_SCHEMA
variable should be deprecated in favor of this new approach, where the new approach would contextually override the value in TABLE_SCHEMA
.
Being able to backup a specific schema is an interesting addition, but I'm not sure it should be the default. The way it has worked up to now is the backup is a full backup, and then the verification verifies the schema of interest. This way when you restore the database, you're restoring a snapshot of everything, not just a single schema.
I also see that the new schema configuration setting is not being used for verification, the verification process is still only using the TABLE_SCHEMA
variable.
…ema option Signed-off-by: Emiliano Suñé <[email protected]>
Implementation was updated to support
Pondering whether to use More testing currently underway. |
I personally prefer the explicit |
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 pending the results of further testing.
Signed-off-by: Emiliano Suñé <[email protected]>
Signed-off-by: Emiliano Suñé <[email protected]>
I tweaked the This results in successful backups, however I am getting errors with verification when executed manually for any of the backed-up databases (usually an error around the user already existing). |
Signed-off-by: Emiliano Suñé <[email protected]>
See #132 (comment) for info about the verification failures. Leaving this PR as draft until I can understand more about how to fix the issue. |
Signed-off-by: Emiliano Suñé <[email protected]>
Finally ready for review. I added code to post-process the roles dump and remove the default users (the user running the command as well as the postgres user) and verification is now successful. If any other users are in the roles dump, they will be retained. Unfortunately it is not (easily) feasible to take old backups generated with versions |
Signed-off-by: Emiliano Suñé <[email protected]>
Removed |
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
Added the ability to specify a custom schema name for backup verification in postgres when using the configuration file spec.
Following the convention used for JDBC, the schema can be specified by adding
?verifySchema=my_schema
to the database spec.This was developed as a set of postgres-specific overrides since other supported database providers do not have the concept of schema and it was simpler to customize one instance of the function to grab the database name from the connection spec rather than handling the different scenarios in the same function, but it can be refactored if necessary.
One thing this pattern could also be used for in the future is to specify the auth database for
mongodb
in the spec rather than using an environment variable like it is currently set-up to do.Opening in draft mode while testing.