Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+59 −11
Open

fix: Don't scan profile events if listener is not set #1686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3f32ec
fix: Don't scan profile events if listener is not set
erezrokah Oct 15, 2025
ce6ffb5
chore: cleanup
erezrokah Oct 15, 2025
30c4d80
Merge branch 'main' into fix/profile_events_memory
erezrokah Oct 23, 2025
df62ac3
Update tests/issues/1685_test.go
erezrokah Oct 27, 2025
d80b272
style: format
erezrokah Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions conn_process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

package clickhouse

import (
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
scanEvents := on.profileEvents != nil
events, err := c.profileEvents(ctx, scanEvents)
if err != nil {
return err
}
on.profileEvents(events)
if scanEvents {
on.profileEvents(events)
}
if on.profileEvents != nil {
events, err := c.profileEvents(ctx)
if err != nil {
return err
}
on.profileEvents(events)
break
}
c.debugf("[skip profile events. no handler registered]")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we don't have to change the c.profileEvents method. We just skip on the consumer side itself. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 c.profileEvents(ctx) (we still need to call c.readData(ctx, proto.ServerProfileEvents, false)).

If we could do something like this in queries

....

SETTINGS PROFILE_EVENTS_DISABLED=1

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

Copy link
Contributor Author

@erezrokah erezrokah Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The 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

case proto.ServerLog:
logs, err := c.logs(ctx)
if err != nil {
Expand Down
7 changes: 5 additions & 2 deletions conn_profile_events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

package clickhouse

import (
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may not need to change this profileEvents api at all? . We can skip calling it from the consumer side. See my other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
Expand Down
16 changes: 10 additions & 6 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ func queryOptionsUserLocation(ctx context.Context) *time.Location {
}

func (q *QueryOptions) onProcess() *onProcess {
return &onProcess{
onProcess := &onProcess{
logs: func(logs []Log) {
if q.events.logs != nil {
for _, l := range logs {
Expand All @@ -278,12 +278,16 @@ func (q *QueryOptions) onProcess() *onProcess {
q.events.profileInfo(p)
}
},
profileEvents: func(events []ProfileEvent) {
if q.events.profileEvents != nil {
q.events.profileEvents(events)
}
},
}

profileEventsHandler := q.events.profileEvents
if profileEventsHandler != nil {
onProcess.profileEvents = func(events []ProfileEvent) {
profileEventsHandler(events)
}
}

return onProcess
}

// clone returns a copy of QueryOptions where Settings and Parameters are safely mutable.
Expand Down
39 changes: 39 additions & 0 deletions tests/issues/1685_test.go
View file
Open in desktop
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))
}
}
Loading