-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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: avoid duplicate tag inserts by checking if the mapping exists already in db #45580
fix: avoid duplicate tag inserts by checking if the mapping exists already in db #45580
Conversation
Sorry, cannot judge the code |
SystemTagObjectMapper / oc_systemtag_object_mapping state if a tag belongs to a given object (e.g. a file). The code does a select to see if a given mapping already exists, not a tag. I suggest updating the commit message accordingly. Is there a reason for this change and the additional select? The situation that a mapping already exists is handled properly by the catching the UniqueConstraintViolationException. |
The catch is enough to prevent Nextcloud logs, but not enough to prevent logs from the DB which apparently can grow very large in short time spans. |
Thanks. I'm aware about this problem with filecache and authtokens. It surprised me a bit to see this problem with the systemtag_object_mapping table as well. |
For some cases, it's possible to use our "insertIgnoreConflict" method: server/lib/public/IDBConnection.php Lines 147 to 158 in d045482
For example: |
Apparently:
The implementation only covers PostgreSQL, is there any equivalent for MySQL? |
INSERT IGNORE (also non-standard unfortunately) |
My proposal: #45655 |
can I ask if there a backport planned for this(or a linked PR) for NC28 or NC29 or it is not a option for this PR and these really cool changes will come only with NC30? |
I read Alexander's support ticket and #44933 and now the change makes much more sense to me. To my understanding, the "duplicate key value violates unique constraint" errors for filecache and auththokes are caused by operations running in parallel. The error for systemtag_object_mapping because our implementation is incomplete or at least strange ;) Take the following flow:
The flow is triggered for all operations and will trigger the warning for 2. and 3. So it's not an edge case or a parallel operation. The api consumer like files_automatedtagging or recognize expect that assignTags takes care if a tag is already assigned. It would be still nice to improve the commit message (it's checking if the mapping does exist and not the tag). |
@bigcat88 we will backport this to NC28 and NC29 |
/backport to stable29 |
/backport to stable28 |
cc @marcelklehr because I recall some reports for recognize about it. Not sure though if about that exact problem, but related to the tagging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…eady in db. if it does not exist alone insert the same or else do nothing Signed-off-by: yemkareems <yemkareems@gmail.com>
…ound the select and insert Signed-off-by: yemkareems <yemkareems@gmail.com>
…arting the inserts Signed-off-by: yemkareems <yemkareems@gmail.com>
9520c34
to
87cd784
Compare
fix: do a select in systemtag_object_mapping to see if tag exists already in db. if it does not exist alone insert the same or else do nothing
Summary
TODO
Checklist