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

[mailpit+rqlite] FOREIGN KEY constraint failed error on deleting messages #374

Closed
hpoznanski opened this issue Oct 17, 2024 · 17 comments
Closed

Comments

@hpoznanski
Copy link

I've deployed mailpit on my GKE cluster with rqlitedb. Everything works fine but theres a problem after I will add tags to mail. Im not able to delete them because im getting: "FOREIGN KEY constraint failed". Im not able to remove all or even one by one.

mailpit: v1.20.6
rqlite: 8.31.2

@axllent
Copy link
Owner

axllent commented Oct 17, 2024

@hpoznanski I think I found the issue (assuming you are using the tenant ID option?). Could you please test using the latest axllent/mailpit:edge docker image (instead of axllent/mailpit:latest) and confirm for me whether this fixes your issue? Thanks.

@axllent
Copy link
Owner

axllent commented Oct 19, 2024

I have released a v1.20.7 with the fix for this (I believe) so I'm closing this issue. If the problem persists then please feel free to re-open this.

@axllent axllent closed this as completed Oct 19, 2024
tmeijn pushed a commit to tmeijn/dotfiles that referenced this issue Oct 22, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [axllent/mailpit](https://github.com/axllent/mailpit) | patch | `v1.20.6` -> `v1.20.7` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>axllent/mailpit (axllent/mailpit)</summary>

### [`v1.20.7`](https://github.com/axllent/mailpit/blob/HEAD/CHANGELOG.md#v1207)

[Compare Source](axllent/mailpit@v1.20.6...v1.20.7)

##### Chore

-   Update caniemail database

##### Fix

-   SQL error deleting a tag while using tenant-id ([#&#8203;374](axllent/mailpit#374))

##### Testing

-   Add tenantIDs to tests

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
@matteovivona
Copy link

Same issue here. Always on GKE.

Mailpit: v1.21.6
Rqlite: v8.35.0

@axllent axllent reopened this Dec 19, 2024
@axllent
Copy link
Owner

axllent commented Dec 19, 2024

@matteovivona I am unable to reproduce this error, so I do not think it is actually the same issue. Can you please provide more information to help me understand the error you have, what you were trying to do at the time, the error message etc? Thanks.

@matteovivona
Copy link

I'm trying to delete some emails, either globally or for a specific tag, but I ran into an error message about the FOREIGN KEY constraint failing. I'm not sure if this info will be useful for troubleshooting, but I have rqlite set up with 3 instances

@axllent
Copy link
Owner

axllent commented Dec 19, 2024

It actually may be useful - the fact that you have 3 instances (I assume you mean replicated?) could mean that the replicated changes are potentially happening in an unexpected order. Leave this with me and I'll see if I can work on a solution 👍

@otoolep
Copy link

otoolep commented Dec 19, 2024

are potentially happening in an unexpected order.

This doesn't really make much sense. All changes e.g. DELETE always take place in a well-defined order, that's what Raft does for you.

As long as you wait for the HTTP 200 from a given write request, any subsequent write will take place against a system that reflects the first write. It doesn't matter which nodes you send the write to, since all writes go through the Leader. A node will transparently redirect its write to the Leader if necessary.

As for reads, just use weak consistency (the default). That way all reads will go through the Leader too, and result in generally consistent results. And, if a node receives a read request, and it's not the Leader, that node will transparently forward the request to the Leader.

@otoolep
Copy link

otoolep commented Dec 19, 2024

If this issue is reproducible I would try it with a single-node cluster, just to remove the clustering complexity.

I'm not saying that clustering isn't an issue here (though I doubt it). This feels more like a rqlite client issue, where the client i.e. mailpit thinks it has deleted some records (deletes the wrong records, is ignoring an error from rqlite, is not waiting for a response to write request, something like that), goes to delete more, only to hit an FK constraint.

But this can't be caused by some "unexpected replication" since rqlite is 100% deterministic when it comes to replicating changes across the cluster, and does do in a very robust manner. Writes are always serialized on the cluster. They cannot overlap, and are never re-ordered relative to the order in which the client sends them.

@axllent
Copy link
Owner

axllent commented Dec 19, 2024

Thanks for the clarification @otoolep - I suspected that was the case, however in saying that, the message_tags table does have two (unused) REFERENCES to the message & tag IDs (part of an original idea to cascade deletes which I never implemented). Mailpit wraps a series of deletes from multiple tables in a single transaction (which I believe wasn't a feature in rqlite due to the TCP / REST nature?) which is maybe why it is failing (deleting things in the wrong order from the Mailpit side). I think the simplest option is for me to remove the REFERENCES references altogether from the table. I just need a little more time to test and work on a solution and dig a little deeper.

@otoolep
Copy link

otoolep commented Dec 19, 2024

rqlite supports wrapping a single HTTP request in a transaction.

https://rqlite.io/docs/api/api/#transactions

If you are putting the "series of deletes" in such a transaction they will all succeed, or none will succeed.

@otoolep
Copy link

otoolep commented Dec 19, 2024

If you are doing explicit BEGIN, COMMIT , and so on -- that is not officially supported by rqlite.

@otoolep
Copy link

otoolep commented Dec 19, 2024

If you are putting the "series of deletes" in such a transaction they will all succeed, or none will succeed.

To give you an example. If a HTTP request , which does not set the transaction query param, contains two DELETE statements (bulk request) and one of those DELETEs works fine, but the other violates a FK constraint, then the first DELETE will remain effective after you receive the response from rqlite. However if you set transaction then neither DELETE will have been effective when you receive the response from rqlite.

It's safe to set the transaction flag even for a single request. So you could just set it for all your requests. I think the gorqlite driver does exactly that.

@axllent
Copy link
Owner

axllent commented Dec 19, 2024

If you are putting the "series of deletes" in such a transaction they will all succeed, or none will succeed.

It's safe to set the transaction flag even for a single request. So you could just set it for all your requests. I think the gorqlite driver does exactly that.

From my understanding the transaction would not work the way I'm using it, and that rqlite handles that as completely separate requests. Furthermore, the BeginTx() and tx.Commit() are in the driver as really just placeholders that don't do anything - if I understand that correctly.

I suspect that the REFERENCES don't trigger an error with a local SQLite database because of the transaction, however with independent queries to rqlite something sometimes goes wrong. I've tested with a single local instance of rqlite without an issue however, so I don't know why it works on my end, but here we are.

I could change the order the tables are deleted from, however the safest and best solution here would be for me to simply remove those unused REFERENCES altogether to prevent future issues. If rqlite handles that as separate transactions then that's not an issue - these are just bulk deletes from the various tables anyway.

axllent added a commit that referenced this issue Dec 20, 2024
…374)

This SQL patch rebuilds the message_tags table to remove the unused ID & TagID REFERENCES that was sometimes causing FOREIGN KEY errors when deleting messages (with tags) using the rqlite database. This is not a bug in rqlite, but rather a limitation of how Mailpit integrated with rqlite as an optional alternative database.
@axllent
Copy link
Owner

axllent commented Dec 20, 2024

@matteovivona I have just released v.1.21.8 with a fix for this (I'm pretty confident it will at least). I'd appreciate if you could test and report back whether it resolves your issue.

Thanks again for your input @otoolep - as always appreciated to get input from the top! I realise I could go down the path to get the this transaction to work with rqlite, however this was the cleaner solution as those two REFERENCES weren't being used anyway and had the potential to cause other issues down the track. If the message deletions were being cascaded through to the message tags table (my original plan) it may have worked as originally intended, however it's a fancy feature I wasn't using.

@otoolep
Copy link

otoolep commented Dec 20, 2024

Ack, got it. Yes those calls probably don't do anything in the driver, since they are not supported by rqlite at this time. I have ideas, but it would be major work.

@matteovivona
Copy link

matteovivona commented Dec 20, 2024

@matteovivona I have just released v.1.21.8 with a fix for this (I'm pretty confident it will at least). I'd appreciate if you could test and report back whether it resolves your issue.

@axllent thanks for the speed! I confirm that the patch works

@axllent
Copy link
Owner

axllent commented Dec 20, 2024

Excellent! Thanks again for bringing this to my attention @matteovivona.

@axllent axllent closed this as completed Dec 20, 2024
tmeijn pushed a commit to tmeijn/dotfiles that referenced this issue Dec 20, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [axllent/mailpit](https://github.com/axllent/mailpit) | patch | `v1.21.6` -> `v1.21.8` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>axllent/mailpit (axllent/mailpit)</summary>

### [`v1.21.8`](https://github.com/axllent/mailpit/blob/HEAD/CHANGELOG.md#v1218)

[Compare Source](axllent/mailpit@v1.21.7...v1.21.8)

##### Chore

-   Update node dependencies
-   Update Go dependencies

##### Fix

-   **db:** Remove unused FOREIGN KEY REFERENCES in message_tags table ([#&#8203;374](axllent/mailpit#374))

### [`v1.21.7`](https://github.com/axllent/mailpit/blob/HEAD/CHANGELOG.md#v1217)

[Compare Source](axllent/mailpit@v1.21.6...v1.21.7)

##### Chore

-   Update node dependencies
-   Update Go dependencies
-   Bump Go version for automated testing
-   Move smtpd & pop3 modules to internal
-   Stricter SMTP 'MAIL FROM' & 'RCPT TO' handling ([#&#8203;409](axllent/mailpit#409))
-   Display "To" details in mobile messages list
-   Display "From" details in message sidebar (desktop) ([#&#8203;403](axllent/mailpit#403))

##### Fix

-   Ignore unsupported optional SMTP 'MAIL FROM' parameters ([#&#8203;407](axllent/mailpit#407))
-   Prevent splitting multi-byte characters in message snippets ([#&#8203;404](axllent/mailpit#404))

##### Testing

-   Add smtpd tests

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS42NC4wIiwidXBkYXRlZEluVmVyIjoiMzkuNzUuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
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

No branches or pull requests

4 participants