SpringAll icon indicating copy to clipboard operation
SpringAll copied to clipboard

Simplify the query

Open jwjwyoung opened this issue 5 years ago • 2 comments

jwjwyoung avatar Nov 28 '19 04:11 jwjwyoung

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.

jhass avatar Dec 03 '19 09:12 jhass

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)

SuperTux88 avatar Oct 20 '22 23:10 SuperTux88