Add safer locking to CreateFieldIfNotExists

[oldmantaiter]

Required for all non-trivial PRs

• Rebased/mergable
• Tests pass
• CHANGELOG.md updated
• Sign CLA (if not already signed)

A deadlock can occur if the field was created while we were waiting for the lock.

[mention-bot]

By analyzing the blame information on this pull request, we identified @jwilder, @benbjohnson and @pauldix to be potential reviewers

[oldmantaiter]

So far, this has stopped the issues I was seeing for #6096

[benbjohnson]

@oldmantaiter That would make sense. It looks like the if block right after m.mu.Lock() isn't being unlocked before returning.

@jwilder We're already doing an RLock() to check the field existence. I'm ok with using defer here since it's not going to happen very often.

[jwilder]

The defer was removed to avoid an allocation. This code is actually called very frequently now. I'd prefer to explicitly unlock in the place where we missed it.

[oldmantaiter]

@jwilder Updated - on that note, let me rebase with a new subject line for the commit (so it makes more sense)

[benbjohnson]

@jwilder The first lock seems like it would get called a lot:

https://github.com/influxdata/influxdb/pull/6462/files#diff-5794b17db5ab55866ed4ee776df9a0eaR561

The second one would only get called on new fields. It doesn't seem like that would happen very often.

[jwilder]

@benbjohnson That's a good point. When I was looking at this code, it was in the context of speeding up shard loading when the DB starts up and new fields were getting created frequently. defer here might be fine since shard loading code has changed quite a bit and the contentious part should be the RLock now.

I'd still prefer the explicit unlock, but defer is also OK w/ me.

[benbjohnson]

@jwilder I spent a full day trying to track this issue down (and was still unsuccessful). If we don't have a specific performance need for not using defer then I would strongly recommend using it.

[jwilder]

@benbjohnson OK w/ me.

[mark-rushakoff]

There's a bit of discussion on defer performance at golang/go#14939.

[jwilder]

@oldmantaiter I didn't realize you had already updated the PR to remove the defer while we were discussing it. @benbjohnson would rather have your original change (using defer) and after discussing it, I'm OK w/ using the defer as well.

If you could switch the unlock back to using defer, then we can get this PR merged.

[oldmantaiter]

@jwilder Sure, I'll bring back the defer.

[benbjohnson]

Thanks @oldmantaiter! 👍