Page MenuHomePhabricator

Consider limiting SqlBagOStuff makeKey logic
Closed, ResolvedPublic

Description

The schema for the objectcache table specifies keyname as varbinary(255).

  • The default BagOStuff::makeKeyInternal does not assume a maximum key length.
  • The Memcached Bag is aware of Memcache's key limit (~ 255 bytes), and enforces it.
  • The WinCache Bag is aware is wancache's key limit (~ 150 chars, or bytes?), and enforces it.

Seems like SqlBagOStuff may need to do this as well.

Event Timeline

Krinkle renamed this task from Review SqlBagOStuff makeKey limit to Consider limiting SqlBagOStuff makeKey logic.Jun 2 2019, 3:14 AM
Krinkle added a subscriber: Anomie.

This might be a good one as well. I'm not sure what the failure mode is right now if e.g. parser cache or CACHE_DB access provides a key that is too long.

I'm not sure what the failure mode is right now if e.g. parser cache or CACHE_DB access provides a key that is too long.

For MySQL with strict mode is off, it'll truncate on writes but then not find the match on reads. OTOH, the truncated write might collide with a key that's exactly 255 bytes long.

For MySQL with strict mode is on, it'll raise an error on writes.

For PostgreSQL, it would raise an error if the schema were using varchar(255). But whoever created the PG schema used TEXT almost everywhere instead (including here), which has a limit of around 1 GB.

For SQLite, it doesn't enforce the declared limit. It'll raise an error if things exceed SQLite's internal limit on string length.

Change 575073 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] SqlBagOStuff: Add a limit to key length.

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

Change 575073 merged by jenkins-bot:
[mediawiki/core@master] SqlBagOStuff: Add a limit to key length

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

@AMooney, I understand that this ticket is resolved. Should it be in the Done column instead of the Waiting for Review one?