8000 Target session attr (2) by JesseDeLoore · Pull Request #987 · MagicStack/asyncpg · GitHub
[go: up one dir, main page]
Skip to content

Target session attr (2) #987

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.

Sign up for GitHub

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

Merged
merged 26 commits into from
May 8, 2023
Merged

Target session attr (2) #987

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
bf6033c
Add target session attribute connection param
Aug 27, 2022
bee17cb
Fixed tests so they pass on windows.
Dec 7, 2022
1c67511
push for workflows
Dec 8, 2022
fae9d9c
push for workflows
Dec 8, 2022
7a847ce
no image for python 3.6
Dec 8, 2022
a3d7342
merge ci changes from master
Dec 8, 2022
7d9234f
merge setup.py changes from master
Dec 11, 2022
a43b064
Add logging to server selection procedure
Dec 12, 2022
24ea4e5
formatting
Dec 12, 2022
3df816d
undo mistake
Dec 12, 2022
e24a091
fix test?
Dec 12, 2022
38984a6
disable tests for pg11 to see if all the rest of the test cases pass
Dec 12, 2022
86423c3
disable tests for pg11 to see if all the rest of the test cases pass
Dec 15, 2022
277ed96
add some more fixes that were already implemented
Dec 16, 2022
5737f76
Add support for read-write and read-only target_session_attribute opt…
Dec 16, 2022
c3133c0
fix little logical error
Dec 16, 2022
ae325ba
linter
Dec 16, 2022
980004c
Merge branch 'master' into target_session_attr
JesseDeLoore Dec 17, 2022
3bc8322
fix logic issue
Dec 18, 2022
4e97acc
Merge branch 'target_session_attr' of github.com:JesseDeLoore/asyncpg…
Dec 18, 2022
d824333
Update based on review.
Jan 29, 2023
08ad1f8
fix tests
Jan 29, 2023
7cdd2ba
See the results on pg11
Jan 29, 2023
19b7a17
...
Jan 30, 2023
ea002ed
Apply fix
May 4, 2023
9f1d836
Merge branch 'master' into target_session_attr
JesseDeLoore May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
disable tests for pg11 to see if all the rest of the test cases pass
  • Loading branch information
    • 8000
Jesse De Loore committed Dec 15, 2022
commit 86423c31e6118e68daf7965811cf44b3c64cf047
8 changes: 5 additions & 3 deletions tests/test_connect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1757,7 +1757,7 @@ async def _run_connection_test(
await conn.close()

async def test_target_server_attribute_port(self):
if self.cluster.get_pg_version()[0] == 11:
if self.master_cluster.get_pg_version()[0] == 11:
self.skipTest("PostgreSQL 11 seems to have issues with this test")
Copy link
Member

Choose a reason for hiding this comment

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

Please elaborate. If tests on 11 are failing then we either need a version-specific workaround or disable the feature on those servers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a failing test that only happens on version 11:
https://github.com/JesseDeLoore/asyncpg/actions/runs/4043448766/jobs/6952375723#step:7:342

Not sure what to make of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To elaborate: It looks like "SELECT pg_catalog.pg_is_in_recovery()" returns False for the hot standby server in PG11.
Pretty sure this is not intended. Do you prefer disabling the feature for this version or trying to find a workaround (although I wouldn't know where to look for that).

Copy link
Member

Choose a reason for hiding this comment

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

I'll see if I can repro locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Were you able to reproduce it locally ? Anything else I can do to help move this along ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. Gentle reminder. If there is anything I can do, do let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, are there any updates? Is there anything I can do ?

Copy link
Member

Choose a reason for hiding this comment

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

This diff fixes the test on PG 11:

diff --git a/asyncpg/cluster.py b/asyncpg/cluster.py
index 0999e41..4467cc2 100644
--- a/asyncpg/cluster.py
+++ b/asyncpg/cluster.py
@@ -626,7 +626,7 @@ class HotStandbyCluster(TempCluster):
                 'pg_basebackup init exited with status {:d}:\n{}'.format(
                     process.returncode, output.decode()))

-        if self._pg_version <= (11, 0):
+        if self._pg_version < (12, 0):
             with open(os.path.join(self._data_dir, 'recovery.conf'), 'w') as f:
                 f.write(textwrap.dedent("""\
                     standby_mode = 'on'

Please apply and rebase, and I think this is good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied and merged master. Is that ok for you, or do you require an actual rebase?

master_port = self.master_cluster.get_connection_spec()['port']
standby_port = self.standby_cluster.get_connection_spec()['port']
Expand All @@ -1772,7 +1772,7 @@ async def test_target_server_attribute_port(self):
)

async def test_target_attribute_not_matched(self):
if self.cluster.get_pg_version()[0] == 11:
if self.master_cluster.get_pg_version()[0] == 11:
self.skipTest("PostgreSQL 11 seems to have issues with this test")
tests = [
(self.connect_standby, 'primary'),
Expand All @@ -1784,7 +1784,7 @@ async def test_target_attribute_not_matched(self):
await connect(target_session_attribute=target_attr)

async def test_prefer_standby_when_standby_is_up(self):
if self.cluster.get_pg_version()[0] == 11:
if self.master_cluster.get_pg_version()[0] == 11:
self.skipTest("PostgreSQL 11 seems to have issues with this test")
con = await self.connect(target_session_attribute='prefer-standby')
standby_port = self.standby_cluster.get_connection_spec()['port']
Expand All @@ -1793,6 +1793,8 @@ async def test_prefer_standby_when_standby_is_up(self):
await con.close()

async def test_prefer_standby_picks_master_when_standby_is_down(self):
if self.master_cluster.get_pg_version()[0] == 11:
self.skipTest("PostgreSQL 11 seems to have issues with this test")
primary_spec = self.get_cluster_connection_spec(self.master_cluster)
connection_spec = {
'host': [
Expand Down
0