(wip) Part/phase 2 of adding Topic model #1339

showell · 2016-07-21T00:34:09Z

The last four commits here comprise phase 2 of #1191, and they set us to backfill Topic from Message. This series is not yet phase 3 (where we actually link Message rows to Topic), so the only risk in this series is operational headaches. To put it another way, running this migration is unlikely to impact app users beyond down time; it just might take a really long time to do the migration.

@timabbott Please review this with a little bit of extra scrutiny, as the approach here does set the template for how we structure code in the phase 3 migration.

smarx · 2016-07-21T00:34:12Z

Automated message from Dropbox CLA bot

@showell, it looks like you've already signed the Dropbox CLA. Thanks!

This migration simply creates an empty Topic model with no data, so it should probably require minimal babysitting.

Only write Topics for now.

ops note: After this commit is deployed, you should start to see rows in the zerver_topic table.

This function applies another migration function to all the rows in Message.

This migration could potentially take hours, even days, for really large installations. We need to test it very carefully before pushing to master.

timabbott · 2016-07-21T21:16:24Z

zerver/models.py

+    name = models.CharField(max_length=MAX_SUBJECT_LENGTH, db_index=True) # type: text_type
+
+    class Meta(object):
+        unique_together = ("recipient", "name")


We want these to be unique_together in a case-insensitive way; is that something we can do in Django?

(Or do we?)

Interesting question. I'm kind of ok with "Bart" and "BART" actually being distinct topics--one is about the Simpsons character, the other is about trains. But I think there's already a built-in concept of case insensitivity to our current code. (If we allowed the Bart/BART distinction, it would be mildly error prone for users, but realistically you're often just replying to a topic or auto-completing, so the system handles it for you.)

To your first question, I don't see a good way to do it in Django. I don't think the "unique_together" does much interesting on the app side; it's more about the database. And, for a composite key like this, we might want to do some DB stuff outside of Django anyway, like build indexes. (It's unclear to me whether postgres makes constraints provide index hints, but I know that postgres doesn't guarantee anything here.)

So, I think the most pragmatic thing to do here might be to omit the Meta class, and just update the migrations to build a unique index/constraint directly in postgres.

timabbott · 2016-07-21T21:24:06Z

Posted a few comments; overall the approach looks pretty reasonable. I'd consider adding to migrate_all_messages support for a sleep=1 option and using that in the migration (sleeping 1s between batches); this can help a lot with allowing other traffic through the database in case there is transaction/lock contention (I seem to recall we had to do that for various migrations back in the day).

showell · 2016-07-21T22:14:40Z

Thanks for reviewing. For all the performance considerations, it might be good to have a plan to try this on a real-world system, so that we can calibrate batch sizes, sleeps, etc., and also figure out how much the round-trips on get_or_create hurt us.

The good news about this particular backfill is that it's almost completely risk-free from a data integrity perspective. Since the app won't be actually using the Topic data yet, if the backfill gets aborted, or we want to re-tune parameters on our experimental instance, we can just blow away the table and start over.

zulipbot · 2017-05-22T21:09:44Z

Hello @showell, a Zulip maintainer reviewed this pull request over a week ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.