Will we ever be able to "un-delete" a user? Would is be easy to implement this to handle that case if we ever support it?

Why we don't implement this as a user status change? Perhaps as an additional field on the table or modify the enum to include deleted. Wrt to changing the enum, it doesn't make much sense to me to differentiate between e.g. deleted/active or deleted/dormant users (or whatever the possibilities are), so changing it makes sense IMO.

That is true. I assume we can still make an index with a conditional expression onstatus == deleted. We might want to have some trigger or something to prevent "un-deletion" if we want to prevent that.

I considered these options, but avoided them simply because they expand the scope beyond a reasonable time cost for this feature. We could relatively easy migrate from this to a space wheredeleted is just another user status.

I'm trying to remain agnostic of the massive rabbit-hole shaped question of what the business rule should be around un/re-deletion.

The trigger and function right now would handle un/re-deletion correctly, but the query would break as identified by@Emyrk in another comment below.

I would prefer that we stick to the current implementation because it is good enough and I can't find anywhere that we support un/re-deletion right now. If y'all would like to explicitly request a specific change I'd be happy to consider it.

@SasSwart can we open an issue to address un-deletion and mention this query if we decide to support it?

Just to track it

If it is inclusive, should it be?

I think that's covered by the initial and final union.

Personally I prefer start_time inclusive, end_time exclusive though, as most of the time that mirrors the intuitive understanding of data. Say we want to view Monday to Sunday (1 week), we ultimately want Monday 00:00 -> Sunday 23:59 but it's simplest to request this as Monday 00:00 -> Monday 00:00 instead of subtracting an arbitrary unit of time or using an end-of-day function. Alternatively, if we include Monday 00:00 at the end, we include data from the beyond the range.

I think that's covered by the initial and final union.

Correct.

Monday 00:00 -> Monday 00:00 instead of subtracting an arbitrary unit of time or using an end-of-day function

I think the inclusive approach is good enough. There are higher impact changes to make. If you'd like me to change it, please ask me directly to do so and I will. My own opinion is that for how this chart is going to be displayed inclusive works well enough.

If a user is deleted twice in it's history (should not be possible, but would be nice to be durable against), then this join will duplicate the status changed rows for each deletion event.

We could make the table a subquery that returns 1 row, or we should enforce a user can only be deleted once.

Minimal example:https://www.db-fiddle.com/f/4jyoMCicNSZpjMt4jFYoz5/15847

Why we don't implement this as a user status change? Perhaps as an additional field on the table or modify the enum to include deleted. Wrt to changing the enum, it doesn't make much sense to me to differentiate between e.g. deleted/active or deleted/dormant users (or whatever the possibilities are), so changing it makes sense IMO.

I think that's covered by the initial and final union.

Personally I prefer start_time inclusive, end_time exclusive though, as most of the time that mirrors the intuitive understanding of data. Say we want to view Monday to Sunday (1 week), we ultimately want Monday 00:00 -> Sunday 23:59 but it's simplest to request this as Monday 00:00 -> Monday 00:00 instead of subtracting an arbitrary unit of time or using an end-of-day function. Alternatively, if we include Monday 00:00 at the end, we include data from the beyond the range.

I'm a bit worried about the number of CTEs and performance, but I think this is fine for now and let's not prematurely optimize. Just raising this so you're aware that in some cases a CTE performs worse than a pure query with joins and subqueries. That's mainly because the result of a CTE lacks indexes.

I was wondering about performance too, but user count and user status changes is going to be way less data than say workspace builds. Like on the order of 1000s. 🤷

Might be worth a benchmark, but we don't have a good way to populate "large" datasets atm.

I did it this way for legibility. Trying to write the optimal solution here made my own query inscrutable even to myself. I don't consider this to be on a particularly hot path. We do have metrics for this query. If it needs to be optimised, we can do so.

Just raising this so you're aware that in some cases a CTE performs worse than a pure query with joins and subqueries

Also these CTEs were designed to reduce the amount of data being handled as early as possible. We use the existing indices while we have them to identify exactly the data we need ASAP and then once we need to join it all the hope is that we've brought down the volume low enough that it's performant regardless of the lack of indices on CTEs.