borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
12
65
Fork 17
Code Issues 48 Pull Requests 2 Actions Packages Releases 135 Activity

Fix multiple bugs in PostgreSQL hook #677

Merged
witten merged 6 commits from :fix-postgres-hook into main 2023-04-21 04:05:22 +00:00
Conversation 7 Commits 6 Files Changed 2 +26 -15
jirutka commented 2023-04-14 14:21:00 +00:00
Contributor
Copy Link

See commit messages.

See commit messages.
jirutka added 1 commit 2023-04-14 14:21:00 +00:00
diivi commented 2023-04-14 14:28:35 +00:00
Collaborator
Copy Link

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.
jirutka commented 2023-04-14 14:33:39 +00:00
Author
Contributor
Copy Link

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
witten commented 2023-04-14 22:59:39 +00:00
Owner
Copy Link

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
Copy Link

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!
borgmatic/hooks/postgresql.py
@ -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')
witten commented 2023-04-14 22:47:53 +00:00
Owner
Copy Link

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

Nice.. I didn't know about `shlex.split()`!
jirutka marked this conversation as resolved
borgmatic/hooks/postgresql.py
@ -62,2 +64,3 @@
list_command = (
('psql', '--list', '--no-password', '--csv', '--tuples-only')
tuple(psql_command)
+ ('--list', '--no-password', '--no-psqlrc', '--csv', '--tuples-only')
witten commented 2023-04-14 23:04:02 +00:00
Owner
Copy Link

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.
jirutka commented 2023-04-17 11:53:52 +00:00
Author
Contributor
Copy Link

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`.
jirutka commented 2023-04-17 12:03:22 +00:00
Author
Contributor
Copy Link

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.
witten commented 2023-04-17 17:10:46 +00:00
Owner
Copy Link

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
witten commented 2023-04-21 17:48:48 +00:00
Owner
Copy Link

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.
Reviewers
No Reviewers
witten
Labels
Clear labels
blocked
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#677
No description provided.
Delete Branch ":fix-postgres-hook"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?