Bug #19698
closed
save_with_unique_name hides other transaction errors
Description
save_with_unique_name works like this:
conn.exec_query 'SAVEPOINT save_with_unique_name'
begin
save!
rescue ActiveRecord::RecordNotUnique => rn
# tries different names, uses
# "ROLLBACK TO SAVEPOINT"
# to recover from transaction errors
ensure
conn.exec_query 'RELEASE SAVEPOINT save_with_unique_name'
end
It seems that if some other transaction error happens, then
- transaction is aborted
- the "ensure" block runs "RELEASE SAVEPOINT"
- because the transaction is aborted, it rejects "RELEASE SAVEPOINT"
- it raises a new error about not being able to accept "RELEASE SAVEPOINT" instead of propagating the original error
The result is a cryptic error about ignoring commands in a failed transaction and not the original error that gives some indication of what was wrong.
I need to read up on savepoints, but it may be that there's no real benefit to releasing the savepoint here, presumably it will be released when the transaction is closed either way.
Updated by
Updated by Tom Clegg over 1 year ago
19698-masked-error @ 924f8f6c13c06afc8a83168929b249e0e8fa7d18 -- developer-run-tests: #3370
Updated by Brett Smith over 1 year ago
Tom Clegg wrote in #note-5:
19698-masked-error @ 924f8f6c13c06afc8a83168929b249e0e8fa7d18 -- developer-run-tests: #3370
This is good to merge. I confirmed that the tests fail if you revert the code change.
I did a little reading about savepoints to try to make sure I was following things correctly. I agree with Peter that it seems like they should automatically be released once the transaction is committed, since they're only allowed to exist within a transaction . But I admit I couldn't find that explicitly documented.
I did note that savepoints still exist after you rollback to them
The savepoint remains valid and can be rolled back to again later, if needed.
Because of that, I think that the SAVEPOINT command inside the ensure block is redundant—although I'm not totally confident that Rails isn't doing some database work behind the scenes in the intervening code. But if it's not, then creating the later savepoints is technically a waste of resources, since PostgreSQL maintains a stack of savepoints with the same name. Not enough to be a big deal, since this is already a corner case and the code won't make more than three of them, but still.
All this means, I think we could delete both the RELEASE SAVEPOINT and later SAVEPOINT command, and it would be less code and work slightly better. I tried this on my checkout and confirmed the tests all pass with this change, including the new tests in this branch. If you agree, I think it would be a nice code cleanup while we have this information front of mind. But, it's not important enough to block the merge.
Updated by Tom Clegg over 1 year ago
Good point. I deleted the "create redundant identical savepoint" line in the retry block.
I think it's worth releasing the savepoint when succeeding, even though it will be automatically released when the transaction ends, for the sake of- clarifying/ensuring the savepoint doesn't affect anything later at spooky distance -- even tests that do more than the usual amount of stuff in a transaction, container-finalization hooks that can create a number of collections in a single transaction, etc.
- giving postgresql more information that can help it optimize things -- what with collection versioning, audit logs, and container finalizing, there can be non-trivial amount of work left in the transaction at this point, so the resources needed to maintain a usable savepoint could actually be worth releasing.
Merged main to make run-tests.sh pass sanity checks on new jenkins image without Perl packages.
19698-masked-error @ 5a33764685ef7fa9578b255e78ad2ed77abc1ded -- developer-run-tests: #3388
Updated by Brett Smith over 1 year ago
Tom Clegg wrote in #note-7:
19698-masked-error @ 5a33764685ef7fa9578b255e78ad2ed77abc1ded -- developer-run-tests: #3388
Everything you said makes sense and this looks good to me. Thanks.
Updated by Tom Clegg over 1 year ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|2c2a0b36cd0ec30d755ac2b4dd4f01e67ba058f9.