CBL-6131: Race creating the expiration column in a collection table

[callumbirks]

Fixed by surrounding the column check with the same file lock used when adding the column.

One can verify that the test Create collection concurrently fails if you revert the lock change.

[callumbirks]

@jianminzhao
I think wrapping the check in a lock is preferable to checking the error, because:
a. The check is guarded by the state _hasExpirationColumn, so this lock should only be entered once by each collection instance.
b. Checking the error would require the error string being correct and never changing. Because the Error has code = SQLITE_ERROR, we have to rely on checking the message string to use that alternative.

[snej]

The fix looks good, I just disagree with the test.

[jianminzhao]

The new version is okay with me: doing an extra mayHaveExpiration inside the transaction, since the positive return is always good to omit the following transaction.

[callumbirks]

The fix looks good, I just disagree with the test.

@snej I guess you mean as in this is not behaviour we support? I agree, but as this is something that users can do (and now have done), I think its important to test that we keep the logical consistency.

Unless you have something to suggest to change / improve the test?

EDIT: Sorry, just saw the code comment above.

[cbl-bot]

Code Coverage Results:

Type	Percentage
branches	67.91
functions	79.26
instantiations	35.26
lines	79.26
regions	75.15

[jianminzhao]

Can you insert a new version before existing versions? How would a version at 500 or 501 get upgraded? There should be a test, I think.

[callumbirks]

Any database at version >=500 already has the expiration column, because addExpiration was added to KeyStore::reopen in the same commit that WithDeletedTable (500) migration was added.

[jianminzhao]

So you actually try to retrofit into current upgrade path. This relies on very precise understanding of other part of code and the schema.
Recognizing this fact, we can also treat it as a new version, but because of the above fact, do
ALTER TABLE ..... IF NOT EXISTS
I am little concerned of retrofitting a new version into the history

[callumbirks]

ALTER TABLE ..... IF NOT EXISTS

There is no such syntax. We need to check the existence of the column manually

[jianminzhao]

If you check the existence, do we still need to insert a new version into past history? This is the new version, after this new version, we will know schema has the expiration table at the time of creation.

[jianminzhao]

What's the code path that supports your statement? Is it in the same transaction of the upgrade transaction? Is it possible that some failure occurs outside the upgrade transaction that arrives following state:

• version bumped 500
• some keystore not opened successfully and expiration not added successfully.
  Since you removed the addExpiration completely, there is no way to have the column.

[callumbirks]

SQLiteKeyStore::reopen calls addExpiration (on the main branch).
So if a database is version >=500, every collection which has been used since it upgraded to version 500 has the expiration column.

[jianminzhao]

"which has been used since ..." -- what if not used?

[callumbirks]

If not used then I'm not sure

[snej]

I don't think it's worth trying to optimize. Seems like you need to make WithExpiration = 502 and always do it (once) for every database, just to make sure.