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

Implement new protocol testing framework; start fixing flush #1406

Merged
merged 2 commits into from May 25, 2020
Merged

Conversation

1st1
Copy link
Member

@1st1 1st1 commented May 25, 2020

Please review the commits individually.

@tailhook The second commit that fixes Flush is the one relevant for the rust protocol implementation. Note that it only addresses #1107 partially.

@1st1 1st1 requested review from tailhook and elprans May 25, 2020 05:44
@tailhook
Copy link
Contributor

@tailhook The second commit that fixes Flush is the one relevant for the rust protocol implementation. Note that it only addresses #1107 partially.

I'm still not sure if this is enough to use flush flawlessly.

@1st1
Copy link
Member Author

1st1 commented May 25, 2020

I'm still not sure if this is enough to use flush flawlessly.

Paul, can you elaborate please? It would also help if you could remind me why we want to send Flush to Postgres -- I can't think of a scenario where this is actually useful.

1st1 added 2 commits May 25, 2020 09:57
* Define protocol messages programmatically.

* Use messages definition to automatically generate C-struct
  layouts for docs.

* Implement a testing framework to test the protocol at
  the messages level: send individual messages to the server,
  receive replies, match replies.
@1st1 1st1 merged commit a3ccdaa into master May 25, 2020
@1st1 1st1 deleted the prototest branch May 25, 2020 18:05
@tailhook
Copy link
Contributor

I'm still not sure if this is enough to use flush flawlessly.

Paul, can you elaborate please?

Still don't recall and had no time to experiment with it. The concern is probably cherry-picking Sync messages here and there (and if we check for sync, why we don't check for flush in the same places?), but no known real use-case yet.

It would also help if you could remind me why we want to send Flush to Postgres -- I can't think of a scenario where this is actually useful.

I didn't propose and didn't support this change in any way. You have mentioned this as you expected it be like that all the time. If we can send a request to postgres and receive data without sync, we don't need to forward flush to postgres (I don't thing I'll do that in repl, but the documentation looks like it should work).

@1st1
Copy link
Member Author

1st1 commented May 26, 2020

The concern is probably cherry-picking Sync messages here and there (and if we check for sync, why we don't check for flush in the same places?)

Well, the implementation should work along these lines:

  • A user types SELECT 1; [ENTER]
  • REPL sends {PREPARE} SELECT 1; {FLUSH}
  • If an error occurs we display it to the user and REPL sends {SYNC}
  • no error - REPL sends {EXECUTE}; {SYNC}

I didn't propose and didn't support this change in any way. ... If we can send a request to postgres and receive data without sync, we don't need to forward flush to postgres

Fair enough. We tightly control all messaging with Postgres and issue {FLUSH} ourselves when necessary. So I think we're good.

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.

None yet

3 participants