API for monthly_active_users table

[neilisfragile]

Doesn't actually do anything other than ensure that table purges old values.

Will form basis for extending #3630

[erikjohnston]

Some nits, but I think this needs to be hooked up so that we write to it, otherwise its hard to see whether this is the right way of doing things.

But broadly looks good so far!

[erikjohnston]

LIES AND SLANDER

[erikjohnston]

Can't this just be SELECT COALESCE(count(*), 0) FROM monthly_active_users?

[erikjohnston]

I think this is spurious

[erikjohnston]

Who's jeff?

I'd probably write it/reformat to simply be:

defer.returnValue(bool(user_present))

[erikjohnston]

Upserts without locks are dangerous as they're racey. Upserts with locks are bad and should be avoided.

I'd quite like to see how this is actually hooked up

[neilisfragile]

Have reworked nits, and left upserts until we figure out a better way to solve this

[krombel]

No prints. Please use logger if you want to have that

[krombel]

You probably want to return somehow if this condition is false.

[neilisfragile]

monthly_active_users now fully backing log in and register tests.
table is reaped, and read requests cached

Especially keen to get comment on adding @defer.inlineCallbacks to insert_client_ip
Also, the caching.

[erikjohnston]

store is the same as self here

[neilisfragile]

ah good point

[erikjohnston]

Deferred with two r's. Also, Returns with an s, and a newline before it. (This is so that IDEs can read it to get type hints)

[erikjohnston]

You don't need to include a LIMIT

[neilisfragile]

I don't need it for Postgres, but best I can tell I do need it for sqlite https://sqlite.org/lang_select.html

[erikjohnston]

Ugh, that's true. But it doesn't look like postgres supports negative limits. You can instead probably structure the query as:

DELETE FROM monthly_active_users
WHERE user_id NOT IN (
   SELECT user_id FROM monthly_active_users
   ORDER BY timestamp DESC
   LIMIT ?
)

[neilisfragile]

groans, I guess the table never gets too large

[erikjohnston]

These invalidations should only happen after the transaction has committed, which can be done by yielding on the self.runInteraction

[erikjohnston]

This seems odd. Either this should raise an AuthError and callers can catch AuthErrors and re-raise as a more appropriate exception, or it should just return a boolean.

[erikjohnston]

I know its a bit nitty, but consistency is quite important for readability:

"""Generates current count of monthly active users.abs

Returns:
    Defered[int]: Number of current monthly active users
""""

(Indentation, returns and square brackets for the type Deferred returns)

[erikjohnston]

This feels like it should be in monthly_active_users.py

[erikjohnston]

I would avoid having to do an update on every request. One way of avoiding this is to have the is_user_monthly_active return the timestamp and use that to only insert if the difference is greater than, say, a couple of hours. Since that DB function is cached we avoid doing queries most of the time.

[neilisfragile]

makes sense

[erikjohnston]

We should only invalidate if we actually added a new user to the table (rather than just updating), helpfully _simple_upsert returns True on insert and False on update.

[erikjohnston]

Especially keen to get comment on adding @defer.inlineCallbacks to insert_client_ip

I think that's fine, so long as we don't actually perform a query in the common case.

[erikjohnston]

There's a _simple_select_one_onecol ftr

[neilisfragile]

handy, done

[erikjohnston]

Can you quickly add a comment to say why we're not always upserting?

[erikjohnston]

Can you add a quick comment saying what timestamp is? In particular, its probably worth calling out that it isn't necessarily accurate.

[krombel]

s/an/and/

[krombel]

abs?

[neilisfragile]

ah, editor autocomplete

[krombel]

s/syanpse/synapse/

[erikjohnston]

Modulo spelling mistakes, thanks @krombel

[ara4n]

I'm really surprised this is called timestamp, which is a reserved keyword in postgres and most SQL (it's a data type), as you can from the syntax highlighting. It also doesn't describe what the column is used for - surely it should be last_active or similar?

[ara4n]

(it's also a problem because you can't rename columns in sqlite, so we really want to get it right the first time).

[ara4n]

this would be monthly_active_users_timestamp to be consistent, as timestamp is one word

[richvdh]

since we're in the business of finding problems in PRs after they have landed:

• why was this in synapse/storage/schema/delta/51 rather than synapse/storage/schema/delta/50 ? Doing so has made it impossible for people to roll back from synapse 0.33.3 to 0.33.2 without manual database work.
• Please can we use CREATE TABLE IF NOT EXISTS and CREATE INDEX IF NOT EXISTS. There are occasions when a delta fails half-way through, and the IF NOT EXISTS makes the delta idempotent.

[erikjohnston]

We usually create a new delta file after a release? Anyway, do we really re run schema files even if they've been applied before? But agreed should try and remember IF NOT EXISTS, though CREATE INDEX doesn't support it on postgres 9.4

[richvdh]

We usually create a new delta file after a release?

A new delta file, sure. A new directory? Well, I didn't get the memo, and I question the value of doing so vs the problems it presents for anyone trying to roll back.

Anyway, do we really re run schema files even if they've been applied before?

we do not, but the update code does get upset if either (a) the database schema number is higher than it knows about or (b) it finds records of delta files from version X already having been applied when trying to upgrade from X-1 to X.

CREATE INDEX doesn't support it on postgres 9.4

gahhhh