sql: reset indexedTypeFormatter on FmtCtx close#73095
sql: reset indexedTypeFormatter on FmtCtx close#73095craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
otan
left a comment
There was a problem hiding this comment.
LGTM, you could optionally add a test to format_test.go that creates a new FmtCtx, uses indexedTypeFormatter, Close the FmtCtx, refetch from the FmtCtx pool and assert that it does not apply the same thing again.
|
Wonder how often this bug is triggered - perhaps worth a release note |
I think the bug is easily reproducible if you just sequence the order of tables and udts are restored. |
31624bb to
cc55cfa
Compare
Updated the release note, gonna leave out the test since we can't test the unexported field anyway. |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @yuzefovich)
pkg/sql/sem/tree/format.go, line 633 at r1 (raw file):
ctx.tableNameFormatter = nil ctx.placeholderFormat = nil ctx.indexedTypeFormatter = nil
Consider replacing all this nilling out with something like
ctx.Buffer.Reset()
*ctx = FmtCtx {
Buffer: ctx.Buffer,
// other fields that we want to keep when putting ctx into the pool
}
fmtCtxPool.Put(ctx)
|
pkg/sql/sem/tree/format.go, line 633 at r1 (raw file): Previously, yuzefovich (Yahor Yuzefovich) wrote…
Ooh nice |
Previously indexedTypeFormatter wouldn't be reset when FmtCtx.Close() was called causing the indexedTypeFormatter function to potentially be reused. Release note (enterprise change): Fix a bug where restore can sometimes map OIDs to invalid types in certain circumstances containing UDTs.
cc55cfa to
d0e09f1
Compare
Good idea - did that instead, there's nothing else that we want to keep other than ctx.Buffer I believe. |
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/format.go, line 633 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Good idea - did that instead, there's nothing else that we want to keep other than ctx.Buffer I believe.
i guess technically there's a difference, since it changes the reference and the old one has to be garbage collected, but maybe that's fine? unless the whole reason we were pooling the FmtCtx's was to avoid having to do GCs?
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
pkg/sql/sem/tree/format.go, line 633 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i guess technically there's a difference, since it changes the reference and the old one has to be garbage collected, but maybe that's fine? unless the whole reason we were pooling the FmtCtx's was to avoid having to do GCs?
My guess is that the pooling was introduced in order to keep the contents of ctx.Buffer (bytes.Buffer.buf) for reuse, and that is retained with this approach.
I believe because bytes.Buffer is stored by value in FmtCtx, then there is no "old reference" to be GCed too.
|
TFTR! |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from d0e09f1 to blathers/backport-release-21.1-73095: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
not important enough to change, but for the future IMO this should should be |
Previously indexedTypeFormatter wouldn't be reset when
FmtCtx.Close() was called causing the indexedTypeFormatter
function to potentially be reused.
Release note (enterprise change): Fix a bug where restore can sometimes map OIDs
to invalid types in certain circumstances containing UDTs.