chore: lock static query map [DET-3633, DET-4011]

[stoksc]

Description

This change locks concurrent access to the queries map in db.PgDB. Technically two concurrent queries could come in, both see the value doesn't exist, and both populate it (the entire critical section isn't locked), but they would overwrite each other with the same value and the write lock stops the go runtime from barfing over concurrent map writes. I think being able to use RLock for the read portion is worth this, since we'll almost never have writes.

Test Plan

Commentary (optional)

Checklist

• User-facing API changes need the "User-facing API Change" label.
• Release notes should be added as a separate file under docs/release-notes/.
  See Release Note for details.

[rb-determined-ai]

As we discussed before, this is kind of a bandaid solution for what really is sort of a silly and unnecessary lazy-loading strategy that we have. I'm ok with doing it this way for now since it brings us to being functionally correct very quickly, but what is the long-term plan?

You mentioned using postgres stored procedures, how much work would that be? Do you think it would be worth it?

[rb-determined-ai]

question: should we be using defer to avoid situations where a panic of some sort happens, causing the unlock to never happen? (the question applies here and below)

[stoksc]

If we defer q.RUnlock() then I think if !ok we could have a deadlock trying to acquire q.Lock(). We could defer q.Unlock() a couple lines below, but I don't see any way a panic could happen (since we're using the ok return from the map).

[stoksc]

@rb-determined-ai

this is kind of a bandaid solution for what really is sort of a silly and unnecessary lazy-loading strategy that we have

yep.

I'm ok with doing it this way for now since it brings us to being functionally correct very quickly, but what is the long-term plan?

I'm doing this too, because since the new api uses this code path, it seems to be occurring more and more often. Going to bring the long term solution up for discussion.

You mentioned using postgres stored procedures, how much work would that be? Do you think it would be worth it?

Maybe I can take my tech debt week to clean up postgres.go, I'll run some ideas by #infra-ag.

[neilconway]

question: do we really think that this is simpler than eager-loading all the queries into the map during master startup?

[neilconway]

FWIW, as a practical matter, I'm not sure this optimization is warranted -- i.e.,

q.Lock()
defer q.Unlock()
query, ok := q.queries[queryName]
if !ok {
    query = string(etc.MustStaticFile(...))
    q.queries[queryName] = query
}
return query

would probably be fine in practice.

[stoksc]

I thought that for this specific case with 10-20 writes and unbounded reads, this would be better. But I did benchmark them for fun and they were very close. I can change it since it's so much simpler.

[stoksc]

Went ahead and changed this, since it's impact is minimal in comparison to the db query associated with each call.

[rb-determined-ai]

I agree with Neil that it seems better to eagerly load the postgres queries when we create the PgDB, but if you have a lot on your plate, I'm not terribly concerned.

In any case, I think we should file a ticket for a longer-term solution and we should probably land this today, so this bug is fixed for the release.

[stoksc]

question: do we really think that this is simpler than eager-loading all the queries into the map during master startup?

@neilconway I do, just because if we need to load them all at startup, we need to be supplied a list of all the queries we preload. So the initial change would require finding all the queries that are loaded this way and then adding any future queries to a list to preload. Forgetting this would be a runtime error; I'd expect to hit it in testing but also I'd rather not rely on that.

That said, I really don't like this pattern, I'm just trying to fix this as simply as possible, saving a larger refactor for after a discussion.

[neilconway]

I'm just trying to fix this as simply as possible, saving a larger refactor for after a discussion.

sgtm.