pgwire: remove readTimeoutConn in favor of a channel by rafiss · Pull Request #124373 · cockroachdb/cockroach

rafiss · 2024-05-17T23:01:11Z

Rather than using a connection that polls for the context being done every second, we now spin up an additional goroutine that blocks until the connection context is done, or the drain signal was received.

I wrote a simple benchmark of the idle connection loop to generate CPU profiles.

With the old readTimeoutConn:

With the new goroutine approach:

There's definitely less noise and overhead.

fixes #25585
Release note: None

cockroach-teamcity · 2024-05-17T23:01:20Z

This change is

yuzefovich

Thanks for looking into this! The change overall seems good to me.

I'm not too familiar with this code, so I want to double check my understanding. IIUC currently for each connection we have two goroutines:

the "reader" goroutine that continuously loops in serveImpl and see what the client is requesting us to do
and the "processor" goroutine for the connExecutor that executes commands added into stmtBuf by the "reader" goroutine.

This PR introduces a third "watchdog" goroutine that monitors the state for cancellation and draining. In other words, we make a trade-off between 1) periodically forcing net.Conn to wake up in the "reader" due to a read deadline to see whether the "reader" should quit and 2) a separate goroutine that only monitors things with no read deadline set for the "reader". In practice, this "watchdog" goroutine should be idle most of the time, so it should remove some system calls. Does this sound right?

yuzefovich · 2024-05-18T20:49:24Z

pkg/sql/pgwire/server.go

@@ -945,6 +942,35 @@ func (s *Server) serveImpl(
 	authOpt authOptions,
 	sessionID clusterunique.ID,
 ) {


nit: the last part of the comment for serveImpl needs an update.

yuzefovich · 2024-05-18T20:50:39Z

pkg/sql/pgwire/server.go

+			tcpConn, _ = underConn.(*net.TCPConn)
+		}
+		if tcpConn == nil {
+			_ = c.conn.Close()


We didn't close the connection in both directions before this patch. Based on the comment to serveImpl, it seems that its the connExecutor's job to do so. Why add this here?

hm good point. it seems safer to only close the read side.

this matches the previous behavior, where if the context was cancelled, then the readTimeoutConn would start returning errors when Read is called:

cockroach/pkg/sql/pgwire/server.go

Line 1039 in ceb19c5

typ, n, err := c.readBuf.ReadTypedMsg(&c.rd)

that should cause the main loop of the reader goroutine to exit here:

cockroach/pkg/sql/pgwire/server.go

Lines 1216 to 1225 in ceb19c5

// If we can't read data because of any one of the following conditions,

// then we should break:

// 1. the connection was closed.

// 2. the context was canceled (e.g. during authentication).

// 3. we reached an arbitrary threshold of repeated errors.

if netutil.IsClosedConnection(err) ||

errors.Is(err, context.Canceled) ||

repeatedErrorCount > maxRepeatedErrorCount {

break

}

although, thinking about it more, the risk is that if the type switch no longer works as expected (like for example, there's a new implementation of net.Conn to deal with later), then not having a default case would make the connection hang forever.

maybe it's safer to keep the Close as a fallback? WDYT?

yuzefovich · 2024-05-18T20:53:20Z

pkg/sql/pgwire/server.go

+			// DrainRequest. This will make the processor quit whenever it finds a
+			// good time.
+			_ /* err */ = c.stmtBuf.Push(ctx, sql.DrainRequest{})
+			<-ctx.Done()


nit: a comment (that eventually the drain request will be processed by the server and all connections will be closed, which is achieved by cancelling the context) would be helpful.

rafiss

This PR introduces a third "watchdog" goroutine that monitors the state for cancellation and draining. In other words, we make a trade-off between 1) periodically forcing net.Conn to wake up in the "reader" due to a read deadline to see whether the "reader" should quit and 2) a separate goroutine that only monitors things with no read deadline set for the "reader". In practice, this "watchdog" goroutine should be idle most of the time, so it should remove some system calls. Does this sound right?

yes sounds right. it's worth noting that a goroutine that does nothing apart from wait on a channel is very efficient in go. the scheduler should never try to run it.

rafiss · 2024-05-20T16:49:59Z

pkg/sql/pgwire/server.go

