go.mod: update to pgx v4.13 by rafiss · Pull Request #68608 · cockroachdb/cockroach

rafiss · 2021-08-09T18:34:32Z

fixes #62840

This also required an update to cockroach-go/v2 to get the crdbpgx
transaction retry helper.

The majority of changes here are because pgx now requires the context to
be passed in always.

Other changes:

The way that pgx prepares statements has changed -- to disable
automatic prepared statements, the PreferSimpleProtocol setting must be
enabled when making the pgxpool.
The noprepare setting was removed from workload. This functionality
is not supported by pgx any more. pgx will either use prepared statements and
cache them automatically or use the simple protocol.
sqlproxy tests had to be modified since pgx now has more
fallback logic to retry connections when using an sslmode of prefer
or allow. pgx also now tries to resolve localhost as the IPv6 address
:::1, which isn't well-supported by sqlproxy, so IP resolution was disabled
for sqlproxy tests.
CHANGEFEED tests had to be changed since pgx.Conn.Query now immediately
tries fetching the results, which didn't work with the synchronization that was added
to TestChangefeedDataTTL.
COPY was fixed to correctly ignore CopyData after encountering an error.

Release note: None

cockroach-teamcity · 2021-08-09T18:34:40Z

This change is

otan

some test failures but they look like you'd have to massage them differently, lgtm

knz

Reviewed 55 of 55 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @rafiss, and @stevendanna)

pkg/ccl/backupccl/backup_test.go, line 7297 at r1 (raw file):

				db, err := pgx.ConnectConfig(ctx, connCfg)
				assert.NoError(t, err)
				defer func() { _ = db.Close(ctx) }()

assert.NoError(t, db.Close(ctxo))

pkg/workload/kv/kv.go, line 444 at r1 (raw file):

		}
		defer func() {
			_ = tx.Rollback(ctx)

maybe assert the return value?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @stevendanna)

pkg/ccl/backupccl/backup_test.go, line 7297 at r1 (raw file):

Previously, knz (kena) wrote…

assert.NoError(t, db.Close(ctxo))

done

pkg/workload/kv/kv.go, line 444 at r1 (raw file):

Previously, knz (kena) wrote…

maybe assert the return value?

done

rafiss · 2021-08-10T20:46:04Z

sqlproxy'sTestFailedConnection is failing in CI, but I cannot get it to fail even when running it under stress on a GCE Worker.

Will debug later today or tomorrow.

Update: This was failing because pgx now tries to resolve localhost to the IPv6 address :::1, and sqlproxy does not support IPv6. Updated the test to not use IPv6.

rafiss · 2021-08-12T23:32:56Z

@stevendanna if you have some time would you be able to help me with this?

I'm trying to upgrade our pgx dependency to use v4 across the board, and I've encountered an issue in TestChangefeedDataTTL which it seems like you have touched last.

The test is getting stuck at this line:

cockroach/pkg/ccl/changefeedccl/changefeed_test.go

Line 2328 in ab15e5f

dataExpiredRows := feed(t, f, "CREATE CHANGEFEED FOR TABLE foo")

The reason is because the sinklessFeed is gets stuck when making the changefeed:

cockroach/pkg/ccl/changefeedccl/testfeed_test.go

Line 186 in ab15e5f

c.rows, err = c.conn.Query(create, c.args...)

pgx v4 differs from the old pgx in that it immediately tries to fetch the result when running Query(). That means the synchronization logic in the test won't let the pgx.Conn.Query("CREATE CHANGEFEED ...") statement complete.

Do you have any other ideas on how to synchronize here while still achieving the goals of the test? The other option I thought of is to jump into a lower level and use jackc/pgconn to create the changefeed without trying to pull results from it right away, but that seems like a big refactor and isn't my first choice.

rafiss · 2021-08-13T01:17:09Z

I understand the test a bit better now so I tried to fix the test myself -- @stevendanna when you have a chance, please review the diff in pkg/ccl/changefeedccl/changefeed_test.go

rafiss

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @knz, @rafiss, and @stevendanna)

