Project

General

Profile

Actions

Bug #19698

closed

save_with_unique_name hides other transaction errors

Added by Peter Amstutz 3 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
11/15/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

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

  1. transaction is aborted
  2. the "ensure" block runs "RELEASE SAVEPOINT"
  3. because the transaction is aborted, it rejects "RELEASE SAVEPOINT"
  4. 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.


Subtasks 1 (0 open1 closed)

Task #19723: Review 19698-masked-errorResolvedTom Clegg11/15/2022

Actions
Actions #1

Updated by Peter Amstutz 3 months ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz 3 months ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz 3 months ago

  • Assigned To set to Tom Clegg
Actions #4

Updated by Tom Clegg 3 months ago

  • Status changed from New to In Progress
Actions #6

Updated by Brett Smith 2 months 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.

Actions #7

Updated by Tom Clegg 2 months 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

Actions #8

Updated by Brett Smith 2 months 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.

Actions #9

Updated by Tom Clegg 2 months ago

  • Status changed from In Progress to Resolved
Actions #10

Updated by Peter Amstutz about 2 months ago

  • Release set to 47
Actions

Also available in: Atom PDF