Target session attr (2) #987
Conversation
Are the hosts unique for the replica clusters ? I find it hard to tell, but at least on Windows all clusters are on localhost, so the test was not actually verifying the code.
… into target_session_attr
|
@elprans I hate to push, but could have a look at this PR ? If there is anything I can do to improve it, please let me know. |
|
+1 |
|
@elprans polite nudge |
asyncpg/connection.py
Outdated
| @@ -2003,6 +2005,16 @@ async def connect(dsn=None, *, | |||
| this connection object. Must be a subclass of | |||
| :class:`~asyncpg.Record`. | |||
|
|
|||
| :param SessionAttribute target_session_attribute: | |||
There was a problem hiding this comment.
libpq uses target_session_attrs as the spelling, so let's follow that.
| :param SessionAttribute target_session_attribute: | ||
| If specified, check that the host has the correct attribute. | ||
| Can be one of: | ||
| "any": the first successfully connected host |
asyncpg/connection.py
Outdated
| except ValueError as exc: | ||
| raise exceptions.InterfaceError( | ||
| "target_session_attribute is expected to be one of " | ||
| "'any', 'primary', 'standby' or 'prefer-standby'" |
There was a problem hiding this comment.
I suggest joining __members__._value_ to avoid repeating yourself and future-proof the message.
tests/test_connect.py
Outdated
|
|
||
| async def test_target_server_attribute_port(self): | ||
| if self.master_cluster.get_pg_version()[0] == 11: | ||
| self.skipTest("PostgreSQL 11 seems to have issues with this test") |
There was a problem hiding this comment.
Please elaborate. If tests on 11 are failing then we either need a version-specific workaround or disable the feature on those servers.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I'll see if I can repro locally.
There was a problem hiding this comment.
Were you able to reproduce it locally ? Anything else I can do to help move this along ?
There was a problem hiding this comment.
Hi. Gentle reminder. If there is anything I can do, do let me know.
There was a problem hiding this comment.
Hi, are there any updates? Is there anything I can do ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I applied and merged master. Is that ok for you, or do you require an actual rebase?
asyncpg/connection.py
Outdated
| @@ -1792,7 +1793,8 @@ async def connect(dsn=None, *, | |||
| direct_tls=False, | |||
| connection_class=Connection, | |||
| record_class=protocol.Record, | |||
| server_settings=None): | |||
| server_settings=None, | |||
| target_session_attribute=SessionAttribute.any): | |||
There was a problem hiding this comment.
For improved libpq compatibility let's add support for the PGTARGETSESSIONATTRS environment variable also. Hence, the default here should be None (unspecified).
asyncpg/connection.py
Outdated
| @@ -2087,6 +2099,15 @@ async def connect(dsn=None, *, | |||
| if record_class is not protocol.Record: | |||
| _check_record_class(record_class) | |||
|
|
|||
| try: | |||
| target_session_attribute = SessionAttribute(target_session_attribute) | |||
There was a problem hiding this comment.
Please move this check to _parse_connect_dsn_and_args. That is also the place where the PGTARGETSESSIONATTRS can be handled in case an explicit argument wasn't passed to connect().
Minor fixes and improvements. Changes ======= * Do not try to cleanup statements (#981) (by @fvannee in d2e710f for #981) * Add Pool.is_closing() method (#973) (by @singingwolfboy in 9cb2c1c for #973) * Fix test_tls_version for LibreSSL (#974) (by @CyberTailor in 7df9812 for #974) * Handle environments without home dir (#1011) (by @LeonardBesson in 172b8f6 for #1011) * fix: salt and iterations parsing for scram (#1026) (by @trigonometr in 7443a9e for #1026) * Add support for target_session_attrs (#987) (by @JesseDeLoore in bf74e88 for #987) * Add support for READ UNCOMMITTED (#1039) (by @benwah in 2f20bae for #1039) * Update benchmarks, add psycopg3 (#1042) (by @elprans in 7d4fcf0 for #1042)
Minor fixes and improvements. Changes ======= * Do not try to cleanup statements (#981) (by @fvannee in d2e710f for #981) * Add Pool.is_closing() method (#973) (by @singingwolfboy in 9cb2c1c for #973) * Fix test_tls_version for LibreSSL (#974) (by @CyberTailor in 7df9812 for #974) * Handle environments without home dir (#1011) (by @LeonardBesson in 172b8f6 for #1011) * fix: salt and iterations parsing for scram (#1026) (by @trigonometr in 7443a9e for #1026) * Add support for target_session_attrs (#987) (by @JesseDeLoore in bf74e88 for #987) * Add support for READ UNCOMMITTED (#1039) (by @benwah in 2f20bae for #1039) * Update benchmarks, add psycopg3 (#1042) (by @elprans in 7d4fcf0 for #1042)
Recreating this PR because it seems to have gone silent.
The target is to implement the
target_session_attributekeyword when connecting to a database cluster.Supported options are