Fix multiple bugs in PostgreSQL hook #677

jirutka · 2023-04-14T14:21:00Z

jirutka commented

2023-04-14 14:21:00 +00:00

See commit messages.

jirutka added 1 commit 2023-04-14 14:21:00 +00:00

Fix PostgreSQL hook not using "psql_command" for list when dumping "all" 874fba7672

diivi commented

2023-04-14 14:28:35 +00:00

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

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 referenced this pull request

2023-04-14 22:58:16 +00:00

Multiple bugs in PostgreSQL hook #678

witten commented

2023-04-14 22:59:39 +00:00

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

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

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

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

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

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

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 referenced this issue from a commit

2023-04-21 04:05:23 +00:00

Fix multiple bugs in PostgreSQL hook (#678).

witten commented

2023-04-21 17:48:48 +00:00

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.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#677