pkg/ccl/changefeedccl/changefeed_test.go, line 2373 at r10 (raw file):

Previously, stevendanna (Steven Danna) wrote…

[nit] We could fail to clean up this feed if there are errors above in upsertRow() or forceTableGC.

in that case there would have already been a t.Fatal, or otherwise closeFeed itself will call t.Fatal

rafiss

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @knz, @rafiss, and @stevendanna)

pkg/ccl/changefeedccl/changefeed_test.go, line 2373 at r10 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

in that case there would have already been a t.Fatal, or otherwise closeFeed itself will call t.Fatal

ah, i see your point. the execution could stop before reaching this defer. fixing it, nice catch!

This also required an update to cockroach-go/v2 to get the crdbpgx transaction retry helper. The majority of changes here are because pgx now requires the context to be passed in always. This revealed a bug in the COPY state machine. We were not handling errors during COPY correctly, so we now always continue reading until encountering a CopyDone message. Also, the way that pgx prepares statements has changed -- to disable automatic prepared statements, the PreferSimpleProtocol setting must be enabled when making the pgxpool. Finally, sqlproxy tests had to be modified since pgx now has more fallback logic to retry connections when using an sslmode of `prefer` or `allow`. Release note: None

This functionality is not supported by pgx any more. pgx will either use prepared statements or the simple protocol. Release note: None

Release note: None

rafiss · 2021-08-17T23:30:35Z

thanks for the reviews all!

bors r=otan,knz,stevendanna

craig · 2021-08-18T00:32:59Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-08-18T03:12:41Z

Build succeeded:

GitHub CI (Cockroach)

In cockroachdb#68608 we upgraded to pgx v4.13, which changed some interface signatures to accept `context.Context`. The usage was updated almost everywhere, except some spots. Release note: None

72998: flowinfra: refactor locking around FlowRegistry r=yuzefovich a=yuzefovich **flowinfra: refactor locking around FlowRegistry** This commit refactors the locking done in `FlowRegistry`. Previously, the `FlowRegistry`'s mutex (global process-wide) was protecting three pieces of information: - `flows` map that stores which flows have already been scheduled on the node - `flowEntry` objects (information about each scheduled flow) - `InboundStreamInfo` objects (information about endpoints to serve `FlowStream` RPCs). This commit refactors the `InboundStreamInfo` objects so that each has its own mutex. This cleans up the things a bit and allows us more cleanly refactor the code in `ConnectInboundStream` so that we don't perform any gRPC calls (which might be blocking, rarely) while holding the `FlowRegistry`'s mutex. Fixes: #72964. Release note: None **flowinfra: remove the reference to waitGroup from InboundStreamInfo** This limits the coupling of `InboundStreamInfo` and `FlowBase`. Release note: None 73312: Fix compose tests r=rafiss a=rail In #68608 we upgraded to pgx v4.13, which changed some interface signatures to accept `context.Context`. The usage was updated almost everywhere, except some spots. This patch also fixes `defer` usage in a loop. Release note: None 73336: misc: add remote visual studio code support to gceworker r=cucaroach a=cucaroach gceworker.sh code will start the gceworker, touch /.active, start code and run a remote SSH to it. When code exits the .active will be removed. Release note: none 73339: cloud: bump orchestrator to v21.2.2 r=JuanLeon1 a=aliher1911 Release note: None 73345: roachtest: update 22.1 version map to v21.2.2 r=rail a=aliher1911 Release note: None Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Rail Aliiev <rail@iqchoice.com> Co-authored-by: Tommy Reilly <treilly@cockroachlabs.com> Co-authored-by: Oleg Afanasyev <oleg@cockroachlabs.com>

In #68608 we upgraded to pgx v4.13, which changed some interface signatures to accept `context.Context`. The usage was updated almost everywhere, except some spots. Release note: None

rafiss requested a review from a team August 9, 2021 18:34

rafiss requested review from a team as code owners August 9, 2021 18:34

rafiss requested a review from a team August 9, 2021 18:34

rafiss requested review from a team as code owners August 9, 2021 18:34

