Conversation
The connection service file was only consulted inside the DSN branch of _parse_connect_dsn_and_args, so connect(service=..., servicefile=...) without a dsn silently fell back to the default host, port, user and database. PGSERVICE was read only after the service file had already been processed, so it never had any effect. Resolve the service name (argument, DSN query, then PGSERVICE) and the service file after the DSN is parsed, whether or not a DSN was given, keeping the existing precedence: explicit arguments and DSN values first, then the service file, then the other environment variables.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
connect()documentsserviceandservicefileparameters, but they only work when a DSN is also passed. The service-file lookup in_parse_connect_dsn_and_argssits inside theif dsn:branch, so this:ignores the service entirely. It silently connects to the default Unix sockets /
localhost:5432as the OS user, the same as a bareconnect(). If something is listening there, you are connected to the wrong database.PGSERVICEhas no effect at all, with or without a DSN: it is read after the service file has already been processed, and nothing uses it afterwards.psqlwith the same service file andPGSERVICE=prodconnects as expected.The fix moves the service name and service file resolution out of the DSN branch, so it runs after the DSN (if any) is parsed. The service name comes from the argument, then the DSN
servicequery parameter, thenPGSERVICE. Precedence is unchanged: explicit arguments and DSN values first, then the service file, then the otherPG*environment variables (the existing "envvars are overridden by service file" test still passes). Most of the diff is re-indentation of the existing block;git diff -wshows the real change.Tests: new cases in
test_connect_connection_service_filecoverservice+servicefilewithout a DSN,service+PGSERVICEFILE,PGSERVICEalone,PGSERVICEwith a DSN that has no service, and explicithost/userarguments overriding the service file. They fail on master (the result is the default socket list instead of('somehost', 5433)) and pass with the fix. End to end, checked against a local PostgreSQL 16 on a non-default port: on master,connect(service=..., servicefile=...)andPGSERVICEboth fail with "Connect call failed ('127.0.0.1', 5432)"; with the fix both connect to the right port.python -m unittest tests.test_connect tests.test_pool tests.test__sourcecodepasses (117 tests, 4 skipped, flake8 and mypy included).This bug was found and the fix prepared by an AI agent (breken-ai); the reproduction and all tests above were run before opening the PR.