Skip to content

Conversation

@Riddlerrr
Copy link
Contributor

@Riddlerrr Riddlerrr commented Nov 26, 2025

This PR addresses concurrency issues where simultaneous updates to the same record could trigger a PG::ExclusionViolation due to overlapping validity ranges.

Key Changes & Technical Details:

Fix PG::ExclusionViolation:

Added explicit row locking (PERFORM 1 ... FOR UPDATE) in the UPDATE trigger. This serializes concurrent updates to the same row, preventing them from attempting to insert overlapping history ranges simultaneously.

After adding the lock we have the second issue with RangeError, we can fix it with slightly updated squashing logic.

Update Squashing Logic:
I updated the query that detects multiple updates for the same transaction and changed conditions from lower(validity) = _now to lower(validity) >= _now

This means that when the validity is already updated in a parallel transaction that runs at the same millisecond, we will squash the changes and not create a new historical record.

@Riddlerrr Riddlerrr force-pushed the bugfix/concurrency-update branch from d5aef09 to d1e362a Compare November 26, 2025 10:03
@Riddlerrr Riddlerrr marked this pull request as ready for review November 26, 2025 10:24
@tagliala
Copy link
Member

tagliala commented Nov 26, 2025

Any chance to run this spec in isolation?

$ be rspec spec/chrono_model/time_machine/concurrency_spec.rb 
Testing against Active Record 8.1.1 with Arel 10.0.0
2025-11-26 12:08:44.882 CET [46819] LOG:  checkpoint starting: immediate force wait
2025-11-26 12:08:44.887 CET [46819] LOG:  checkpoint complete: wrote 25 buffers (0.2%); 0 WAL file(s) added, 1 removed, 0 recycled; write=0.003 s, sync=0.001 s, total=0.006 s; sync files=11, longest=0.001 s, average=0.001 s; distance=5208 kB, estimate=5208 kB; lsn=9C/3B3B8888, redo lsn=9C/3B3B8850

Randomized with seed 32877

Concurrency
  concurrent deletions
    handles concurrent deletions without error (FAILED - 1)
  concurrent updates
    handles concurrent updates without PG::ExclusionViolation (FAILED - 2)

Failures:

  1) Concurrency concurrent deletions handles concurrent deletions without error
     Failure/Error: countries = Array.new(threads_count) { |i| Country.create!(name: "test-delete-#{i}") }
     
     NameError:
       uninitialized constant Country
     # ./spec/chrono_model/time_machine/concurrency_spec.rb:49:in 'block (4 levels) in <top (required)>'
     # ./spec/chrono_model/time_machine/concurrency_spec.rb:49:in 'Array#initialize'
     # ./spec/chrono_model/time_machine/concurrency_spec.rb:49:in 'Array.new'
     # ./spec/chrono_model/time_machine/concurrency_spec.rb:49:in 'block (3 levels) in <top (required)>'
     # /~.rvm/gems/ruby-3.4.7/gems/aruba-2.3.2/lib/aruba/rspec.rb:40:in 'block (2 levels) in <top (required)>'
     # /~/.rvm/gems/ruby-3.4.7/gems/aruba-2.3.2/lib/aruba/rspec.rb:27:in 'block (2 levels) in <top (required)>'

  2) Concurrency concurrent updates handles concurrent updates without PG::ExclusionViolation
     Failure/Error: Country.create!(name: 'test')
     
     NameError:
       uninitialized constant Country
     # ./spec/chrono_model/time_machine/concurrency_spec.rb:11:in 'block (3 levels) in <top (required)>'
     # /~/.rvm/gems/ruby-3.4.7/gems/aruba-2.3.2/lib/aruba/rspec.rb:40:in 'block (2 levels) in <top (required)>'
     # /~/.rvm/gems/ruby-3.4.7/gems/aruba-2.3.2/lib/aruba/rspec.rb:27:in 'block (2 levels) in <top (required)>'

Top 2 slowest examples (0.00016 seconds, 5.9% of total time):
  Concurrency concurrent deletions handles concurrent deletions without error
    0.00011 seconds ./spec/chrono_model/time_machine/concurrency_spec.rb:47
  Concurrency concurrent updates handles concurrent updates without PG::ExclusionViolation
    0.00005 seconds ./spec/chrono_model/time_machine/concurrency_spec.rb:14

Finished in 0.00261 seconds (files took 0.69954 seconds to load)
2 examples, 2 failures

Failed examples:

rspec ./spec/chrono_model/time_machine/concurrency_spec.rb:47 # Concurrency concurrent deletions handles concurrent deletions without error
rspec ./spec/chrono_model/time_machine/concurrency_spec.rb:14 # Concurrency concurrent updates handles concurrent updates without PG::ExclusionViolation

Randomized with seed 32877

@Riddlerrr
Copy link
Contributor Author

@tagliala This is expected behavior. You should run rake first to setup the default rails app for the tests. After the setup you can run separate tests as usual.
The rake command will re-create the rails app with latest DDL, so you have to run it each time you changing the DDL (template queries)

@tagliala
Copy link
Member

tagliala commented Nov 26, 2025

Got it, but it is not expected there to fail because specs under chrono_model do not require the rails application

 for file in spec/chrono_model/**/*_spec.rb; do be rspec $file; done

They all pass in isolation

Specs requiring rails are placed under aruba/ IIRC, but it is weird to have this under aruba, because this issue is related to the chronomodel core

@Riddlerrr Riddlerrr marked this pull request as draft November 26, 2025 13:34
@Riddlerrr
Copy link
Contributor Author

@tagliala Thanks for clarification 👍
I'll make them isolated then

@Riddlerrr Riddlerrr force-pushed the bugfix/concurrency-update branch from fe9ab7b to 3251767 Compare November 27, 2025 13:37
@Riddlerrr Riddlerrr marked this pull request as ready for review November 27, 2025 13:57
@Riddlerrr Riddlerrr force-pushed the bugfix/concurrency-update branch from f026cb5 to 522e564 Compare November 27, 2025 16:33
@Riddlerrr Riddlerrr force-pushed the bugfix/concurrency-update branch from 525460d to 5f48110 Compare November 28, 2025 13:26
@Riddlerrr Riddlerrr requested a review from tagliala December 15, 2025 09:09
@Riddlerrr Riddlerrr force-pushed the bugfix/concurrency-update branch from 8c95067 to 0aceac6 Compare December 15, 2025 10:29
@Riddlerrr Riddlerrr force-pushed the bugfix/concurrency-update branch from 0aceac6 to d5bf643 Compare December 15, 2025 10:32
@Riddlerrr Riddlerrr requested a review from tagliala December 15, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants