[bugfix] Ensure `pending_approval` set on statuses + status faves

[tsmethurst]

Description

Fix an issue with the interaction policy migration where pending_approval did not have NOT NULL set on it, and write a migration to correct statuses and status_faves. Also ensure that this field is always set by adding some logic for it to the type converter.

Note, this PR doesn't bother completely undoing and redoing the previous migration by dropping and recreating the statuses + status faves tables, because I'm not really keen on having folks who've run the RC having to do another massive migration. The NOT NULL is not actually that important as long as we remember to set it ourselves anyway.

Checklist

Please put an x inside each checkbox to indicate that you've read and followed it: [ ] -> [x]

If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).

• [x] I/we have read the GoToSocial contribution guidelines.
• [x] I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
• [x] I/we have not leveraged AI to create the proposed changes.
• [x] I/we have performed a self-review of added code.
• [x] I/we have written code that is legible and maintainable by others.
• [x] I/we have commented the added code, particularly in hard-to-understand areas.
• [ ] I/we have made any necessary changes to documentation.
• [ ] I/we have added tests that cover new code.
• [x] I/we have run tests and they pass locally with the changes.
• [x] I/we have run go fmt ./... and golangci-lint run.

[mirabilos]

Is it deliberate that you didn’t add an ALTER TABLE to set the column to NOT NULL afterwards?

That would add a safety net and possibly help the query optimiser.

[tsmethurst]

Yes it's deliberate, it's mentioned in the PR description.

[mirabilos]

I don’t quite see that. The second paragraph says the NOT NULL is “not important” (but see what I noted above) but not why you don’t fire a simple ALTER TABLE statuses ALTER COLUMN pending_approval SET NOT NULL; to fix it up, which is quick enough.

[tsmethurst]

You can do that on postgres but not on sqlite. The process for doing the same on sqlite is convoluted and involves either dropping and recreating the column, or creating an entirely new table, deleting the old table, and then renaming the new one. Rather than introduce even more disparity between the two versions and potentially add another long migration I opted to just let it be.

[tsmethurst]

At some point in the future we'll probably make a version that migrates all tables to some nice neatly defined version anyway, and then we can remove all the old migrations, so if the columns of some people's DBs are missing a NOT NULL constraint until then it's not really that important.

[mirabilos]

On Sun, 13 Oct 2024, tobi wrote:

You can do that on postgres but not on sqlite.

Ah, okay. Too bad.

The process for doing the same on sqlite is convoluted and involves either dropping and recreating the column, or creating an entirely new table, deleting the old table, and then renaming the new one.

Ugh.

Rather than introduce even more disparity between the two versions and potentially add another long migration I opted to just let it be.

Fair enough. As to how much a missing NOT NULL constraint really slows down queries, to answer this we’d need a PostgreSQL expert; probably, lower-hanging fruits exist wrt. DB performance, anyway.

I was just puzzled, as I was unaware of that particular sqlite issue (I don’t use sqlite myself, as my licence review of it was more than unfortunate but detail would be offtopic here).

Thanks for the info, //mirabilos