Fix multiple bugs in PostgreSQL hook #677

Merged
witten merged 6 commits from jirutka/borgmatic:fix-postgres-hook into main 2023-04-21 04:05:22 +00:00
Contributor

See commit messages.

See commit messages.
jirutka added 1 commit 2023-04-14 14:21:00 +00:00
Collaborator

I think there are issues open for some of these too, could you link to them in the description so it's easier to close them? Anyway, I'll try to do that in some time.

I think there are issues open for some of these too, could you link to them in the description so it's easier to close them? Anyway, I'll try to do that in some time.
Author
Contributor

I think there are issues open for some of these too

I haven’t found any in this repository.

> I think there are issues open for some of these too I haven’t found any in this repository.
jirutka force-pushed fix-postgres-hook from ab91570603 to c105bea740 2023-04-14 14:55:25 +00:00 Compare
jirutka force-pushed fix-postgres-hook from 589d98c156 to 17f122bfe5 2023-04-14 15:39:03 +00:00 Compare
Owner

Thank you for finding and fixing these issues! I filed a ticket with the issues you've identified (pretty much copy and pasted): #678 ... mostly so folks have a ticket to reference from the eventual changelog.

Thank you for finding and fixing these issues! I filed a ticket with the issues you've identified (pretty much copy and pasted): https://projects.torsion.org/borgmatic-collective/borgmatic/issues/678 ... mostly so folks have a ticket to reference from the eventual changelog.
witten requested changes 2023-04-14 23:16:34 +00:00
witten left a comment
Owner

This looks great! I appreciate you diving in and fixing these issues. The only thing potentially missing is adding (or updating) a test demonstrating that the psql_command and pg_restore_command options now support commands with arguments in these functions. Do you think that's something you could add? It wouldn't have to be a full end-to-end test that calls out to PostgreSQL commands or anything; a simple unit test that the right arguments get created would suffice. Thanks!

This looks great! I appreciate you diving in and fixing these issues. The only thing potentially missing is adding (or updating) a test demonstrating that the `psql_command` and `pg_restore_command` options now support commands with arguments in these functions. Do you think that's something you could add? It wouldn't have to be a full end-to-end test that calls out to PostgreSQL commands or anything; a simple unit test that the right arguments get created would suffice. Thanks!
@ -59,8 +60,10 @@ def database_names_to_dump(database, extra_environment, log_prefix, dry_run):
if dry_run:
return ()
psql_command = shlex.split(database.get('psql_command') or 'psql')
Owner

Nice.. I didn't know about shlex.split()!

Nice.. I didn't know about `shlex.split()`!
jirutka marked this conversation as resolved
@ -62,2 +64,3 @@
list_command = (
('psql', '--list', '--no-password', '--csv', '--tuples-only')
tuple(psql_command)
+ ('--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only')
Owner

The addition of --no-psqlrc sounds more correct, but I do worry a little that someone will be relying on their ~/.psqlrc to be read. I guess we'll find out.

The addition of `--no-psqlrc` sounds more correct, but I do worry a little that someone will be relying on their `~/.psqlrc` to be read. I guess we'll find out.
Author
Contributor

This is not a hypothetical situation, it happened to me… borgmatic failed because I have \pset linestyle unicode and \pset border 2 in .psqlrc.

This is not a hypothetical situation, it happened to me… borgmatic failed because I have `\pset linestyle unicode` and `\pset border 2` in `.psqlrc`.
Author
Contributor

The only thing potentially missing is adding (or updating) a test demonstrating that the psql_command and pg_restore_command options now support commands with arguments in these functions.

According to theconfiguration reference, it was already supposed to support it, but it didn't work:

# Command to use instead of "psql". This can be
# used to run a specific psql version (e.g.,
# one inside a running container). Defaults to
# "psql".
# psql_command: docker exec my_pg_container psql

Do you think that's something you could add?

I’m sorry, but I’ve already spent more time on it than I would have liked, I need to focus on other projects.

> The only thing potentially missing is adding (or updating) a test demonstrating that the psql_command and pg_restore_command options now support commands with arguments in these functions. According to the[configuration reference](https://torsion.org/borgmatic/docs/reference/configuration/), it was already supposed to support it, but it didn't work: ``` # Command to use instead of "psql". This can be # used to run a specific psql version (e.g., # one inside a running container). Defaults to # "psql". # psql_command: docker exec my_pg_container psql ``` > Do you think that's something you could add? I’m sorry, but I’ve already spent more time on it than I would have liked, I need to focus on other projects.
Owner

Okay, thanks anyway! I appreciate your contribution, and I'll take it from here.

Okay, thanks anyway! I appreciate your contribution, and I'll take it from here.
witten merged commit da0f5a34f2 into main 2023-04-21 04:05:22 +00:00
Owner

FYI I ended backing out this one change:

Exit on error when restoring all PostgreSQL databases

"--set ON_ERROR_STOP=on" is equivalent to "--exit-on-error" in
pg_restore.

The reason is that I couldn't get the end-to-end tests to pass with it in place, and I think those tests are probably pretty indicative of how real users will use dumps and restores. borgmatic PostgreSQL dumps are created with --clean --if-exists and so they include statements like DROP ROLE IF EXISTS postgres;. But then when the restore is attempted via psql --set ON_ERROR_STOP=on, it'll abort with: ERROR: current user cannot be dropped.

I don't know of a way around this without:

  • Omitting the --clean / --if-exists from the dump process.
  • Or requiring users to connect as a user other than the database owner.
  • Or editing the dump contents to omit the DROP ROLE IF EXISTS postgres;

... none of which I'm really excited about. I'm happy to hear about other options though. Thanks!

FYI I ended backing out this one change: > Exit on error when restoring all PostgreSQL databases > > "--set ON_ERROR_STOP=on" is equivalent to "--exit-on-error" in pg_restore. The reason is that I couldn't get the end-to-end tests to pass with it in place, and I think those tests are probably pretty indicative of how real users will use dumps and restores. borgmatic PostgreSQL dumps are created with `--clean` `--if-exists` and so they include statements like `DROP ROLE IF EXISTS postgres;`. But then when the restore is attempted via `psql --set ON_ERROR_STOP=on`, it'll abort with: `ERROR: current user cannot be dropped.` I don't know of a way around this without: * Omitting the `--clean` / `--if-exists` from the dump process. * Or requiring users to connect as a user other than the database owner. * Or [editing the dump contents](https://dba.stackexchange.com/questions/75033/how-to-restore-everything-including-postgres-role-from-pg-dumpall-backup) to omit the `DROP ROLE IF EXISTS postgres;` ... none of which I'm really excited about. I'm happy to hear about other options though. Thanks!
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#677
No description provided.