powa-team / powa-archivist

powa-archivist: the powa PostgreSQL extension
http://powa.readthedocs.io/
PostgreSQL License
51 stars 20 forks source link

Narrow the error condition in powa_prevent_concurrent_snapshot() #38

Closed dlax closed 3 years ago

dlax commented 3 years ago

We use 'lock_not_available' error condition in powa_prevent_concurrent_snapshot() in order to only warn about possible concurrent snapshot when this is true. (For instance, when default transaction mode is set to read-only in configuration, we'd previously get an invalid error message.)

rjuju commented 3 years ago

Hello Denis,

Thanks a lot for the PR!

So while I agree that we enhance the error handling in that function, I think we should still have a dedicated human-readable powa-specific error message for cases different from lock_not_available, something along the line 'An error happened while trying to lock...', and then append the stack diagnostics information (see e.g. https://github.com/powa-team/powa-archivist/blob/master/powa--4.1.2.sql#L2078 for a template on how to add those details).

Note that in case of read-only transaction a lot of other thing would probably go wrong, but I'm assuming that it's only a side effect you noticed with the patch you're working on for the pg_upgrade bug and not a real use case.

Now, for extension specific infrastructure, we don't back-patch any change in older versions of the extension. Since v4.1.2 was already released, what needs to be done here is to start a 4.1.3 by duplicating the powa--4.1.2.sql script, adding an empty powa--4.1.2--4.1.3.sql and then adding changes in both files. I can take care of that part if you prefer, but in any case it should be a different commit to ease history digging.

Finally, please also add your name in the contributor file :)

dlax commented 3 years ago

Hello Julien,

Thanks for the quick reply and guidance. I think I've made the necessary changes in commits I just pushed. Let me know there's anything else.

rjuju commented 3 years ago

Thanks Denis!

It's perfect, and I confirm that everything is working as intended when simulating the 2 error paths locally. 👍