@@ -945,6 +942,35 @@ func (s *Server) serveImpl(
 	authOpt authOptions,
 	sessionID clusterunique.ID,
 ) {


rafiss · 2024-05-20T16:52:20Z

pkg/sql/pgwire/server.go

+			// DrainRequest. This will make the processor quit whenever it finds a
+			// good time.
+			_ /* err */ = c.stmtBuf.Push(ctx, sql.DrainRequest{})
+			<-ctx.Done()


rafiss · 2024-05-20T17:01:21Z

pkg/sql/pgwire/server.go

+			tcpConn, _ = underConn.(*net.TCPConn)
+		}
+		if tcpConn == nil {
+			_ = c.conn.Close()


hm good point. it seems safer to only close the read side.

this matches the previous behavior, where if the context was cancelled, then the readTimeoutConn would start returning errors when Read is called:

cockroach/pkg/sql/pgwire/server.go

Line 1039 in ceb19c5

typ, n, err := c.readBuf.ReadTypedMsg(&c.rd)

that should cause the main loop of the reader goroutine to exit here:

cockroach/pkg/sql/pgwire/server.go

Lines 1216 to 1225 in ceb19c5

// If we can't read data because of any one of the following conditions,

// then we should break:

// 1. the connection was closed.

// 2. the context was canceled (e.g. during authentication).

// 3. we reached an arbitrary threshold of repeated errors.

if netutil.IsClosedConnection(err) ||

errors.Is(err, context.Canceled) ||

repeatedErrorCount > maxRepeatedErrorCount {

break

}

yuzefovich

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/pgwire/server.go line 968 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

although, thinking about it more, the risk is that if the type switch no longer works as expected (like for example, there's a new implementation of net.Conn to deal with later), then not having a default case would make the connection hang forever.

maybe it's safer to keep the Close as a fallback? WDYT?

Sounds reasonable to me.

I traced where Close is called currently, and I think it's a few layers up in the defer in in the async task in netutil.TCPServer.ServeWith, so the comment (that "serveImpl always closes the network connection before returning") is misleading before your patch but will actually become closer to reality after the patch. It might be worth adjusting the comment.

yuzefovich

Looks like the updates haven't been pushed yet?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

pkg/sql/pgwire/server.go line 968 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Sounds reasonable to me.

I traced where Close is called currently, and I think it's a few layers up in the defer in in the async task in netutil.TCPServer.ServeWith, so the comment (that "serveImpl always closes the network connection before returning") is misleading before your patch but will actually become closer to reality after the patch. It might be worth adjusting the comment.

I was about to push the change, then realized that another fallback option is to call conn.SetReadDeadline(now()), which might be less disruptive. I'll do that instead.

yuzefovich

Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/pgwire/server.go line 968 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

I was about to push the change, then realized that another fallback option is to call conn.SetReadDeadline(now()), which might be less disruptive. I'll do that instead.

Nice, I like it.

Rather than using a connection that polls for the context being done every second, we now spin up an additional goroutine that blocks until the connection context is done, or the drain signal was received. Release note: None

This benchmark was only useful for capturing a CPU profile, it doesn't measure anything else. Release note: None

rafiss · 2024-05-20T19:57:31Z

tftr!

bors r=yuzefovich

craig · 2024-05-20T20:16:19Z

Build failed:

lint

rickystewart · 2024-05-20T20:26:52Z

bors r=yuzefovich

craig · 2024-05-20T21:14:16Z

Build succeeded:

rafiss requested a review from yuzefovich May 17, 2024 23:01

rafiss requested review from a team as code owners May 17, 2024 23:01

rafiss requested a review from a team May 17, 2024 23:01

rafiss force-pushed the remove-readTimeoutConn branch from ce2e425 to 17e388a Compare May 18, 2024 08:10

yuzefovich reviewed May 18, 2024

View reviewed changes

rafiss commented May 20, 2024

View reviewed changes

yuzefovich reviewed May 20, 2024

View reviewed changes

rafiss requested a review from yuzefovich May 20, 2024 18:14

yuzefovich reviewed May 20, 2024

View reviewed changes

rafiss commented May 20, 2024

View reviewed changes

rafiss force-pushed the remove-readTimeoutConn branch from 17e388a to d7150f9 Compare May 20, 2024 18:29

rafiss requested a review from yuzefovich May 20, 2024 18:30

yuzefovich approved these changes May 20, 2024

View reviewed changes

rafiss added 2 commits May 20, 2024 14:54

pgwire: remove readTimeoutConn in favor of a channel

a65fea7

Rather than using a connection that polls for the context being done every second, we now spin up an additional goroutine that blocks until the connection context is done, or the drain signal was received. Release note: None

pgwire: remove unneeded test

1351385

This benchmark was only useful for capturing a CPU profile, it doesn't measure anything else. Release note: None

rafiss force-pushed the remove-readTimeoutConn branch from d7150f9 to 1351385 Compare May 20, 2024 18:54

craig bot merged commit fb4ee19 into cockroachdb:master May 20, 2024

yuzefovich mentioned this pull request May 21, 2024

pkg/sql/logictest/tests/fakedist-vec-off/fakedist-vec-off_test: TestLogic_synthetic_privileges failed #124449

Closed

rafiss deleted the remove-readTimeoutConn branch May 21, 2024 13:54

yuzefovich mentioned this pull request Jun 4, 2024

pkg/sql/logictest/tests/fakedist-vec-off/fakedist-vec-off_test: TestLogic_synthetic_privileges failed #125040

Closed

dhartunian mentioned this pull request Jun 28, 2024

pkg/ccl/serverccl/statusccl/statusccl_test: TestTenantStatusAPI failed #125404

Closed

yuzefovich mentioned this pull request Nov 4, 2024

testccl: handle different type of connection closed errors #124571

Merged

tbg mentioned this pull request Jan 21, 2025

release-24.1: pgwire: synchronize readTimeoutConn's timeout with max drain wait #139477

Merged

rafiss mentioned this pull request Jan 21, 2025

release-23.2: pgwire: synchronize readTimeoutConn's timeout with max drain wait #139479

Merged

	// If we can't read data because of any one of the following conditions,
	// then we should break:
	// 1. the connection was closed.
	// 2. the context was canceled (e.g. during authentication).
	// 3. we reached an arbitrary threshold of repeated errors.
	if netutil.IsClosedConnection(err) \|\|
	errors.Is(err, context.Canceled) \|\|
	repeatedErrorCount > maxRepeatedErrorCount {
	break
	}

Conversation

rafiss commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented May 17, 2024

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented May 20, 2024

Uh oh!

craig bot commented May 20, 2024

Uh oh!

rickystewart commented May 20, 2024

Uh oh!

craig bot commented May 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rafiss commented May 17, 2024 •

edited

Loading

rafiss May 20, 2024 •

edited

Loading