Page MenuHomePhabricator

content.content_id is not "NOT NULL" while being primary key
Closed, ResolvedPublic

Description

While doing T230428: Migrate tables.sql to abstract schema. It seems content.content_id is primary key (and auto incremented) but unlike most PKs in other tables, it's not "NOT NULL":

CREATE TABLE /*_*/content (
  -- ID of the content object
  content_id bigint unsigned PRIMARY KEY AUTO_INCREMENT,
  -- Nominal size of the content object (not necessarily of the serialized blob)
  content_size int unsigned NOT NULL,
  -- Nominal hash of the content object (not necessarily of the serialized blob)
  content_sha1 varbinary(32) NOT NULL,
  -- reference to model_id. Note the content format isn't specified; it should
  -- be assumed to be in the default format for the model unless auto-detected
  -- otherwise.
  content_model smallint unsigned NOT NULL,
  -- URL-like address of the content blob
  content_address varbinary(255) NOT NULL
) /*$wgDBTableOptions*/;

Is this intentional? It might need fixing otherwise you can set it to "null" explicitly and it would probably break lots of things.

Event Timeline

AFAIK, MySQL doesn't let you insert a null value as primary key. I think it sort of adds an implicit NOT NULL for primary keys, which is then redundant.

Unsure about other DBMSs (and also not 100% sure about MySQL). Either way, the intention is to have a non-nullable value (and that's the status quo for MySQL), so we might well add an explicit NOT NULL constraint.

hmm, It seems other DBMSes also implicitly take PKs as non-nullable but the problem is that at least for what I read for oracle, this is bound to the column being PK. For example, if we decide that content_id should not be a PK anymore, it automatically loses the non-nullable status. I don't like implicit assumptions in our database scheme tbh.

Great: https://www.sqlitetutorial.net/sqlite-primary-key/

In SQL standard, the primary key column must not contain NULL values. It means that the primary key column has an implicit NOT NULL constraint.
However, to make the current version of SQLite compatible with the earlier version, SQLite allows the primary key column to contain NULL values.

hmm, It seems other DBMSes also implicitly take PKs as non-nullable but the problem is that at least for what I read for oracle, this is bound to the column being PK. For example, if we decide that content_id should not be a PK anymore, it automatically loses the non-nullable status. I don't like implicit assumptions in our database scheme tbh.

+1, because even if redundant, we cannot rely on everyone remembering what the PK constraint implies on different DBMSs.

Great: https://www.sqlitetutorial.net/sqlite-primary-key/

In SQL standard, the primary key column must not contain NULL values. It means that the primary key column has an implicit NOT NULL constraint.
However, to make the current version of SQLite compatible with the earlier version, SQLite allows the primary key column to contain NULL values.

It's not the first time I hear about incompatibilities with SQLite. For instance, T253199 is concerning. I wonder how good our support really is for SQLite, but that's off-topic here.

It's not the first time I hear about incompatibilities with SQLite. For instance, T253199 is concerning. I wonder how good our support really is for SQLite, but that's off-topic here.

It was way worse for DBMSes that we didn't have tests for (like MSSQL, Oracle) and my favorite being MSSQL having user_group expiry column on wrong table for two years (including one LTS release).

In general, the abstract schema and schema changes that I'm working on (the T191231) will address the issue to a high degree (in fact, these bugs have been unearthed because of migrating to the abstract schema) and I have already found so many inconsistencies in Postgres already (and have been fixing those as well): T164898. Once we finish this migration, we will be in a much better shape in supporting non-MySQL DBMSes

One other note: slot_roles and content_models are also similar.

eprodromou subscribed.

OK, this seems reasonable to do. Added to Clinic Duty for review.

If this is sorta gets approved. I can do it as part of migrating tables to abstract schema (T230428: Migrate tables.sql to abstract schema) but first I need your abstract approval.

Change 619155 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Migrate slot_roles and content_models to abstract schema

https://gerrit.wikimedia.org/r/619155

Change 619155 merged by jenkins-bot:
[mediawiki/core@master] Migrate slot_roles and content_models to abstract schema

https://gerrit.wikimedia.org/r/619155

Ladsgroup claimed this task.