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

fix(postgres): fix pg_isready issue with empty listen_addresses #165

Merged
merged 2 commits into from
Apr 14, 2024

Conversation

johnhampton
Copy link
Contributor

This PR resolves the issue where pg_isready would fail when socketDir is set and listen_addresses is empty. pg_isready is now modified to use socketDir when it is available.

This commit resolves the issue where `pg_isready` would fail when
`socketDir` is set and `listen_addresses` is empty. `pg_isready` is now
modified to leverage `socketDir` when it is available.
Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Shoudn't this be done at a level close to the option def? Maybe:

default = {
listen_addresses = config.listen_addresses;
port = config.port;
unix_socket_directories = config.socketDir;
hba_file = "${config.hbaConfFile}";
};

(I haven't looked the whole code, so I might be missing something)

@shivaraj-bh
Copy link
Member

Shoudn't this be done at a level close to the option def?

I doubt that is possible here, because then we would expect pg_isready to automatically detect, which amongst unix_socket_directories or listen_addresses to choose from based on postgresql.conf but don’t think that is supported: https://www.postgresql.org/docs/current/app-pg-isready.html

@shivaraj-bh
Copy link
Member

@johnhampton could you also add a basic test services.postgres.”pg3”, that would test for empty listen_addresses and non-empty socketDir?

You can add the test here: https://github.com/juspay/services-flake/blob/main/nix/postgres/postgres_test.nix

Test a postgres service with socketDir set and empty listen_addresses.
@johnhampton
Copy link
Contributor Author

@johnhampton could you also add a basic test services.postgres.”pg3”, that would test for empty listen_addresses and non-empty socketDir?

You can add the test here: https://github.com/juspay/services-flake/blob/main/nix/postgres/postgres_test.nix

@shivaraj-bh done.

@johnhampton
Copy link
Contributor Author

Shoudn't this be done at a level close to the option def? Maybe:

@srid I'm not sure, I followed the code just above my change.

${ if config.socketDir != "" then ''
PGSOCKETDIR=$(readlink -f "${config.socketDir}")
postgres -k "$PGSOCKETDIR"
'' else ''
postgres
''}

@shivaraj-bh
Copy link
Member

I doubt that is possible here, because then we would expect pg_isready to automatically detect, which amongst unix_socket_directories or listen_addresses to choose from based on postgresql.conf but don’t think that is supported: https://www.postgresql.org/docs/current/app-pg-isready.html

And primarily that is because the settings above (postgresql.conf) only applies to the server and not the client CLI utils.

I couldn’t find any documentation on the clients picking up configs from postgresql.conf, but did for the server: https://www.postgresql.org/docs/current/config-setting.html

Copy link
Member

@shivaraj-bh shivaraj-bh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@shivaraj-bh shivaraj-bh merged commit 4d363b6 into juspay:main Apr 14, 2024
4 checks passed
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.

3 participants