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

try to read any notifies or errors that come in asynchronously #2143

Merged
merged 14 commits into from May 20, 2021

Conversation

davecramer
Copy link
Member

Lets see what this breaks

@davecramer
Copy link
Member Author

interestingly this test does not fail locally.

@vonzshik
Copy link

vonzshik commented May 5, 2021

interestingly this test does not fail locally.

That's because kill completes when a signal is send, so there is a race condition between a signal being received/processed and another query.

@davecramer
Copy link
Member Author

You do realize the kill is being sent on a different connection, right ?

@vonzshik
Copy link

vonzshik commented May 5, 2021

Right now the test works like this:

  1. Execute a query on the first connection
  2. The second connection executes a query which kills the second connection
  3. The result set from the first connection is read
  4. Another query is executed on the first connection

AFAIK PG doesn't notify other connections if one of them is dead, so I assume pgjdbc just closes other open connections if there is a problem with a host. Which leads me to believe that although there is no race condition between killing the backend and sending another query, there is a race condition between killing the backend and reading a response from the second step. Essentially, the second step might not throw an exception.

Correct me if I've missed something.

@davecramer
Copy link
Member Author

Yes, PG does notifiy other connections if the connection is killed in such a way that causes a restart of the backend. Before it restarts it will send out the warning to all active connections and then restart. That is the whole point of this PR and the test. As I said this works fine locally.

@vonzshik
Copy link

vonzshik commented May 5, 2021

Yes, PG does notifiy other connections if the connection is killed in such a way that causes a restart of the backend. Before it restarts it will send out the warning to all active connections and then restart.

I actually didn't know that, thank you! Was able to reproduce this behavior on linux and partially on windows (I was unable to read a notice, but it's most likely due to windows socket implementation).

Still, just to test the theory of a kill race condition you can add a delay just before a second query on a first connection is send.

@davecramer
Copy link
Member Author

This test seems to be a little too destructive. On travis the db does not recover very quickly

kneeraj added a commit to yugabyte/pgjdbc that referenced this pull request Dec 24, 2021
Merge done to get the perf issue addressed in upstream which was introduced in the upstream earlier.
The upstream pgjdbc ( on 42.3.0 ) had introduced this performance issue in rb3d5fba91a9ddbd6c7e6bfce774f0c0cc94593b9
This issue was reverted later on Oct 7th this year in commit: c4c35d1

Commit comment:

Revert "try to read any notifies or errors that come in asynchronously" (pgjdbc#2276)

Revert "fix: try to read any notifies or errors that come in asynchronously (try to read any notifies or errors that come in asynchronously pgjdbc#2143)"
This reverts commit b3d5fba.

fix spacing on GSS_ENC_MODE (fix spacing on GSS_ENC_MODE [SKIP-CI] pgjdbc#2280)
kneeraj added a commit to yugabyte/yugabyte-db that referenced this pull request Feb 9, 2022
…-yugabytedb driver

Summary:
The degradation was because the upstream pgjdbc ( on 42.3.0 ) had introduced this performance issue in rb3d5fba91a9ddbd6c7e6bfce774f0c0cc94593b9 … upstream repo: git@github.com:pgjdbc/pgjdbc.git

This issue was reverted later on Oct 7th this year in commit: c4c35d1

```
Commit comment:

Revert "try to read any notifies or errors that come in asynchronously" (pgjdbc#2276)

Revert "fix: try to read any notifies or errors that come in asynchronously (try to read any notifies or errors that come in asynchronously pgjdbc/pgjdbc#2143)"
This reverts commit b3d5fba.

fix spacing on GSS_ENC_MODE (fix spacing on GSS_ENC_MODE [SKIP-CI] pgjdbc/pgjdbc#2280)
```

But by that time we had already forked our repo. With this change there was some unnecessary read of the sockets happening and in the process some buffer allocation happening which was the cause of the slowness.

I have merged the latest version i.e. 42.3.2 in our fork.
The smart driver jar referenced in the yb-pgsql pom.xml is the latest fixed version now.

Test Plan:
yb-sample-apps
yb-pgsql unit tests
smart driver demo apps in: https://github.com/yugabyte/pgjdbc/tree/master/examples

Reviewers: nikhil, ashetkar, mihnea, ssong

Reviewed By: ssong

Subscribers: ssong, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D14504
jayant07-yb pushed a commit to jayant07-yb/yugabyte-db that referenced this pull request Mar 8, 2022
…est jdbc-yugabytedb driver

Summary:
The degradation was because the upstream pgjdbc ( on 42.3.0 ) had introduced this performance issue in rb3d5fba91a9ddbd6c7e6bfce774f0c0cc94593b9 … upstream repo: git@github.com:pgjdbc/pgjdbc.git

This issue was reverted later on Oct 7th this year in commit: c4c35d1

```
Commit comment:

Revert "try to read any notifies or errors that come in asynchronously" (pgjdbc#2276)

Revert "fix: try to read any notifies or errors that come in asynchronously (try to read any notifies or errors that come in asynchronously pgjdbc/pgjdbc#2143)"
This reverts commit b3d5fba.

fix spacing on GSS_ENC_MODE (fix spacing on GSS_ENC_MODE [SKIP-CI] pgjdbc/pgjdbc#2280)
```

But by that time we had already forked our repo. With this change there was some unnecessary read of the sockets happening and in the process some buffer allocation happening which was the cause of the slowness.

I have merged the latest version i.e. 42.3.2 in our fork.
The smart driver jar referenced in the yb-pgsql pom.xml is the latest fixed version now.

Test Plan:
yb-sample-apps
yb-pgsql unit tests
smart driver demo apps in: https://github.com/yugabyte/pgjdbc/tree/master/examples

Reviewers: nikhil, ashetkar, mihnea, ssong

Reviewed By: ssong

Subscribers: ssong, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D14504
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

2 participants