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

[+] allow to specify multi-host connection string for configDB with target_session_attrs #647

Merged
merged 5 commits into from Jun 13, 2023

Conversation

krisavi
Copy link
Contributor

@krisavi krisavi commented May 30, 2023

With that Config DB can be PG cluster. Should be combined with #646 to avoid either setting password in service file or having pg_hba rule with "trust"

@pashagolub
Copy link
Collaborator

Would you please provide the situation when target_session_attrs are useful?

@krisavi
Copy link
Contributor Author

krisavi commented May 31, 2023

For example when Config DB is PG cluster and you want to set it to read-write node. Host field accepts currently already multiple hosts if you supply them and separate by comma. Connection String will be built out of the parameters supplied. However target_session_attrs is not supplied and its default value is 'any' if not supplied.
https://www.postgresql.org/docs/current/libpq-connect.html

target_session_attrs
This option determines whether the session must have certain properties to be acceptable. It's typically used in combination with multiple host names to select the first acceptable alternative among several hosts. There are six modes:

any (default)
any successful connection is acceptable

In our environment I have set up 2 pgwatch machines, one per site, they share config DB and it is clustered. Web is accessible on both nodes. PgWatch2 service itself I unfortunately have to stop and start depending on which site is mainly active. It is more in case something would happen to one site, then I can easily start monitoring service on machine located in 2nd site.

After that change I can connect to Config DB RW node when provide those options in pgwatch2-webui.service file.

[Service]
Environment="PW2_PGHOST=pgwatch11.krisavi.intra,pgwatch21.krisavi.intra"
Environment="PW2_PGDATABASE=pgwatch2_config"
Environment="PW2_PGUSER=pgwatch2"
Environment="PW2_TARGET_SESSION_ATTRS=read-write"
...

Maybe the Environment variable should be named to PW2_PGTARGET_SESSION_ATTRS just to align with other of it's PG related variables.

@kmoppel-cognite
Copy link
Collaborator

I would suggest not to have the new PW2_TARGET_SESSION_ATTRS variable at all as this is a pretty rare case and we already have too many :D Just add target_session_attrs='read-write' to the connect string if psycopg2.__libpq_version__ >= 100000, i.e. if session attributes are supported at all.

@kmoppel-cognite
Copy link
Collaborator

This approach would accepts a certain risk when all nodes are degraded to standby for a while...but still OK I think as the config DB / UI is not operationally critical compared to the metrics collector - which does actually configuration caching to handle short config file / DB downtimes.

Copy link
Collaborator

@kmoppel-cognite kmoppel-cognite left a comment

Choose a reason for hiding this comment

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

lgtm now!

@pashagolub pashagolub self-assigned this Jun 12, 2023
@pashagolub pashagolub changed the title Multi-host for configDB, adds target_session_attrs [+] allow to specify multi-host connection string for configDB with target_session_attrs Jun 12, 2023
@pashagolub
Copy link
Collaborator

Would you please resolve conflicts? It's probably introduced by #646

@krisavi
Copy link
Contributor Author

krisavi commented Jun 13, 2023

OK, will resolve the conflict that other PR caused.

I added row to documentation as well about read-write being default.

@pashagolub pashagolub merged commit c51a4f1 into cybertec-postgresql:master Jun 13, 2023
2 checks passed
@pashagolub
Copy link
Collaborator

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants