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

Remove PrepareStatementStateMachine #391

Merged

Conversation

fabianfett
Copy link
Collaborator

Preparing a statement is a substep of running an extended query. For this reason we should reuse the ExtendedQueryStateMachine as much as we can. This patch removes the PrepareStatementStateMachine and uses the ExtendedQueryStateMachine. As a result of this we can simplify our code in lots of other places.

Preparing a statement is a substep of running an extended query. For this reason we should reuse the ExtendedQueryStateMachine as much as we can. This patch removes the `PrepareStatementStateMachine` and uses the `ExtendedQueryStateMachine`. As a result of this we can simplify our code in lots of other places.
@fabianfett fabianfett force-pushed the ff-remove-PrepareStatementStateMachine branch from 7e4baca to 5ae831d Compare August 9, 2023 12:20
@fabianfett fabianfett requested a review from gwynne August 9, 2023 12:20
@fabianfett fabianfett added the semver-patch No public API change. label Aug 9, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #391 (7e4baca) into main (220eb50) will decrease coverage by 1.55%.
Report is 1 commits behind head on main.
The diff coverage is 72.18%.

❗ Current head 7e4baca differs from pull request most recent head 85fd93a. Consider uploading reports for the commit 85fd93a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #391      +/-   ##
==========================================
- Coverage   46.02%   44.47%   -1.55%     
==========================================
  Files         110      109       -1     
  Lines        8833     8724     -109     
==========================================
- Hits         4065     3880     -185     
- Misses       4768     4844      +76     
Files Changed Coverage Δ
...es/PostgresNIO/Connection/PostgresConnection.swift 35.45% <0.00%> (-0.10%) ⬇️
Sources/PostgresNIO/New/PSQLTask.swift 19.35% <28.57%> (-13.98%) ⬇️
...nection State Machine/ConnectionStateMachine.swift 55.68% <40.00%> (-0.53%) ⬇️
...urces/PostgresNIO/New/PostgresChannelHandler.swift 72.27% <65.38%> (+0.84%) ⬆️
...tion State Machine/ExtendedQueryStateMachine.swift 76.27% <90.32%> (+2.08%) ⬆️
Sources/PostgresNIO/New/PSQLRowStream.swift 54.48% <100.00%> (-30.52%) ⬇️

... and 2 files with indirect coverage changes

@@ -4,7 +4,7 @@ struct ExtendedQueryStateMachine {

private enum State {
case initialized(ExtendedQueryContext)
case parseDescribeBindExecuteSyncSent(ExtendedQueryContext)
case messagesSent(ExtendedQueryContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the name messageSent intentionally generic? When I read it my first reaction was "What message? Do we have different kind of messages flying around?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. it depends on what the internal QueryContext says.

If we run an unnamed query, we sent parse, describe, bind, execute & sync.
If we prepare a statement, we sent parse, describe & sync.
If we execute a statement, we sent bind, execute & sync.

enum RowSource {
case stream(PSQLRowsDataSource)
enum Source {
case stream([RowDescription.Column], PSQLRowsDataSource)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between RowDescription and [RowDescription.Column]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RowDescription is a wrapper around [RowDescription.Column]. A bit stupid. This makes sense if you look at how the PostgresBackendMessages are structured.

bufferState = .streaming(buffer: .init(), dataSource: dataSource)
case .noRows(.success(let commandTag)):
self.rowDescription = []
Copy link
Contributor

Choose a reason for hiding this comment

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

In other parts of the codebase (e.g. in PrepareStatementStateMachine) we allowed row description to be nil. Is there any semantic difference between a RowDescription? with a nil value and an empty [RowDescription.Column]?

@fabianfett fabianfett merged commit b6597f7 into vapor:main Aug 9, 2023
13 checks passed
@fabianfett fabianfett deleted the ff-remove-PrepareStatementStateMachine branch August 9, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants