Skip to content

feat: Retry on Postgres deadlock#21268

Merged
kodiakhq[bot] merged 11 commits intomainfrom
feat/ENG-1905-syncing-deadlocks
Aug 26, 2025
Merged

feat: Retry on Postgres deadlock#21268
kodiakhq[bot] merged 11 commits intomainfrom
feat/ENG-1905-syncing-deadlocks

Conversation

@stoovon
Copy link
Copy Markdown
Contributor

@stoovon stoovon commented Aug 22, 2025 •
edited
Loading

Summary

  • We do not recommend customers configure overlapping syncs.
    • Overlapping syncs can cause deadlocks in the Postgres destination.
    • Avoiding overlapping syncs is the primary recommendation to prevent deadlocks.
  • As a secondary safeguard, this PR introduces retries to handle most Postgres deadlock scenarios (other destinations are out of scope).
    • Retries are attempted up to 5 times, with random jitter between attempts.
      • Deadlocks are rare, even with multiple concurrent syncs. Five retries balances resilience and respect for Postgres backpressure.
    • Users can enable this behaviour via the retry_on_deadlock option.

@stoovon stoovon requested a review from a team as a code owner August 22, 2025 09:39
@stoovon stoovon requested a review from dcelasun August 22, 2025 09:39
@stoovon stoovon changed the title Feat/eng 1905 syncing deadlocks feat: Retry on Postgres deadlock Aug 22, 2025
@cq-bot
Copy link
Copy Markdown
Contributor

cq-bot commented Aug 22, 2025 •
edited
Loading

🚀 PostgreSQL Cloud UI deployed to Vercel:
https://plugin-destination-postgresql-cloud-ui-21268.vercel.app
You can also check out this plugin in action at:
https://cloud.cloudquery.io/teams/cloudquery-test/destinations/create?plugin-cloud-ui=cloudquery|destination|postgresql|https://plugin-destination-postgresql-cloud-ui-21268.vercel.app
Unique Vercel deployment URL:
https://plugin-cloud-kvnc500ni-cloudquery.vercel.app

stoovon
stoovon commented Aug 22, 2025
View reviewed changes
erezrokah
erezrokah reviewed Aug 25, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI there's a test in

cloudquery/plugins/destination/clickhouse/client/client_test.go

Line 238 in 630e091

func TestConcurrentSyncsSameTable(t *testing.T) {
you can borrow to simulate many concurrent syncs

@stoovon
Copy link
Copy Markdown
Contributor Author

stoovon commented Aug 26, 2025

test to simulate many concurrent syncs

I borrowed and tuned the test but I was not quite able to repro the issue. Nevertheless, the test exercises the correct code path so I think it still has value in terms of showing how to set up a spec with RetryOnDeadlock

@stoovon stoovon force-pushed the feat/ENG-1905-syncing-deadlocks branch 2 times, most recently from 75129d2 to 31243a1 Compare August 26, 2025 16:41
erezrokah
erezrokah reviewed Aug 26, 2025
View reviewed changes
erezrokah
erezrokah reviewed Aug 26, 2025
View reviewed changes
erezrokah
erezrokah reviewed Aug 26, 2025
View reviewed changes
erezrokah
erezrokah approved these changes Aug 26, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good pending adding docs for the new configuration

@stoovon stoovon force-pushed the feat/ENG-1905-syncing-deadlocks branch from 2b03787 to 9d1b1cc Compare August 26, 2025 17:12
stoovon added 9 commits August 26, 2025 18:13
@stoovon
feat: Add optional retryOnDeadlock to the client
2076839 
This is flagged behind the Spec to enable incremental deployment.
@stoovon
refactor: Introduce DBPool to allow low-level testing of deadlocks
f8704c7
@stoovon
feat: Add jitter, test
6667696
@stoovon
fix: Make gen
63c19f4
@stoovon
fix: Restore switch behaviour whilst keeping default branch for linter
318a3dd
@stoovon
test: Implement TestConcurrentSyncsAgainstSameTable
5ba5134 
This does not currently overwrite rows, but instead merely adds
new ones. It will not fully reproduce the deadlock unless we can
overwrite rows.
@stoovon
test: Refine TestConcurrentSyncsAgainstSameTable
60c5f46 
Definitely exercising the right code paths but still as yet unable
to create a repro.
@stoovon
fix: Reduce number of clients
931e8d5 
We weren't reproducing the deadlock and we're getting SQLSTATE 53300 (too many clients).
Could run the tests sequentially but not worthwhile unless an actual repro is possible.
@stoovon
refactor: RetryOnDeadlock is now the number of times to retry on dead…
188e02e 
…lock
@stoovon stoovon force-pushed the feat/ENG-1905-syncing-deadlocks branch from 9d1b1cc to 188e02e Compare August 26, 2025 17:15
erezrokah
erezrokah reviewed Aug 26, 2025
View reviewed changes
@stoovon stoovon added the automerge Automatically merge once required checks pass label Aug 26, 2025
@kodiakhq kodiakhq bot merged commit cfcffba into main Aug 26, 2025
20 of 22 checks passed
@kodiakhq kodiakhq bot deleted the feat/ENG-1905-syncing-deadlocks branch August 26, 2025 17:52
@cq-bot cq-bot mentioned this pull request Aug 26, 2025
Merged
kodiakhq bot pushed a commit that referenced this pull request Aug 27, 2025
@cq-bot
chore(main): Release plugins-destination-postgresql v8.10.0 (#21251)
bcdf1fb 
🤖 I have created a release *beep* *boop*
---


## [8.10.0](plugins-destination-postgresql-v8.9.0...plugins-destination-postgresql-v8.10.0) (2025-08-27)


### Features

* Retry on Postgres deadlock ([#21268](#21268)) ([cfcffba](cfcffba))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.89.0 ([#21238](#21238)) ([deb4cee](deb4cee))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erezrokah erezrokah erezrokah approved these changes

@dcelasun dcelasun Awaiting requested review from dcelasun dcelasun is a code owner automatically assigned from cloudquery/backend

Assignees

No one assigned

Labels

area/plugin/destination/postgresql automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stoovon @cq-bot @erezrokah