-
Notifications
You must be signed in to change notification settings - Fork 621
fix: Don't scan profile events if listener is not set #1686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e3f32ec
ce6ffb5
30c4d80
df62ac3
d80b272
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,3 @@ | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| package clickhouse | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -185,11 +184,14 @@ func (c *connect) handle(ctx context.Context, packet byte, on *onProcess) error | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| c.debugf("[table columns]") | ||||||||||||||||||||||||||||||||||||||||
| case proto.ServerProfileEvents: | ||||||||||||||||||||||||||||||||||||||||
| events, err := c.profileEvents(ctx) | ||||||||||||||||||||||||||||||||||||||||
| scanEvents := on.profileEvents != nil | ||||||||||||||||||||||||||||||||||||||||
| events, err := c.profileEvents(ctx, scanEvents) | ||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| on.profileEvents(events) | ||||||||||||||||||||||||||||||||||||||||
| if scanEvents { | ||||||||||||||||||||||||||||||||||||||||
| on.profileEvents(events) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+187
to
+194
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This way we don't have to change the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the comment @kavirajk, I tried that initially but it appears we still need to read the block from the server in If we could do something like this in queries That would be the best I think, but this is probably a bigger change and needs to be done in https://github.com/ClickHouse/ClickHouse
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regardless of having an option to disable profile events in the session, I think it's good for the client to skip scanning them if the listener is not registered
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree! thanks for the clarification @erezrokah 👍 I don't think we have to block this PR for the server settings to be available. |
||||||||||||||||||||||||||||||||||||||||
| case proto.ServerLog: | ||||||||||||||||||||||||||||||||||||||||
| logs, err := c.logs(ctx) | ||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
|
|
||
| package clickhouse | ||
|
|
||
| import ( | ||
|
|
@@ -18,12 +17,16 @@ type ProfileEvent struct { | |
| Value int64 | ||
| } | ||
|
|
||
| func (c *connect) profileEvents(ctx context.Context) ([]ProfileEvent, error) { | ||
| func (c *connect) profileEvents(ctx context.Context, scanEvents bool) ([]ProfileEvent, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may not need to change this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replied, I did that initially but it appears we have to consume the data from the server otherwise the querying fails |
||
| block, err := c.readData(ctx, proto.ServerProfileEvents, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| c.debugf("[profile events] rows=%d", block.Rows()) | ||
| if !scanEvents { | ||
| c.debugf("[profile events] skipping scan") | ||
| return nil, nil | ||
| } | ||
| var ( | ||
| events []ProfileEvent | ||
| names = block.ColumnsNames() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package issues | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/ClickHouse/clickhouse-go/v2" | ||
| "github.com/ClickHouse/clickhouse-go/v2/tests" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func benchmark1685(ctx context.Context, conn clickhouse.Conn) error { | ||
| for i := 0; i < 10_000; i++ { | ||
| err := conn.Exec(ctx, | ||
| "INSERT INTO test_xxxx VALUES (?, ?, [1, 2, 3, 4, 5, 6, 7, 8, 9], now())", | ||
| i, "Golang SQL database driver", false) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func BenchmarkIssue1685(b *testing.B) { | ||
| conn, err := tests.GetConnectionTCP("issues", nil, nil, nil) | ||
| ctx := context.Background() | ||
| require.NoError(b, err) | ||
|
|
||
| const ddl = `CREATE TABLE test_xxxx (Col1 UInt64, Col2 String, Col3 Array(UInt8), Col4 DateTime) Engine ReplacingMergeTree() ORDER BY Col1` | ||
| err = conn.Exec(ctx, ddl) | ||
| require.NoError(b, err) | ||
| defer func() { | ||
| conn.Exec(ctx, "DROP TABLE IF EXISTS test_xxxx") | ||
| }() | ||
|
|
||
| for k := 0; k < b.N; k++ { | ||
| require.NoError(b, benchmark1685(ctx, conn)) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.