rafiss requested review from adityamaru and stevendanna and removed request for a team August 9, 2021 18:34

otan approved these changes Aug 9, 2021

View reviewed changes

knz approved these changes Aug 10, 2021

View reviewed changes

rafiss force-pushed the bump-to-pgx4 branch from a472c44 to 5d3cf14 Compare August 10, 2021 16:50

rafiss requested a review from a team as a code owner August 10, 2021 16:50

rafiss commented Aug 10, 2021

View reviewed changes

rafiss force-pushed the bump-to-pgx4 branch 9 times, most recently from ba86c07 to f99ce2a Compare August 12, 2021 23:22

rafiss force-pushed the bump-to-pgx4 branch from f99ce2a to f2d6433 Compare August 13, 2021 01:16

rafiss force-pushed the bump-to-pgx4 branch from f2d6433 to fad3d79 Compare August 16, 2021 23:41

rafiss commented Aug 17, 2021

View reviewed changes

rafiss added 3 commits August 17, 2021 09:54

workload: remove noprepare setting

f446498

This functionality is not supported by pgx any more. pgx will either use prepared statements or the simple protocol. Release note: None

pgwire: refactor if-statement for debugging ease

d4e9113

Release note: None

rafiss force-pushed the bump-to-pgx4 branch from 174c020 to d4e9113 Compare August 17, 2021 13:55

craig bot merged commit 472008d into cockroachdb:master Aug 18, 2021

tbg mentioned this pull request Aug 18, 2021

roachtest: kv95limited-spans/enc=false/nodes=1/cpu=32/splt=0/seq/no-load-splitting failed [tx is closed] #69079

Closed

rafiss deleted the bump-to-pgx4 branch August 18, 2021 14:02

This was referenced Aug 21, 2021

roachtest: kv95limited-spans/enc=false/nodes=1/cpu=32/splt=0/seq/no-load-splitting failed #69189

Closed

workload: large number of DEALLOCATE statements after pgx upgrade #69316

Closed

ajwerner mentioned this pull request Aug 24, 2021

sql: OOM during TPCC with schema changes #69317

Open

stevendanna mentioned this pull request Sep 20, 2021

kv95/enc=false/nodes=3/cpu=96 regression on July 27 #70357

Closed

rail mentioned this pull request Nov 30, 2021

Fix compose tests #73312

Merged

blathers-crl bot mentioned this pull request Dec 1, 2021

release-21.2: Fix compose tests #73360

Merged

blathers-crl bot pushed a commit that referenced this pull request Dec 1, 2021

Fix compose tests

688c001

In #68608 we upgraded to pgx v4.13, which changed some interface signatures to accept `context.Context`. The usage was updated almost everywhere, except some spots. Release note: None

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go.mod: update to pgx v4.13#68608

go.mod: update to pgx v4.13#68608
craig[bot] merged 3 commits intocockroachdb:masterfrom
rafiss:bump-to-pgx4

rafiss commented Aug 9, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Aug 9, 2021

Uh oh!

otan left a comment

Uh oh!

knz left a comment

Uh oh!

rafiss left a comment

Uh oh!

rafiss commented Aug 10, 2021 •

edited

Loading

Uh oh!

rafiss commented Aug 12, 2021

Uh oh!

rafiss commented Aug 13, 2021 •

edited

Loading

Uh oh!

rafiss left a comment

Uh oh!

rafiss left a comment

Uh oh!

rafiss commented Aug 17, 2021

Uh oh!

craig bot commented Aug 18, 2021

Uh oh!

craig bot commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

rafiss commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 9, 2021

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

knz 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!

rafiss commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafiss commented Aug 12, 2021

Uh oh!

rafiss commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafiss 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!

rafiss commented Aug 17, 2021

Uh oh!

craig bot commented Aug 18, 2021

Uh oh!

craig bot commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rafiss commented Aug 9, 2021 •

edited

Loading

rafiss commented Aug 10, 2021 •

edited

Loading

rafiss commented Aug 13, 2021 •

edited

Loading