SpringAll
SpringAll copied to clipboard
Simplify the query
Looks like there's some edge cases/historic crufty databases where that can happen:
https://github.com/diaspora/diaspora/commit/30af2e7188fbe6e29d3aa566d710f5a6a67aaabd
As for the username, if I remember correctly at some point our invite system would create blank user records and that's why it's there. The initial version of this has the filter: https://github.com/diaspora/diaspora/commit/c98f2d166429129dbfe3c34c49ccf8ce431c85f0 So I cannot find a documented reasoning.
In any case the difference seems small, the database has to do a sequential scan over the table for either case.
The username
shouldn't be a problem, I cleaned up old invitations in https://github.com/diaspora/diaspora/pull/6976, and the column is NOT NULL
now.
But I'm not sure about created_at
. It wasn't always NOT NULL
, and it looks like the NOT NULL
was silently added in https://github.com/diaspora/diaspora/commit/28a71a46aaf306809822a5c71eb610216d4e4a31 without ever having a proper migration which would add it on pods which don't have it yet.
So there might still be pods where there are users without a created_at
. At least joindiaspora has users without created_at
, but maybe that's a JD-only thing again. All other pods I have access to don't have users without created_at
(but they also already have the NOT NULL
, as they were created after that, or migrated to postgres after that commit), and I asked DenSchub about geraspora (to check another pretty old pod), and there are also no users without a created_at
.
With the current code, if created_at
is NULL, it crashes in the line week = u.created_at.beginning_of_week.strftime("%Y-%m-%d")
where it tries to access that.
So I'm unsure about that. I feel like we need a proper migration first, which ensures that all (old) pods have all the columns with NOT NULL
where it was silently added in https://github.com/diaspora/diaspora/commit/28a71a46aaf306809822a5c71eb610216d4e4a31, only then we can be sure that it's really NOT NULL
as expected everywhere.
(It doesn't need to be changed in this PR, I think this PR is fine as it is ... but I think it should be changed before this is merged, because otherwise we maybe risk breaking some pods)