We used to have a UNIQUE constraint, but it was removed at rEFLW873cbbca3e61: Remove unique constraints from flow_*_ref. See also T92284: S9. LQT: Unique index flow_ext_ref_pk blocking false positives.
We should discuss that, as well as alternatives like an artificial primary key (as we have on most of the Flow tables).
| Status | Subtype | Assigned | Task | |
|---|---|---|---|---|
| · · · | ||||
| Resolved | LSobanski | T17441 Some tables lack unique or primary keys, may allow confusing duplicate data | ||
| Resolved | matthiasmullie | T109676 Add artificial primary key to flow_wiki_ref and flow_ext_ref | ||
| Resolved | jcrespo | T111084 Do backfill and final schema change for "Segregate Reference objects by source wiki." | ||
| Resolved | jcrespo | T117787 Make ref_id of flow_wiki_ref and flow_ext_ref not null and PK (DBA phase 2) | ||
| Resolved | Catrope | T117786 Run maintenance/FlowPopulateRefId.php on all production wikis | ||
| Resolved | Catrope | T117785 Deploy artificial primary key reference change to all production wikis simultaneously | ||
| Resolved | jcrespo | T117783 Add ref_id to flow_wiki_ref and flow_ext_ref (DBA phase 1) | ||
| · · · |
Allow me to help with this at any time (ping me for support). If the real primary key is unsuitable, because it is too large, or does not cover the uniqueness properly because of index limitations, an artificial primary key is certainly an alternative.
Please note that for the particular engine we are using, there is not such a thing as a table without a primary key: if we do not define one explicitly, and there is no other unique constraint, innodb will create a hidden column for it, but that cannot be accessed by the user. This means that certain maintenance tasks, like altering a table online, checksuming it or even row based replication, in some cases, will be not possible or have worse performance.
Those things will affect you directly, because they can be the difference between taking months (so a failover is necessary) to perform an alter or taking hours.
We'll probably go with an artificial primary key (which I think should be a UUID), both to reduce index size and avoid collisions (which can be caused by prefixing even if the full URL is unique).
Change 238393 had a related patch set uploaded (by Matthias Mullie):
Add artificial primary key to flow_wiki_ref and flow_ext_ref
@Mattflaschen: is there a particular reason you thought it should be a UUID? (see comment on https://gerrit.wikimedia.org/r/#/c/238393/)
I mostly made it a UUID for consistency with the rest of the Flow tables & because you had also already expressed a preference for UUID :p
Consistency (e.g. with workflow_id) was my primary reason as well.
I also thought it might cause some small issues. Of course, now that I go looking for it, I can't seem to find any code that specifically *needs* a UUID as PK (except that strings that were not UUIDs but are the right length would be Bad due to convertUUIDs).
I still think it would be best unless it causes significant performance problems (probably not, since Jcrespo +1'ed on Gerrit after we explicitly discussed this). If we standardize things like coding conventions, we should probably standardize this as well, absent a good reason to make an exception.
@Mattflaschen, I am ok with you explanation. Just remember that when there is no other reason, in general, an auto_increment would be preferred for pure size and monotonic increase reasons (which has some advantages for InnoDB). Not an issue for small tables.
@Catrope We should make T111084 a blocker, so after that is deployed.
@jcrespo We're ready for you to move ahead on https://gerrit.wikimedia.org/r/#/c/238393/ .
@jcrespo See updates at https://gerrit.wikimedia.org/r/#/c/238393/ and T117138. I think all the glitches I saw were caused by T117138, which was not introduced by this patch.
After Monday morning (we have a morning SWAT then for T117138), this is ready for you to look at again.
@Mattflaschen, let's try to do formally once the new proposed workflow:
I think this has little overhead, in exchange I will reward you with a faster ticket processing (among other reasons because I will notice it faster).
Done, except DBA apparently doesn't exist yet, so I used Database. This is T117783: Add ref_id to flow_wiki_ref and flow_ext_ref (DBA phase 1).
- As you will want to do 2 phases at separate times, I will ask you to create another ticket for the second phase (that way you know what exactly has been done and what is left to do)
Done. This is T117787: Make ref_id of flow_wiki_ref and flow_ext_ref not null and PK (DBA phase 2).
- Please add the following information to the subtask: What (point me to the git repo with the schema change), Where (officewiki on the main servers and flowdb on x1), when (point me to the merge of https://gerrit.wikimedia.org/r/#/c/238393/ and the supposed date- Monday), why? (this ticket) and that you have checked that application will work after the change (that should be a yes)
Done. To be clear, I was not saying this had to be done Monday, just sometime after Monday morning.
The current deployment plan is based on doing T117783: Add ref_id to flow_wiki_ref and flow_ext_ref (DBA phase 1) before merge (or at least before deployment, but before merge is less error-prone). Let me know if that is acceptable.
Done, except DBA apparently doesn't exist yet, so I used Database.
You did well. It requires discussion for the rename, which is like an unnecessary overhead, but anyway... :-)
To be clear, I was not saying this had to be done Monday, just sometime after Monday morning.
I understanded correctly. I only asked to have time to schedule it.
Let me know if that is acceptable.
This is more than ok. Thanks for taking the time.
Change 238393 merged by jenkins-bot:
Add artificial primary key to flow_wiki_ref and flow_ext_ref
Change 251415 had a related patch set uploaded (by Mattflaschen):
Add artificial primary key to flow_wiki_ref and flow_ext_ref
Change 251416 had a related patch set uploaded (by Mattflaschen):
Add artificial primary key to flow_wiki_ref and flow_ext_ref
Change 251415 abandoned by Alex Monk:
Add artificial primary key to flow_wiki_ref and flow_ext_ref
Reason:
Branch no longer in use.
Change 251416 merged by jenkins-bot:
Add artificial primary key to flow_wiki_ref and flow_ext_ref
Change 251558 had a related patch set uploaded (by Mattflaschen):
Add isset to handle when ref_id is not in row returned from MC index
Change 251558 merged by jenkins-bot:
Add isset to handle when ref_id is not in row returned from MC index
Change 259444 had a related patch set uploaded (by Catrope):
Make ref_id the primary key for DbStorage purposes
I don't think think this is Blocked anymore. Does anyone still want to do https://gerrit.wikimedia.org/r/259444 ?
I don't think so.
Matthias said in 9ca081f7675cd3a1e23bac1eb9c04d539fb47f6d, "The newly added ref_id is not used in the code, it's only meant to make DB maintenance easier."
Change 259444 abandoned by Catrope:
Make ref_id the primary key for DbStorage purposes