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

workload: reintroduce old prepare behavior #69313

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Aug 24, 2021

This reverts commit f446498,
and makes the 'prepare' setting work more closely to how it used to.

This is needed because we are seeing that the schemachange workload is
deallocating huge amounts of automatically prepared statements. By
explicitly preparing statements again, they should no longer be
deallocated.

This also disables the automatic prepared statement cache in pgx entirely,
which was enabled in jackc/pgx@0c3e59b

Release justification: test-only change
Release note: None

@rafiss rafiss requested review from jordanlewis, ajwerner and a team August 24, 2021 17:35
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This reverts commit f446498,
and makes the 'prepare' setting work more closely to how it used to.

This is needed because we are seeing that the schemachange workload is
deallocating huge amounts of automatically prepared statements. By
explicitly preparing statements again, they should no longer be
deallocated.

Release justification: test-only change
Release note: None
@rafiss
Copy link
Collaborator Author

rafiss commented Aug 24, 2021

I ran schemachange/index/tpcc/w=1000 with this change and still see hundreds/thousands of DEALLOCATE statements.

This makes it so we only prepare the statements we want to, which we
control when we use the `prepare` mode.

Release justification: test only change
Release note: None
@rafiss
Copy link
Collaborator Author

rafiss commented Aug 25, 2021

Ah I see, jackc/pgx@0c3e59b is new in pgx4 and enables this automatic prepared statement caching. I made a 2nd commit in this PR which turns that off and it has gotten rid of all the DEALLOCATE statements.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm: though I wonder how important the first commit is. What would happen if we only merged the second commit?

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

@rafiss
Copy link
Collaborator Author

rafiss commented Aug 25, 2021

I agree, the 2nd commit is what prevents the thousands of DEALLOCATE statements.

I think the 1st commit is still good since we shouldn't change the client behavior unintentionally. If we didn't merge the first commit, what would happen is that every query would be run using Parse/Bind/Execute every time, using an anonymous prepared statement. I feel that if we want that behavior it's better to opt-in explicitly by using the noprepare workload setting.

@rafiss
Copy link
Collaborator Author

rafiss commented Aug 25, 2021

bors r=ajwerner

@rafiss rafiss linked an issue Aug 25, 2021 that may be closed by this pull request
@craig
Copy link
Contributor

craig bot commented Aug 25, 2021

Build succeeded:

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.

workload: large number of DEALLOCATE statements after pgx upgrade
3 participants