Hi Jelte,
Thanks for working on this. I’ve done an initial review of patch 4 and
here are some comments below.
1) Test infra: tmp_check() fixture looks wrong / unused variable
def tmp_check(tmp_path_factory):
d = os.getenv("TESTDATADIR")
if d:
d = pathlib.Path(d)
else:
d = tmp_path_factory.mktemp("tmp_check")
return tmp_path_factory.mktemp("check_tmp")
You compute d then ignore it and always return a new temp dir
"check_tmp". It should probably return d (or d / "check_tmp").
2) PQexec NULL handling is missing
+ def exec(self, query: str):
+ ...
+ res = self._lib.PQexec(self._handle, query.encode())
+ return self._stack.enter_context(PGresult(self._lib, res))
No NULL check. If PQexec returns NULL (OOM, connection lost), the
PGresult wrapper will pass it to PQresultStatus etc., causing
undefined behavior or crash.
3) array parsing may be too simple
_parse_array() does inner.split(","). That will break for valid
Postgres arrays containing:
quoted elements with commas: {"a,b","c"}
escapes / quotes: {"a\"b"} or {"a\\b"}
nested arrays: {{1,2},{3,4}}
Should we document the constraint here or implement a more complete
state-machine?
4) Type conversion: timestamp/timestamptz conversion could be wrong
datetime.datetime.fromisoformat for both timestamp and timestamptz.
- timestamptz output formatting from Postgres is not always
fromisoformat() friendly (can include timezone formats that differ).
- fromisoformat() yields timezone-aware datetimes only if the string
has an offset; but Postgres output depends on DateStyle and timezone
settings.
This could make tests brittle across environments. Should we use
dateutil.parser (if allowed) or document that the server uses ISO
settings for pytest.
5) Server init: pg_ctl init vs initdb
In non-Windows branch:
run(pg_ctl, "-D", datadir, "init")
initdb seems to be a more common way here.
6) Logging config: log_connections = all seems wrong
print("log_connections = all", file=f)
I don't see an option "all" for this parameter
https://postgresqlco.nf/doc/en/param/log_connections/
7) UX: error message handling and query attachment
raise_error() builds message with primary + optional Query: ....
Should we include SQLSTATE and severity in the message string by
default, because it helps when reading CI logs.
--
Best,
Xuneng
