Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: on conflict criteria is ambiguous and cannot be used for upsert #2197

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

CassOnMars
Copy link
Member

@CassOnMars CassOnMars commented Jul 17, 2024

Why is this change needed?

Shuttle defines the messages table with two unique constraints, one for the hash, another for the triple of hash, fid and type. Postgres does not allow multiple ON CONFLICT criteria, and ambiguous conflicts are essentially surfaced as uniqueness violation failures in spite of ON CONFLICT criteria given. This change alters the merge to instead attempt insert, doing nothing on any conflict, then resolving with an update if necessary and the original conflict resolution criteria is satisfied.

Addresses Issue #2167

Merge Checklist

Choose all relevant options below by adding an x now or at any time before submitting for review


PR-Codex overview

This PR updates the conflict resolution logic in MessageProcessor to fix ambiguity issues during upsert operations.

Detailed summary

  • Updated conflict resolution logic to use doNothing() method instead of complex conditions
  • Improved handling of conflict criteria for upsert operations

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@CassOnMars CassOnMars added the t-bug A fix for a bug with the current system label Jul 17, 2024
Copy link

changeset-bot bot commented Jul 17, 2024

🦋 Changeset detected

Latest commit: 8c0b0e6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@farcaster/shuttle Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Jul 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hub-monorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2024 11:09pm

@@ -62,28 +62,35 @@ export class MessageProcessor {
...opData,
})
.returning(["id"])
.onConflict((oc) =>
oc
.columns(["hash", "fid", "type"])
Copy link
Member

@sds sds Jul 17, 2024

Choose a reason for hiding this comment

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

If I'm understanding the issue correctly, we could have probably replaced this line with .columns(["hash"]) and solved the problem, but this approach is perhaps more comprehensive (though it's impossible to have a conflict on hash-fid-type without also having a conflict on hash).

That said, I suppose it's technically the case that we allow two messages to have the same hash as long as they are of different types and/or from different FIDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

the problem is that postgres seems to handle it ambiguously when there's two conflict conditions, but you cannot bifurcate or aggregate conflict handlers. seems odd since they allow where clauses in the conflict scope

@CassOnMars CassOnMars merged commit ee0947e into main Jul 18, 2024
8 checks passed
@CassOnMars CassOnMars deleted the cassie-heart/shuttle-on-conflict branch July 18, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-bug A fix for a bug with the current system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants