Skip to content
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

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

Merged
merged 3 commits into from
Aug 18, 2021
Merged

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

merged 3 commits into from
Aug 18, 2021

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Aug 9, 2021

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

@rafiss rafiss requested a review from a team August 9, 2021 18:34
@rafiss rafiss requested review from a team as code owners August 9, 2021 18:34
@rafiss rafiss requested a review from a team August 9, 2021 18:34
@rafiss rafiss requested review from a team as code owners August 9, 2021 18:34
@rafiss rafiss requested review from adityamaru and stevendanna and removed request for a team August 9, 2021 18:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 55 of 55 files at r1, all commit messages.
Reviewable status: :shipit: 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?

Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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
Copy link
Collaborator Author

rafiss commented Aug 10, 2021

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 rafiss force-pushed the bump-to-pgx4 branch 9 times, most recently from ba86c07 to f99ce2a Compare August 12, 2021 23:22
@rafiss
Copy link
Collaborator Author

rafiss commented Aug 12, 2021

@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:

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

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

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
Copy link
Collaborator Author

rafiss commented Aug 13, 2021

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

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Uff, apologies for the delay here. Thanks for digging into this. I think your fix looks reasonable. This test has been a thorn in our side for a bit as the existing coordination is a bit suspected. I noted a small nitpick but overall it :LGTM:

Reviewed 1 of 35 files at r3, 1 of 25 files at r9.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @knz, and @rafiss)


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

		resume <- struct{}{}
		dataExpiredRows := <-changefeedInit
		defer closeFeed(t, dataExpiredRows)

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


pkg/ccl/changefeedccl/testfeed_test.go, line 129 at r10 (raw file):

}

var _ cdctest.TestFeed = (*sinklessFeed)(nil)

Thanks for adding these!

Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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

Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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
@rafiss
Copy link
Collaborator Author

rafiss commented Aug 17, 2021

thanks for the reviews all!

bors r=otan,knz,stevendanna

@craig
Copy link
Contributor

craig bot commented Aug 18, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 18, 2021

Build succeeded:

@craig craig bot merged commit 472008d into cockroachdb:master Aug 18, 2021
@rafiss rafiss deleted the bump-to-pgx4 branch August 18, 2021 14:02
@rail rail mentioned this pull request Nov 30, 2021
rail added a commit to rail/cockroach that referenced this pull request Dec 1, 2021
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
rail added a commit to rail/cockroach that referenced this pull request Dec 1, 2021
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
craig bot pushed a commit that referenced this pull request Dec 1, 2021
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 <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>
Co-authored-by: Tommy Reilly <[email protected]>
Co-authored-by: Oleg Afanasyev <[email protected]>
blathers-crl bot pushed a commit that referenced this pull request Dec 1, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: refactor pgx_helpers to use pgx/v4
5 participants