Add config flag to skip cleanup

[jdpedrie]

I've found the cleanup step to cause more issues than any other part of this (extremely useful!) library. We use it in combination with a testcontainer instance of postgres, so it's not strictly necessary for us, as the cleanup is handled by tearing down the docker container.

This PR adds a flag to skip the teardown part of the cleanup step. I wasn't quite sure how or if it was testable, so I didn't add a test. I'd be happy to add one if a) you're open to merging this, and b) a test is desired and you could give me a tip on what I should do!

Thanks for making this and reviewing the PR.

[peterldowns]

@jdpedrie stoked to hear you're using pgtestdb and find it useful! I'll take a look through the code within a day or two, but at first glance this looks very merge-able. I'll try to think of an easy and reasonable way to add a test, and if I can't come up with one we shouldn't let that block. Thank you for the contribution!

While you're thinking about pgtestdb, if there is anything you like or dislike, or any other wishlist items on your mind, I'd love to hear them.

Cheers, Peter

[jdpedrie]

The issue I wasn't able to solve which led to this PR was the following. I had some other issues which were related to not closing resources in the right order. This seems like the same, but I've not been able to work it out.

testdb.go:244: could not drop test database 'testdb_tpl_f991bf6e77c90a59a1cd8b54746c52d6_inst_27549a95d41d8cd98f00b204e9800998ecf8427e': pq: database "testdb_tpl_f991bf6e77c90a59a1cd8b54746c52d6_inst_27549a95d41d8c" is being accessed by other users

Maybe a better solution than mine would be to terminate connections before dropping the database?

SELECT pg_terminate_backend(pg_stat_activity.pid)
FROM pg_stat_activity
WHERE pg_stat_activity.datname = 'testdb_tpl_f991bf6e77c90a59a1cd8b54746c52d6_inst_27549a95d41d8cd98f00b204e9800998ecf8427e'
  AND pid <> pg_backend_pid();

I read your note regarding test container in the README when I found the project on HN, but I had already been using them in other contexts inside tests without issues, so I followed the same pattern with postgres without much trouble.

For what it's worth, I did take some time this morning to switch over to a separate container in Github Actions and local testing, and I'll likely stick with that going forward. It didn't resolve the above issue though.

If I stick with the external container, would definitely not want to use the new flag I added. If I keep using test containers, it's less of a concern since everything got cleaned up when I tore down the container.

[jdpedrie]

Maybe a better solution than mine would be to terminate connections before dropping the database?

I tried that out, and it seemed to work. With the default role, it resolved the error and completed successfully.

https://github.com/jdpedrie/pgtestdb/commit/c8c95a775ea9af1a48c0dd4825fa68588f53e852

[peterldowns]

@jdpedrie sorry for the radio silence, I've been busy working on other projects, but I'd like to revisit this. I think I understand what you've written above, but could you let me know — what's the next step to getting a solution merged? Do you need me to choose between the current approach and the changes you made in https://github.com/jdpedrie/pgtestdb/commit/c8c95a775ea9af1a48c0dd4825fa68588f53e852 ?

Thank you for your patience!

[jdpedrie]

hey @peterldowns no problem. I've been running my branch in CI since we last spoke with the suggested change to terminate connections before dropping the database, and haven't seen any flakiness. If that makes sense to you (as you have more context on the particulars I might not be aware of), I'll just update to do only that instead of the config changes.

I did also switch to an outside docker container for running tests.

[peterldowns]

@jdpedrie that's great news. OK, I would happily merge a PR that adds a ForceTerminateConnections flag, defaulting to false. If set, pgtestdb should call pg_terminate_backend the way you've implemented before running DROP DATABASE. Ideally, if there is an error calling DROP DATABASE due to existing connections, we would wrap that and include a suggestion to set ForceTerminateConnections to point users to this feature.

Spelled it out to make sure we're on the same page, but basically send me something like the above and I'll get it merged. Thanks again for using the library, writing in about the issue, and iterating on the solution with me 🫡

[jdpedrie]

Replaced by #13.