Update on postgres impl is inefficient

[thockin]

Edit: This was originally about updates being non-atomic but I was wrong.

There does still seem to be a significant opportunity to optimize.

I am not a pgsql expert, so please correct me if I am wrong.

Jet Chiang (@supreme-gg-gg)

Roughly, it looks like:

Query:
          SELECT proto FROM actors
          WHERE atespace = $1 AND name = $2
          FOR UPDATE`, atespace, name

Unmarshal
call mutate(oldVal)
validate
Marshal

Query:
          UPDATE actors
          SET version = $1, proto = $2
          WHERE atespace = $3 AND name = $4`,
          dbActor.GetMetadata().GetVersion(), protoBytes, atespace, name

I am not a PGSQL expert but I thought "FOR UPDATE" requires a txn. Also gemini says not to use "FOR UPDATE" but "FOR NO KEY UPDATE". IIUC, also: row-locks are not auto-released, so we need to handle a crash mid-txn.

I assumed I would find something more like:

UPDATE actors
          SET version = $5, proto = $6
          WHERE atespace = $1 AND name = $2 AND uid = $3 AND version = $4,
          atespace, name, expectUid, expectVersion, newVersion, protoBytes

...and then check that it affected exactly 1 row.

[EItanya]

I dug further into this last night and it appears as though the update is transaction safe, but there are definitely a couple of optimizations we could make.

1. We use FOR UPDATE even though no keys are updated so we can indeed switch to FOR NO KEY UPDATE.
2. As Joe Betz (@jpbetz) pointed out, we hold the transaction across client side marshal/unmarshal which absolutely presents potential performance issues.

[jpbetz]

+100. I highly recommend switching entirely away from the pessimistic locking approach implemented for writes and switching to an optimistic locking approach like Tim Hockin (@thockin) describes. For the target scale numbers I've seen, the write path needs to be dramatically simplified. You'll probably also need to drop the DB enforced uniqueness check on UID and rely on the server to enforce that uniqueness guarantee.

EDIT: See the scale notes I put here: #940 (comment)

[thockin]

Indeed, I missed the tx, err := p.pool.Begin(ctx) and defer tx.Rollback(ctx)

We can Marshal the new value outside the txn, but as long as we have a txn, we need to do some non-trivial work inside it, right? Again, not a postgres expert, but I did a lot of other DBs back in the day. Moving version and uid to be columns seems like the closest way to get atomic test-and-set, but I can't say what side-effects will be.

I will edit the title of this

[EItanya]

I agree, I think optimistic locking with version numbers would probably be better for performance, seems like an easy win to take that approach across our modification calls.

[thockin]

Given that we have exactly 2 preconditions (uid, version) and are unlikely to add more, it seems OK to me. Having UID as a column gives us an option to do lookups by UID (e.g. retain-after-delete) if we are careful (e.g. can't violate the primary key, so may need a "deleted" field in it)?

[supreme-gg-gg]

In #1023 I've replaced pessimistic locking with optimistic concurrency, we already have version and UID column in most tables in the initial schema. The new update steps look like this:

1. Read the current protobuf plus its uid and version.
2. Unmarshal it and invoke mutate(current).
3. Validate immutable fields and serialize the result.
4. Conditionally write it:
   UPDATE ...
   WHERE uid = $expectedUID AND version = $expectedVersion

This moves marshal/unmarshal out of the transaction