WIP: chore(deps): upgrade pgx to v5

[janisz]

Description

pgx follows semantic versioning for the documented public API on stable releases.
v5 is the latest stable major version.

• Security flaw in dependency library for UUID generation go-gorm/postgres#139
• Upgrade to pgx and pgxpool v5 #4676

Important changes in v5:

• CommandTag is a type
• handling arrays – we have code that convert []TextArray but now it's just a []string so only filtering part is needed.
• handling null values in jsonb there was 'null' but now it's null so I added a change to use {} with pgutils.EmptyOrMap

Checklist

• Investigated and inspected CI test results
• Unit test and regression tests added
• Evaluated and added CHANGELOG entry if required
• Determined and documented upgrade steps
• Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

CI

[roxbot]

Images are ready for the commit at 44009eb.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-541-g44009eb6f0.

[stehessel]

I think you need some more fixes in the unit tests because of this change:

CommandTag is now an opaque type instead of directly exposing an underlying []byte.

See for example

=== RUN   TestAlertDataStore/TestConnExec
    conn_test.go:106: 
        	Error Trace:	/__w/stackrox/stackrox/pkg/postgres/tests/conn_test.go:106
        	Error:      	Expected nil, but got: pgconn.CommandTag{s:""}
        	Test:       	TestAlertDataStore/TestConnExec
    conn_test.go:112: 
        	Error Trace:	/__w/stackrox/stackrox/pkg/postgres/tests/conn_test.go:112
        	Error:      	Expected nil, but got: pgconn.CommandTag{s:""}
        	Test:       	TestAlertDataStore/TestConnExec

[janisz]

Indeed the handling or arrays also changed causing problems https://github.com/jackc/pgx/blob/v5.4.2/CHANGELOG.md?plain=1

[janisz]

Current dependencies on/for this PR:

• master
  • PR test(pg): add test for nil map serialisation #6937
    • PR test(pg): log queries in tests #6938
    • PR refactor(pg): store nil map as empty {} #6929
      • PR refactor(pg): extract input values template #7108
        • PR refactor(pg): remove unused context arg #7051
      • PR WIP: chore(deps): upgrade pgx to v5 #6915 👈
        • PR chore(deps): upgrade pgx to v5 #6916

This comment was auto-generated by Graphite.

[dashrews78]

I haven't had a chance to look at this in detail yet, but I do have a question. What level of testing have we done on this? Reason I ask is that this looks like a significant upgrade and we've seen some things in the past where pgx took some liberties with how they do things. My concern is they adjusted some behaviors and we won't necessarily see them with just vanilla CI. Things we found during scale and chaos type testing would be missed in CI.

Additionally should we go through the v5 changelog and write tickets to step up to some of their improved flows such as the new way batching works, etc?

[stehessel]

I haven't had a chance to look at this in detail yet, but I do have a question. What level of testing have we done on this? Reason I ask is that this looks like a significant upgrade and we've seen some things in the past where pgx took some liberties with how they do things. My concern is they adjusted some behaviors and we won't necessarily see them with just vanilla CI. Things we found during scale and chaos type testing would be missed in CI.

Agreed, my review was mostly just looking at the changes surface level, but I don't believe we should bump this just because the unit tests pass. I don't know how these upgrades have been done in the past, but I'd be more comfortable with some larger scale tests on the new version.

[janisz]

I concur we need some better test plan. So far I only manually verified some generated queries and besides of map nil serialization I haven't seen any differences. But queries are just a small part of what pgx does. I can enable run UI tests as they run more gql queries.

Unfortunately there is no upgrade guide for pgx. I was under the impression it's mostly backward compatible. E.g.: gorm just merged it and released as bugfix release and we already are using this version.

Maybe better time to merge it will be just after the release so we will have more time to catch possible bugs. We will need to bump it at some point.

[janisz]

@connorgorman @mtodor could you help with perf/scale tests?

[janisz]

/retest

[janisz]

/test all

[janisz]

/test all

[janisz]

/test all

[janisz]

/retest

[openshift-ci]

@janisz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocp-4-10-compliance-e2e-tests	44009eb	link	false	/test ocp-4-10-compliance-e2e-tests
ci/prow/gke-upgrade-tests	44009eb	link	false	/test gke-upgrade-tests
ci/prow/ocp-4-10-qa-e2e-tests	44009eb	link	false	/test ocp-4-10-qa-e2e-tests
ci/prow/ocp-4-13-core-bpf-qa-e2e-tests	44009eb	link	false	/test ocp-4-13-core-bpf-qa-e2e-tests
ci/prow/ocp-4-13-qa-e2e-tests	44009eb	link	false	/test ocp-4-13-qa-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[janisz]

Closing, we can reopen it once we have a proper test plan for this upgrade.