Repository navigation
Conversation
el-marvin
left a comment
There was a problem hiding this comment.
Changes requested.
Reviewed commit 22819e3 (one commit on the merge base c2556ed; first round). I built the head and the merge base the same way (Release, Python 3.13), made a private copy of the released duckdb 2.0.0, and ran every probe offline (a fresh network namespace with no network, and an empty HOME). The premise holds. On 2.0.0 and on the merge base, extension_directories is empty and a binary under site-packages/.duckdb_extensions is neither listed nor used, so an extension that a duckdb-core-ext-<name> wheel installed is downloaded again online and fails to autoload offline. On the head, the reproducer in the description prints what it says, and an offline autoload of tpch with a stand-in binary in that directory goes straight to loading the stand-in instead of trying to install. The Python package is the right layer for adding the directory, since only Python knows where site-packages is. The order rule is a containment of the engine's install check, which looks only in the first directory; the description says so and names the engine change that would make the order irrelevant (duckdb/duckdb#28480, still open). The rule is the duckdb-cli wrapper's, which also puts the directory first when writable and last otherwise, with the same default; this change adds the deduplication the wrapper lacks. The tests are strong: I reverted the parts of the change one at a time, 18 weakenings of the Python module and 3 of the C++ side, and every one turns at least one test red. What keeps this from landing: the directory is found only when the extension wheel went into the same site directory as duckdb itself, which misses pip's own fallback for a read-only site-packages; the behaviour the change exists for (autoload without a download) has no test; and the release note leaves out one consequence of the order rule. The order rule and its consequences are already with Evert as an open question; I list them below as behaviour changes for him to decide, not as defects.
Findings
1. The pip directory is found only beside duckdb's own dist-info, so an extension wheel installed into another site directory of the same interpreter is never searched (duckdb/_extension_directories.py:58).
Severity: should fix. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: look for .duckdb_extensions in every site directory the interpreter uses, in sys.path order, with the same rule for each (writable ones first, read-only ones last); or, if the narrower scope is the decision, say in the description and the release note that an extension wheel must be installed into the same site directory as duckdb.
The evidence is in the inline comment on duckdb/_extension_directories.py:58.
2. No test pins the behaviour the change exists for: an autoload that finds a pip-installed binary without an install attempt (tests/fast/api/test_pip_extension_directory.py:413).
Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: add an offline autoload test with a stand-in binary of an extension that is not built in, with autoinstall_extension_repository pointed at an empty local directory, for the writable and the read-only position.
The evidence is in the inline comment on tests/fast/api/test_pip_extension_directory.py:413.
3. The release note leaves out that, once a writable .duckdb_extensions comes first, UPDATE EXTENSIONS no longer sees extensions installed in ~/.duckdb/extensions.
Severity: should fix. Category: docs. Evidence: ran. Fix: containment. Origin: engine. Base: widened.
Ask: add this consequence to the release note in the description and to the open question about the order rule.
ExtensionHelper::UpdateExtensions lists only the first directory (external/duckdb/src/main/extension/extension_helper.cpp:341, auto ext_directory = ExtensionHelper::ExtensionDirectory(db, fs);). The description says UPDATE EXTENSIONS "then write[s] into site-packages/.duckdb_extensions", which is true for what is there, but the user-visible change is that everything installed earlier stops being updated, without an error. Probe (round-1/probes/p03_update_extensions.py, a stand-in tpch in ~/.duckdb/extensions, an empty pip directory):
merge base: update: [{'extension_name': 'tpch', ..., 'update_result': 'MISSING_INSTALL_INFO', ...}]
head: update: []
4. With the two-entry default list, duckdb_extensions() reports the later directory's copy of an extension present in both, while LOAD opens the first.
Severity: minor. Category: correctness. Evidence: ran. Fix: containment. Origin: engine. Base: widened.
Ask: a sentence in the release note; the engine fix is separate.
The listing overwrites an earlier directory's entry with a later one (external/duckdb/src/function/table/system/duckdb_extensions.cpp:145, entry->second.file_path = info.file_path;), while the loader stops at the first directory that holds the file (external/duckdb/src/main/extension/extension_load.cpp:572 to 575). Before this change the default search had one directory, so the two could not disagree unless the user configured a list. Now a user who pip-installs an extension that is also in ~/.duckdb/extensions sees the ~/.duckdb path in duckdb_extensions() while LOAD opens the pip copy (round-1/probes/p05_listing_with_two_copies.py: listed: <home>/.duckdb/extensions/29b26cb106/linux_arm64/tpch.duckdb_extension with the pip directory first; which copy LOAD opens is read from the loader's code, since a stand-in's load error does not name its file). The body of duckdb/duckdb#28480 names this as a gap tracked separately.
5. Path(directory).is_dir() raises when the directory cannot be looked up, and that error fails every connect (duckdb/_extension_directories.py:32).
Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: use os.path.isdir(directory), which reads an error as "not a directory".
The evidence is in the inline comment on duckdb/_extension_directories.py:32.
6. A database opened through the ADBC driver does not get the directory (adbc_driver_duckdb/__init__.py:32); this does not block the pull request.
Severity: minor. Category: design. Evidence: ran. Fix: containment. Origin: duckdb-python. Base: pre-existing.
Ask: none for this pull request, which states the limit; a later change can pass the list as a database option.
adbc_driver_duckdb.connect opens the database through the C API entry point duckdb_adbc_init in the same extension module, so Connect and its hook never run. Probe (round-1/probes/p10_adbc.py, pip directory present, memory_limit and threads passed as database options): adbc: ([], 8). The driver already forwards database options to the engine, and the list literal text form is accepted (the tests pin it), so connect could pass extension_directories from search_path the same way.
Behaviour changes for the maintainer to decide
These follow from the order rule, which is with Evert as an open question; they are not defects in the code.
- With a writable
.duckdb_extensionsfirst, extensions installed earlier into~/.duckdb/extensionsno longer autoload offline (probe: merge base loads the stand-in from~/.duckdb/extensions; head tries to downloadtpchand fails), and online they are downloaded again into site-packages. INSTALL,FORCE INSTALLand autoinstall write into site-packages, which pip neither tracks nor removes, andUPDATE EXTENSIONScovers only that directory (finding 3).- A list the user configured on purpose loses its install target: with a writable pip directory,
INSTALLwrites into site-packages instead of the first configured directory (probep02_autoload_order.pywith a configured one-entry list that holds a stand-intpch, autoinstall repository empty: the merge base loads the stand-in from the configured directory; the head puts the pip directory first and tries to install from the empty repository). Keeping a configured list's first entry first, and adding the pip directory after it, would leave that choice with the user at the price of no offline autoload from the pip directory for them; that is a decision, not a defect. - With a read-only
.duckdb_extensionslast, autoload of a pip-installed extension downloads online and fails offline; an explicitLOAD, or autoload withautoinstall_known_extensions = false, finds it (probe: head with the directory read only and autoinstall off loads the stand-in from it).
Smaller things
- The write probe creates and removes a file in site-packages at every connect, also for a second connect to an open file, whose result is thrown away. Whether an existing directory is writable does not change when a wheel is installed into it, so the answer could be kept per directory once the directory exists. The cost is not the issue (31 microseconds per connect for the hook, against a connect and close of 26 to 35 ms on the same build); touching site-packages at every connect is.
- Once this ships, the "What is not promised" section of
PROTOCOL.mdin duckdb/duckdb-python-extensions ("TheduckdbPython module does not readsite-packages/.duckdb_extensionsyet") needs updating. - The last sentence of the description (an earlier form of the tests failing 23 of 28 on the merge base) is about tests that are not in the pull request and cannot be checked. As the description says, the tests as submitted cannot be collected on the merge base (
ImportError: cannot import name '_extension_directories').
Outside findings not taken
- Codex: deduplication misses a
~or relative spelling of the pip directory in a configured list. Not taken: a user who lists the directory first in a spelling of their own gets the install target they configured, as on the base; the cost is one directory searched twice. - Codex: the fixed
~/.duckdb/extensionsdrops a build's own default directories. Not taken: the description states the limit; the engine keepsDefaultExtensionFoldersprivate (external/duckdb/src/include/duckdb/main/extension_helper.hpp:278to284), and conda-forge's python-duckdb recipe builds the sdist withoutEXTENSION_DIRECTORIES. - Codex: the tests should use a valid extension binary to prove LOAD and the install target. Taken in a narrower form as finding 2; no valid binary for a development build is available offline, and a stand-in tells the two outcomes apart.
- Codex: the duplicate listing in
duckdb_extensions()needs an engine fix before this ships. Taken as finding 4 at minor: it misreports a system table and changes no query result.
Prerequisites in the engine
- Install check of the first directory only:
ExtensionHelper::ExtensionDirectory(external/duckdb/src/main/extension/extension_install.cpp:155,extension_directories[0]) andInstallExternalExtension(extension_install_dynamic.cpp:454,fs.FileExists(local_extension_path) && !options.force_install). Upstream main is unchanged; duckdb/duckdb#28480 is open. UpdateExtensionslists only the first directory (extension_helper.cpp:341). Upstream main is unchanged; duckdb/duckdb#28480 leaves it open on purpose.duckdb_extensions()reports the last copy (duckdb_extensions.cpp:145). Upstream main is unchanged.
Recommendation: ship with the containment once the findings above are addressed, and relax the order rule when duckdb/duckdb#28480 lands; waiting for the engine buys nothing for the read-only case, which needs the engine change either way.
What I verified
- Head 22819e3, range c2556ed..22819e3, one commit, no merge commits. Engine submodule 29b26cb1060 at both ends.
- Builds: head (the coordinator's build) and merge base in
759/base, both with the repository's two stepuv syncinstall on Python 3.13, Release. A variant clone in759/variantfor the reverts. Released duckdb 2.0.0 copied into a private venv. Probes ran from non-editable copies of each build (package, extension module and dist-info copied into a fresh venv's site-packages), so the dist-info sits beside the package as in a wheel install. - Probes, all offline under
unshare -rn(orunshare -cnwhere a read-only directory must hold, since the mapped root of-rwrites anywhere), with HOME pointed at an empty folder: the description's reproducer on 2.0.0 ([],[]), merge base ([],[]) and head (pip directory then~/.duckdb/extensions; the file listed); autoload order with a stand-in binary in each directory, writable and read only, autoinstall on and off, default and empty local repository;UPDATE EXTENSIONS; two copies in the listing; user site;enable_external_access = false(the merge base and the head failduckdb_extensions()the same way);SETandRESETof the list; a database opened through the ADBC driver. Scripts and outputs are inround-1/probes/. - Tests:
pytest tests/fast/api/test_pip_extension_directory.py -n 8with a 600 second timeout, on the head, editable layout, non-root:40 passed, 1 skipped in 1.86s(the skip is the non-editable location test); as mapped root:39 passed, 2 skipped; on a non-editable copy of the head:41 passed in 1.18s. On the merge base the module cannot be collected, as the description says. - Full suite:
pytest tests/fast -n 8with a 600 second timeout, offline, non-root, on the head with a writable.duckdb_extensionspresent, so every connection of the suite got the new list:17641 passed, 114 skipped, 164 xfailed in 837.65s. Nothing was written into the directory. - Reverts, each against the same test file: 18 weakenings of
duckdb/_extension_directories.py(always first, always last, no directory check,existsforis_dir, deprecated setting ignored, no deduplication, deduplication without normalising, no default, no cache, no UTF-8 check, no backslash check, first dist-info always, noabsolute, unlink after close, no unlink,os.access, only open errors caught, every exception caught) and 3 ofsrc/pyconnection.cpp(no finalizing check, the result written into the user options, the hook called beforeSetOptionsByName). Each failed between 1 and 24 tests; none passed. - Cost: connect and close of
:memory:with one thread, 300 iterations, best of five, alternating builds: the merge base took 32.8, 34.7, 31.3 and 25.6 ms; the head with the directory absent 31.8, 27.7 and 27.5 ms (a fourth run is void, another probe created the directory during it); the head with it present 30.8, 33.0, 26.2 and 29.1 ms. The hook alone, called 20,000 times, best of five: 6.6 microseconds with the directory absent and 30 to 35 with it present, which matches the description's 7 and 32. The spread between runs of one build is about 300 times the hook's cost, so the connect path shows no measurable change. Scriptround-1/probes/b01_connect_cost.py, figures inround-1/bench-connect.txt. - Limited API:
src/pyconnection.cppandsrc/duckdb_python.cpp, compiled from the build's own command lines withPy_LIMITED_API=0x030C0000and syntax checking only, pass on the head and on the merge base. - Outside reviews: Codex (read only, reasoning effort xhigh), report in
round-1/codex-report.md; the code-review skill at effort xhigh, still running when this was written; its triage follows in an update. - CI at the time of review: 8 checks passed (code quality, submodule sanity, the sdist and five seed wheel builds with their tests) and 4 were still running (two sanity wheels on Linux, the Windows x86 seed, the full API build with assertions); none failed.
What held
- The order rule as stated: writable first, read-only last, a configured list kept after or before it, an explicit empty list given the default, the deprecated
extension_directorygiven no default, a configured entry naming the directory kept once with or without a trailing separator. - Nothing changes when the directory is absent, is a file, or no duckdb metadata is on
sys.path; a directory created after a connect is found by the next one. - A second connect to an open database file does not fail with "different configuration" although the probe answers differently; the result never enters the user options.
- No Python runs while the interpreter finalizes; an
atexithandler still gets the directory. Without the check, the finalizer's connect raises from inside the hook. - Every connection the package opens, apart from the ADBC driver (finding 6), goes through the one
Connect: memory, file, read only, cursors,:default:andduckdb.sql.SETreplaces the list andRESETempties it, as the engine does. - The hook is called with the GIL held and before the instance cache lock; an exception from it reaches the caller of
connectand ofduckdb.sql. - The duckdb-cli wrapper (
duckdb/duckdb-python-cli,src/duckdb_cli/__main__.py) uses the same rule and the same default.
| return path | ||
|
|
||
|
|
||
| def _site_directory() -> str | None: |
There was a problem hiding this comment.
The pip directory is found only beside duckdb's own dist-info, so an extension wheel installed into another site directory of the same interpreter is never searched (duckdb/_extension_directories.py:58).
Severity: should fix. Category: design. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: look for .duckdb_extensions in every site directory the interpreter uses, in sys.path order, with the same rule for each (writable ones first, read-only ones last); or, if the narrower scope is the decision, say in the description and the release note that an extension wheel must be installed into the same site directory as duckdb.
def _site_directory() -> str | None:
...
for distribution in importlib.metadata.distributions(name="duckdb"):
site = str(Path(str(distribution.locate_file(""))).absolute())The extension wheels do not install beside duckdb; they install into the platlib of whatever scheme pip picks for them. When duckdb sits in a site-packages the user cannot write (a shared or system interpreter, or a container image that installed duckdb as root and runs as another user), pip falls back to a user install on its own (Defaulting to user installation because normal site-packages is not writeable, pip src/pip/_internal/commands/install.py:1018), so pip install duckdb-core-ext-spatial puts the binary in ~/.local/lib/python3.x/site-packages/.duckdb_extensions. A venv that sees the system site packages of an interpreter that has duckdb is the same case. Probe (round-1/probes/p06_user_site.py): duckdb copied into a read-only directory on PYTHONPATH, a stand-in fakeext binary under the user site's .duckdb_extensions, the interpreter's user site enabled:
user site: <round>/userbase/lib/python3.13/site-packages True
duckdb: <venvs>/system-site/duckdb/__init__.py
dirs: []
fakeext listed: []
The description's sentence "for wheel, user-site, target-directory and editable installs alike" holds only when duckdb and the extension wheel went into the same scheme. The wheel's own helper already locates the directory from the extension's side (get_extensions_dir() returns the directory beside its own module, duckdb-python-extensions src/duckdb_extension_wheel/wheel.py:146). The duckdb-cli wrapper has the same limit (os.path.dirname(pkg_dir) alone), so whichever way this goes, the wrapper should follow.
| finally: | ||
| duckdb.default_connection().close() | ||
|
|
||
| def test_engine_lists_an_extension_file_in_the_directory(self, pip_directory): |
There was a problem hiding this comment.
No test pins the behaviour the change exists for: an autoload that finds a pip-installed binary without an install attempt (tests/fast/api/test_pip_extension_directory.py:413).
Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: add an offline autoload test with a stand-in binary of an extension that is not built in, with autoinstall_extension_repository pointed at an empty local directory, for the writable and the read-only position.
Every test reads the setting back, or lists a file through duckdb_extensions(), which marks any file with the right name as installed without opening it. None runs an autoload, so nothing checks that the order the hook produces is the order the engine's install check needs, which is the reason for the rule. The test can run without a network and without a valid binary: with the autoinstall repository set to an empty local directory, a stand-in tpch.duckdb_extension in the pip directory gives a load error (the install was a no op because the first directory holds the file), and a stand-in elsewhere gives an install error. Probe (round-1/probes/p02_autoload_order.py, SELECT count(*) FROM tpch_queries(), autoinstall_extension_repository set to an empty folder):
merge base, binary in the pip directory: Failed to install local extension "tpch", no access to the file at PATH "<round>/empty-repo/29b26cb106/linux_arm64/tpch.duckdb_extension"
head, binary in the pip directory: Failed to load 'tpch', The file is not a DuckDB extension. The metadata at the end of the file is invalid
head, binary only in ~/.duckdb/extensions: Failed to install local extension "tpch", no access to the file at PATH ...
I added the sketch below, with the read-only case, to a copy of the test file: it passes on the head, fails in its read-only case when the rule always puts the directory first, and fails in its writable case when the rule always puts it last (round-1/variants/proposed-test.txt). The test should skip when tpch is linked into the build (install_mode = 'STATICALLY_LINKED' in duckdb_extensions()). The read-only case (deny_creates_in) then pins the documented consequence that autoload installs into the first directory.
A sketch, reusing the file's fixtures (place_stand_in writes tpch.duckdb_extension under <directory>/<version>/<platform>, as assert_engine_lists_a_file_in already computes):
def test_autoload_takes_the_pip_copy_without_an_install(pip_directory, tmp_path):
connection = duckdb.connect(config={"autoinstall_extension_repository": str(tmp_path / "empty-repository")})
place_stand_in(connection, pip_directory, "tpch")
with pytest.raises(duckdb.Error, match="is not a DuckDB extension"):
connection.execute("SELECT count(*) FROM tpch_queries()")| The directories to search, the pip directory once among them, or None when there is no pip directory. | ||
| """ | ||
| directory = _pip_directory() | ||
| if directory is None or not Path(directory).is_dir(): |
There was a problem hiding this comment.
Path(directory).is_dir() raises when the directory cannot be looked up, and that error fails every connect (duckdb/_extension_directories.py:32).
Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: use os.path.isdir(directory), which reads an error as "not a directory".
if directory is None or not Path(directory).is_dir():pathlib's is_dir swallows only "no such file" and a few other error numbers and raises the rest, so a .duckdb_extensions that is a symlink into a directory the user cannot search turns every duckdb.connect() and the default connection into an error, where the merge base connected. Probe (round-1/probes/p11_unsearchable_symlink.py, non-root, Python 3.13.18): connect raised: PermissionError [Errno 13] Permission denied: '<site>/.duckdb_extensions'. The state is rare, but the cost of the extra search path should never be a failed connect; the rest of the lookup already treats a missing or odd directory as "nothing to add".
el-marvin
left a comment
There was a problem hiding this comment.
Addendum to my review above, same head 22819e3, verdict unchanged: changes requested. The review was posted before its last pass finished; this adds what that pass found.
Two findings are new. A connect to a database file that is already open runs the search-path hook and its write probe before the instance cache lookup and then discards the list, which more than doubles the cost of that connect: 73 us on the merge base, 90 us on the head without the directory, 166 to 183 us with it (should fix, performance; details inline on src/pyconnection.cpp). And test_without_duckdb_metadata_on_sys_path_nothing_changes passes even when the lookup ignores sys.path, because no .duckdb_extensions exists anywhere it could look (minor, missing coverage; details inline on tests/fast/api/test_pip_extension_directory.py).
Finding 3 (the release note and UPDATE EXTENSIONS) gained a concrete case: with a writable .duckdb_extensions first, UPDATE EXTENSIONS (tpch) for a tpch installed in ~/.duckdb/extensions now raises "the extension is not installed!". The behaviour changes listed for the maintainer gain one entry: FORCE INSTALL now overwrites the file a wheel installed. With the two new findings the count is 0 blocking, 4 should fix, 4 minor.
| config_dict["duckdb_api"] = Value("python/" + DuckDBPyConnection::FormattedPythonVersion() + " jupyter"); | ||
| } | ||
| config.SetOptionsByName(config_dict); | ||
| ApplyExtensionSearchPath(config); |
There was a problem hiding this comment.
A connect to a database file that is already open runs the hook and its write probe for a result it throws away, which more than doubles the cost of that connect (src/pyconnection.cpp:1745).
Severity: should fix. Category: performance. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: look the database up in the instance cache first and call the hook only when a new database is created; and put before and after figures for a connect to an open file and a :memory: connect, with and without the directory, in the description.
config.SetOptionsByName(config_dict);
ApplyExtensionSearchPath(config);
auto res = FetchOrCreateInstance(database, config);The instance cache returns the open database unchanged, so for that connect the list is discarded, as the description says, but the Python call, the directory check and a file create and unlink in site-packages still run. Connecting to an open file is the cheap path the instance cache exists for (a program that opens the same file per request while another connection holds it). Measured on Release builds made the same way, one thread, 5,000 connects and closes per round, best of five, two alternating runs (round-1/probes/b02_reconnect_cost.py, round-1/bench-reconnect.txt):
merge base: 73.3 73.4 us
head, no .duckdb_extensions: 89.8 92.3 us
head, writable directory: 183.0 165.6 us
DBInstanceCache::GetInstance (external/duckdb/src/main/db_instance_cache.cpp:106) returns the cached database, or nothing, for the same configuration comparison, so the lookup can come first (with the GIL released, as FetchOrCreateInstance does) and the hook run only on a miss; the user options it compares do not hold the list, so its answer does not depend on the hook. test_second_connect_to_an_open_file_keeps_its_list then expects no probe on the second connect instead of one. The code-review run measured the same gap (97 against 236 microseconds); the figures above are mine. A :memory: connect does not show the cost: 26 to 35 ms per connect and close against 7 to 35 microseconds for the hook.
| assert directories(duckdb.connect()) == [] | ||
|
|
||
|
|
||
| def test_without_duckdb_metadata_on_sys_path_nothing_changes(monkeypatch): |
There was a problem hiding this comment.
test_without_duckdb_metadata_on_sys_path_nothing_changes passes even when the lookup ignores sys.path, because no .duckdb_extensions exists anywhere it could look (tests/fast/api/test_pip_extension_directory.py:266).
Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: build the stand-in site with its .duckdb_extensions first (the site fixture), then remove every duckdb dist-info from sys.path, so the test fails when the lookup finds the directory some other way.
def test_without_duckdb_metadata_on_sys_path_nothing_changes(monkeypatch):
monkeypatch.setattr(sys, "path", [entry for entry in sys.path if not any(Path(entry).glob("duckdb-*.dist-info"))])
assert directories(duckdb.connect()) == []With _site_directory replaced by return _PACKAGE_PARENT, the test still passes (round-1/variants/V19-ignore-sys-path.txt: two location tests fail, this one passes); search_path returns None only because the directory beside the package does not exist. The code-review run found this; I reproduced it.
… search for a database already open
…op the writability probe
el-marvin
left a comment
There was a problem hiding this comment.
Changes requested.
Reviewed commit e872206, two commits on the round 1 head 22819e3: e58b855 answers the first review, and e872206 reworks the change to the maintainer's decision that the pip directories always go last, after the configured list or the engine default. I rebuilt the head (Release, Python 3.13), reused the merge base build c2556ed made the same way, and kept a private copy of the released 2.0.0; every probe ran offline in a network namespace with no network and an empty HOME, from copies of each build. The premise still holds: on 2.0.0 and on the merge base the description's program prints [] twice; on the head it prints ['~/.duckdb/extensions', '<site-packages>/.duckdb_extensions'] and the stand-in's path. The Python package is the right layer for finding the site directories, and with the directories last the change no longer competes with the engine's first directory rules: INSTALL, UPDATE EXTENSIONS and the precedence of ~/.duckdb/extensions behave as on the merge base, and what is left (autoload of a wheel-only extension) is the engine's, named and open upstream. Every finding of the last round is addressed, each checked by rerunning its reproducer, and the writable first order, its write probe and their tests are gone from the tree, not switched off. The tests are strong: of 27 weakenings of the Python module and 5 of the C++ side, built and run one at a time, all but one turn the file red. Two things keep this from landing: the cache lookup that answered the last round's performance finding gives up the GIL once more per connect, which doubles a connect to an open file while another Python thread runs, and the autoload test pins the documented limit as its expected result. Three minor findings follow.
Since the last review
- The directory was found only beside duckdb's own dist-info. Addressed: with duckdb in a read-only directory on
PYTHONPATHand a stand-in in the user site, the head lists<userbase>/lib/python3.13/site-packages/.duckdb_extensionsafter the default andduckdb_extensions()reports the file; with the user site disabled the list stays empty. A venv made with system site packages over an interpreter that holds duckdb finds the venv's own.duckdb_extensions. This is the first form of what I asked, with the dist-info directory kept for a target directory that holds duckdb and the wheels together. - No test ran an autoload. Addressed:
TestEngineUseruns autoload, LOAD, UPDATE EXTENSIONS and INSTALL offline against stand-ins and an empty local repository, and it fails when the directories are put first. Its form is the subject of finding 2 below. - UPDATE EXTENSIONS stopped seeing
~/.duckdb/extensions. Addressed by decision: with the directories last, a stand-in in~/.duckdb/extensionsgivesMISSING_INSTALL_INFOforUPDATE EXTENSIONSand forUPDATE EXTENSIONS (tpch)on the head, as on the merge base. - A connect to an open file ran the hook for a result it threw away. Addressed for one thread: 74.5 to 77.7 us per connect and close on the head with and without the directory against 75.9 to 77.2 us on the merge base (5,000 iterations, best of five, two alternating runs), and the test now counts lookups. The lookup added a GIL round trip; that is finding 1.
duckdb_extensions()reports the later copy while LOAD opens the first. Addressed by the release note sentence, which I checked with 16 byte stand-ins in both directories: the listing names the pip copy, LOAD opens the~/.duckdb/extensionscopy.Path.is_dirraised for a symlink into an unsearchable directory and failed every connect. Addressed: the connect now succeeds with[], and puttingPath.is_dirback fails the new test.- The
sys.pathtest passed with a lookup that ignoressys.path. Addressed: that weakening now fails it. - The ADBC driver does not get the directory. Addressed by decision: the description states the limit; unchanged (
([], 8)from the driver).
The behaviour changes I listed for the maintainer last round (a second download of what ~/.duckdb/extensions holds, INSTALL and UPDATE EXTENSIONS moving to site-packages, a configured list losing its install target) are settled by the decision and none of them happens on the head: INSTALL of a local file writes into ~/.duckdb/extensions, a configured list keeps its first entry first, and a stand-in in ~/.duckdb/extensions autoloads without an install attempt and wins a LOAD over the wheel's copy.
Findings
1. FetchOrCreateInstance now gives up the GIL and takes it back once more on every connect than the merge base did, which doubles the wait of a connect to an open file while another Python thread runs (src/pyconnection.cpp:1690).
Severity: should fix. Category: performance. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: keep the cache lookup and the connection setup in one release scope when the lookup finds the database, and the creation and the connection setup in one scope when it does not; then put figures for a connect with a busy Python thread beside the one thread figures in the description.
The evidence is in the inline comment on src/pyconnection.cpp:1690.
2. The autoload test pins the documented limit as its expected result, so the behaviour the decision wants (autoload finds the wheel's copy once the engine looks in every directory) has no test (tests/fast/api/test_pip_extension_directory.py:547).
Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: split the test: one that LOAD tpch opens the wheel's copy, which holds today, and one that autoload opens it without an install, marked as a strict xfail whose reason names the engine's first directory install check and duckdb/duckdb#28480.
The evidence is in the inline comment on tests/fast/api/test_pip_extension_directory.py:547.
3. With the working directory removed and '' on sys.path (as python -c, the REPL and Jupyter run), every duckdb.connect() and duckdb.sql() fails with FileNotFoundError, where the merge base connects (duckdb/_extension_directories.py:72).
Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: skip a sys.path entry whose absolute path cannot be formed, as the import system does for '' when the working directory is gone.
The evidence is in the inline comment on duckdb/_extension_directories.py:72.
4. No test covers another connect creating the database between the cache lookup and the creation, so replacing GetOrCreateInstance with CreateInstance on the miss path passes the whole file (tests/fast/api/test_pip_extension_directory.py:510).
Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: add a test in which a hook holds the first connect between its lookup and its creation while a second thread opens the same file, and assert that both connections share one database, with the list of the connect that created it.
The evidence is in the inline comment on tests/fast/api/test_pip_extension_directory.py:510.
5. With a wheel's extension listed as installed, UPDATE EXTENSIONS (name) says it is not installed, and the release note does not say that UPDATE EXTENSIONS leaves a wheel's extension alone.
Severity: minor. Category: docs. Evidence: ran. Fix: containment. Origin: engine. Base: widened.
Ask: one sentence in the release note: UPDATE EXTENSIONS covers only the first directory, so a wheel's extension is updated by pip, and UPDATE EXTENSIONS (<name>) for one reports it as not installed.
Probe (p03_update_extensions.py pip, a stand-in tpch only in .duckdb_extensions):
merge base: listed: [('tpch', False, '')]
UPDATE EXTENSIONS (tpch): the extension is not installed!
head: listed: [('tpch', True, '<site>/.duckdb_extensions/29b26cb106/linux_arm64/tpch.duckdb_extension')]
UPDATE EXTENSIONS: []
UPDATE EXTENSIONS (tpch): the extension is not installed!
On the merge base the two views agreed; on the head duckdb_extensions() searches every directory and says installed, while the named update looks only in the first (external/duckdb/src/main/extension/extension_helper.cpp:362). The behaviour is the right one for a file pip owns; only the message is surprising, and the release note is where a user would look. Upstream main is unchanged there.
Smaller things
- The opening sentence of the description, "An extension installed from PyPI is not found when a query asks for it", still holds after this change for a query that autoloads; what the change fixes is the explicit
LOAD, which the third paragraph says. SayingLOADin the first sentence keeps the reader from expecting autoload to work. - With
autoinstall_known_extensionsset to false, autoload does find the wheel's copy (probe: the stand-in in.duckdb_extensionsis opened, no install attempted; the merge base says not found). That is the one way to get offline autoload of a wheel's extension until the engine changes, and worth a clause in the release note's autoload sentence. - The
posix_onlyskip reason still says "POSIX file names and descriptors"; it was written for the write probe tests (test_probe_is_removed_before_it_is_closedused it), which e872206 removed. counting.cache_clear = real.cache_clearincount_lookupsnever runs: the monkeypatch is undone before the autouse fixture clears the cache, and neither test that counts callscache_clearwhile patched.- The
Returns:line ofsearch_pathsays "The configured directories as given, then each pip directory"; with nothing configured the default comes first, as the body above it says. - The duckdb-cli wrapper now orders the other way (first when writable). The description says so; the wrapper should follow the decision in its own repository, and the "What is not promised" section of
PROTOCOL.mdin duckdb/duckdb-python-extensions needs its sentence about this module updated once this ships.
Outside findings not taken
- Codex: no finding beyond the GIL round trip, which is finding 1 with my measurements.
- code-review: a bytes entry on
sys.pathmakes every connect fail. Not taken:importlib.metadata.distributionsraisesTypeErroron such an entry on the merge base too, so it breaks every package's metadata in that process. - code-review: RESET or a runtime SET of
extension_directoriesdrops the directories. Not taken: the description states it, and it is how the engine resets the setting. - code-review: discovery misses wheels in a
sys.pathentry that is neither a site directory nor duckdb's (one entry per wheel as Bazel's rules_python lays them out, an extension installed into a target directory onPYTHONPATHwhile duckdb sits in site-packages); it suggests reading the extension wheels' own metadata instead. Not taken here: the description states the rule and the cases it covers, this is the form the last round asked for, and the layouts named are ones the description does not claim. Reading the wheels'RECORDwould be a fine later change. - code-review: one
tryaround both site functions, so a missinggetsitepackagesskips the user site, and other exception types fail connect. Not taken: a stand-in site module of a frozen interpreter; the description chooses which errors reach the caller. - code-review: the hard coded
~/.duckdb/extensions. Not taken, as last round: the engine keepsDefaultExtensionFoldersprivate and the description states the limit. - code-review: a first open of a file canonicalizes the path twice. Not taken separately: microseconds against a database open; the GIL part is finding 1.
- code-review: a deprecated
extension_directorynaming the pip directory gets it appended again. Not taken: the description states it, and the second search finds nothing new. - code-review: the ADBC driver. The description states it; round 1 finding 8.
- code-review: no test opens a named in-memory database (
:memory:name) twice. Not taken: it takes the same cache path as a file, which the open file tests cover.
Prerequisites in the engine
- Autoload installs into the first directory before it loads:
ExtensionHelper::AutoLoadExtension(external/duckdb/src/main/extension/extension_helper.cpp:393), with the install check on the first directory only (extension_install.cpp:155). Upstream main is unchanged; duckdb/duckdb#28480 is open. Recommendation: ship with the documented limit, as decided, and turn the strict xfail of finding 2 into a plain test when the engine pin includes it. UpdateExtensionandUpdateExtensionsread the first directory only (extension_helper.cpp:341and362), andduckdb_extensions()reports the last copy (src/function/table/system/duckdb_extensions.cpp:145). Upstream main is unchanged. Neither blocks: both are documented or asked for above.
What I verified
- Head e872206, range c2556ed..e872206, three commits, no merge commits; the GitHub diff matches the range line for line. Engine submodule 29b26cb1060 at both ends. Since round 1:
duckdb/_extension_directories.py,src/pyconnection.cpp(e58b855 only) and the test file. The two commits change what the author's reply says and nothing else. - Builds: the head (the coordinator's build), the merge base, and a clone of the head for weakenings, all Release on Python 3.13 by the repository's two step install; released 2.0.0 in a private venv. Probes ran from non-editable copies of each build, and from copies that point at the build tree.
- Probes, offline, empty HOME: the description's program on 2.0.0, merge base and head; the user site with duckdb in a read-only directory; a venv with system site packages over a copied interpreter; autoload, LOAD on the same and on a fresh connection, for a stand-in in the pip directory, in
~/.duckdb/extensions, in both and in neither, writable and read only, autoinstall on and off, against an empty local repository; UPDATE EXTENSIONS and its named form for a stand-in in each directory; two copies with 16 byte stand-ins; the unsearchable symlink; SET and RESET; the ADBC driver; a removed working directory; a bytessys.pathentry. - Tests: the file on the head, non-root, offline,
-n 8with a 600 second timeout:54 passed, 1 skippedfrom the build tree (the skip is the location test for a non-editable install), and55 passedon a non-editable copy run from outside the tree. Fulltests/faston the head,-n 8, with a.duckdb_extensionspresent so every connection of the suite got the new list:1 failed, 17654 passed, 114 skipped, 164 xfailed in 1088.26s, and nothing was written into the directory. The failure istest_binding_pressure_leak.py::TestBindingPressureLeak::test_executemany_leak, which ran into the 600 second timeout ("Query interrupted" after the timeout's stack dump) on a loaded machine; run alone it passes in 591.6 s on the head and 565.9 s on the merge base, so it is a slow test that does not touch extension directories, not this change. - Limited API:
src/pyconnection.cppandsrc/duckdb_python.cppcompiled from the build's own command lines withPy_LIMITED_API=0x030C0000and syntax checking only: both pass on the head. - Weakenings, each applied in a clone of the head and run against the file (the C++ ones rebuilt first): 27 of the Python module (directories first; no existence check;
existsorPath.is_dirforos.path.isdir; deprecated setting ignored; no deduplication; deduplication without normalising; no default; no cache; no UTF-8 or backslash check; first dist-info always; noabsolute; site candidates offsys.pathkept; no user site; user site when disabled; nogetsitepackages; no identity deduplication; unnormalised candidates; nosys.pathorder; dist-info directory always last; dist-info offsys.pathdropped; every exception caught; onlyAttributeErrorcaught;sys.pathignored for the dist-info; the candidate's spelling; no dist-info directory) and 5 ofsrc/pyconnection.cpp(no finalizing check; the result in the user options; the hook beforeSetOptionsByName; no cache lookup;CreateInstanceon a miss). Each fails between 1 and 35 tests, exceptCreateInstanceon a miss (finding 4). - Performance: one thread, connect and close to an open file, merge base against head with and without the directory (finding 1 has the figures); with a busy Python thread, per connect distributions for the merge base, the head and the head with one release scope per path, two alternating runs each, twice; the hook alone, 20,000 calls, best of five: 2.8 to 3.3 us without the directory and 4.3 with it, first lookup 230 to 300 us in a fresh process, in line with the description's figures.
- Outside reviews: Codex, read only, reasoning effort xhigh, report in the round folder; the code-review skill at effort xhigh on the right tree and range, 13 findings, triaged above.
- CI: on this head, at 16:35 UTC one check had passed and the main workflow was still running; none had failed. On 22819e3 the job
Test abi3: py3.12-macosx_x86_64failed in its dependency step:uv syncof the test group timed out fetchinghttps://download.pytorch.org/whl/cpu/torch/("operation timed out" after three retries), before any duckdb code ran; six sibling jobs were cancelled after it and 22 passed. Not this pull request's fault.
What held
- The order: the configured list as given, or the default when nothing is configured and the deprecated setting is unset, then each existing directory the list does not name, in
sys.pathorder; a configured list that names the directory first keeps it first. - Nothing changes when the directory is absent, is a file, or is a symlink into an unsearchable directory; a directory created after a connect is found by the next database.
- Site discovery: the user site, a disabled user site, a site directory off
sys.path, alib64andlibpair, a spelling with a parent step, a dist-info from a finder offsys.path, a site module that cannot name its directories. - A second connect to an open file runs no lookup and keeps its list; a connect after the file is closed looks up again; two threads racing to create the same file share one database, with the list of the connect that created it (my sketch, finding 4).
- No Python runs while the interpreter finalizes; an
atexithandler still gets the directory. - INSTALL of a local file writes into
~/.duckdb/extensionsand leaves the pip directory empty; a copy in~/.duckdb/extensionswins a LOAD; UPDATE EXTENSIONS lists only what~/.duckdb/extensionsholds.
| DUCKDB_PY_ASSERT_GIL_HELD(); | ||
| auto &instance_cache = GetModuleState().instance_cache; | ||
| shared_ptr<DuckDB> database; | ||
| if (cache_instance) { |
There was a problem hiding this comment.
FetchOrCreateInstance now gives up the GIL and takes it back once more on every connect than the merge base did, which doubles the wait of a connect to an open file while another Python thread runs.
Severity: should fix. Category: performance. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: keep the cache lookup and the connection setup in one release scope when the lookup finds the database, and the creation and the connection setup in one scope when it does not; then put figures for a connect with a busy Python thread beside the one thread figures in the description.
if (cache_instance) {
nb::gil_scoped_release release;
database = instance_cache.GetInstance(database_path, config);
}
if (!database) {
ApplyExtensionSearchPath(config);
nb::gil_scoped_release release;
database = instance_cache.GetOrCreateInstance(database_path, config, cache_instance, InstantiateNewInstance);
}
{
nb::gil_scoped_release release;
unique_lock<std::recursive_mutex> lock(res->py_connection_lock);
res->con.SetDatabase(std::move(database));The merge base released the GIL once in this function. The head releases it twice when the cache holds the database, twice for :memory:, and three times for a file that is not open yet. Nothing between the first scope's end and the last scope's start needs the GIL on a cache hit. When another thread is running Python, each release lets that thread take the GIL, and taking it back waits for the switch interval (5 ms by default), so the number of release scopes shows up directly in the connect time. Probe (b04_connect_under_gil_contention.py): one thread spins in pure Python while the main thread times 300 connects to a file another connection holds open, and 100 :memory: connects, each timed alone; Release builds of the merge base, the head and the head with the change below, the same layout, two alternating runs each:
connects to an open file that took 10 ms or more, out of 300
merge base: 1, 3
head, no .duckdb_extensions: 262, 135
head, with it: 66, 26
head with one scope: 2, 3 (no directory), 0, 0 (with it)
median :memory: connect
merge base: 29.1, 31.5 ms
head: 42.6, 40.0 ms (no directory), 42.7, 43.8 ms (with it)
head with one scope: 34.1, 35.7 ms (no directory), 35.9, 40.9 ms (with it)
The connect times fall into 5 ms steps, one per wait. On the merge base and with one scope a connect waits at most once (0 to 10 ms); on the head the 10 to 15 ms step appears, which is the second wait. A second, earlier pair of runs of the merge base and the head gave the same picture (194, 137, 166 and 11 slow connects on the head against 0 and 2 on the merge base). With one thread the head matches the merge base, as the description says (74.5 to 77.7 us against 75.9 to 77.2 us per connect and close to an open file here), so the one thread benchmark cannot see this. The change I measured, which passes every test in the file:
auto attach = [&res](shared_ptr<DuckDB> database) {
unique_lock<std::recursive_mutex> lock(res->py_connection_lock);
res->con.SetDatabase(std::move(database));
res->con.SetConnection(make_uniq<Connection>(res->con.GetDatabase()));
};
if (cache_instance) {
nb::gil_scoped_release release;
auto database = instance_cache.GetInstance(database_path, config);
if (database) {
attach(std::move(database));
return res;
}
}
ApplyExtensionSearchPath(config);
{
nb::gil_scoped_release release;
attach(instance_cache.GetOrCreateInstance(database_path, config, cache_instance, InstantiateNewInstance));
}
return res;The :memory: path stays about one wait above the merge base with it, because os.path.isdir in the hook is a stat that releases the GIL too. That one is the price of checking for the directory at every database creation, which the description promises; I do not ask to remove it.
| skip_unless_loadable(connection, "tpch") | ||
| return connection | ||
|
|
||
| def test_autoload_of_a_wheel_extension_attempts_an_install_but_load_finds_it(self, pip_directory, connection): |
There was a problem hiding this comment.
The autoload test pins the documented limit as its expected result, so the behaviour the decision wants (autoload finds the wheel's copy once the engine looks in every directory) has no test.
Severity: should fix. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: split the test: one that LOAD tpch opens the wheel's copy, which holds today, and one that autoload opens it without an install, marked as a strict xfail whose reason names the engine's first directory install check and duckdb/duckdb#28480.
def test_autoload_of_a_wheel_extension_attempts_an_install_but_load_finds_it(self, pip_directory, connection):
place_stand_in(connection, pip_directory, "tpch")
with pytest.raises(duckdb.Error, match="Failed to install local extension"):
connection.execute("SELECT count(*) FROM tpch_queries()")"Failed to install local extension" is the limit the description calls "a limit of this change": the engine installs into its first directory before it loads (external/duckdb/src/main/extension/extension_helper.cpp:393). A test that expects the limit makes the limit the specification. The suite already marks an engine limit the other way: tests/fast/test_variant.py:150 writes the wanted behaviour and marks it xfail(strict=True, reason="Arrow export for VARIANT not yet supported in DuckDB core"). Both forms turn red when the engine pin moves past duckdb/duckdb#28480, but only the xfail states what is wanted, and it carries the reference that tells the next person what to delete. The sketch below xfails on the head, offline, with the file's fixtures (I appended it to a copy of the file in a clone):
@pytest.mark.xfail(strict=True, reason="autoload installs into the first directory before it loads (duckdb/duckdb#28480)")
def test_autoload_finds_the_wheel_copy(pip_directory, tmp_path):
repository = tmp_path / "empty-repository"
repository.mkdir()
connection = duckdb.connect(config={"autoinstall_extension_repository": str(repository)})
skip_unless_loadable(connection, "tpch")
place_stand_in(connection, pip_directory, "tpch")
with pytest.raises(duckdb.Error, match="is not a DuckDB extension"):
connection.execute("SELECT count(*) FROM tpch_queries()")The LOAD half of the current test stays as it is. The test does tell the order apart today: with the directories put first it fails (the autoload opens the stand-in), and with no directory added LOAD says not found.
|
|
||
| entries: dict[str, tuple[int, str]] = {} | ||
| for index, entry in enumerate(sys.path): | ||
| entry = str(Path(entry).absolute()) |
There was a problem hiding this comment.
With the working directory removed and '' on sys.path (as python -c, the REPL and Jupyter run), every duckdb.connect() and duckdb.sql() fails with FileNotFoundError, where the merge base connects.
Severity: minor. Category: correctness. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: skip a sys.path entry whose absolute path cannot be formed, as the import system does for '' when the working directory is gone.
for index, entry in enumerate(sys.path):
entry = str(Path(entry).absolute())Path('').absolute() calls os.getcwd(), which raises FileNotFoundError once the directory has been removed. The import system and importlib.metadata keep working in that state; the lookup does not, and since functools.cache keeps no exception, every later connect fails the same way, with a message that names no file, whether or not any .duckdb_extensions exists. Probe (p12_cwd_removed.py, a temporary directory removed after os.chdir into it):
merge base: connect 1: ok [] connect 2: ok [] duckdb.sql: [(42,)]
head: connect 1: FileNotFoundError: [Errno 2] No such file or directory
connect 2: FileNotFoundError: [Errno 2] No such file or directory
duckdb.sql: FileNotFoundError: [Errno 2] No such file or directory
It is the principle of the os.path.isdir change from the last round: the extra search path should never be the reason a connect fails. The code-review run found this; I reproduced it.
| assert lookups == [] | ||
| assert directories(second) == directories(first) == [DEFAULT_DIRECTORY, pip_directory] | ||
|
|
||
| def test_connect_after_the_file_is_closed_looks_up_again(self, pip_directory, tmp_path, monkeypatch): |
There was a problem hiding this comment.
No test covers another connect creating the database between the cache lookup and the creation, so replacing GetOrCreateInstance with CreateInstance on the miss path passes the whole file.
Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Ask: add a test in which a hook holds the first connect between its lookup and its creation while a second thread opens the same file, and assert that both connections share one database, with the list of the connect that created it.
The lookup and the creation are now two calls (src/pyconnection.cpp:1692 and :1697), and the second call must look again, because the hook runs between them with the GIL held and another thread can create the database meanwhile. GetOrCreateInstance does look again (external/duckdb/src/main/db_instance_cache.cpp:191), but the tests are sequential: with CreateInstance in its place, built in a clone, the file gives 54 passed, 1 skipped, and only the sketch below fails. A sketch, using the file's helpers:
def test_database_created_by_another_connect_between_lookup_and_creation_is_shared(tmp_path):
import threading
path = str(tmp_path / "file.duckdb")
other_created = threading.Event()
results = {}
def other():
results["other"] = duckdb.connect(path)
other_created.set()
def hook(configured, deprecated_directory):
if threading.current_thread() is threading.main_thread():
thread = threading.Thread(target=other)
thread.start()
assert other_created.wait(30)
thread.join(30)
return ["/first"]
return ["/second"]
_duckdb._set_extension_search_path(hook)
try:
first = duckdb.connect(path)
finally:
_duckdb._set_extension_search_path(_extension_directories.search_path)
first.execute("CREATE TABLE t AS SELECT 1 AS i")
assert results["other"].execute("SELECT i FROM t").fetchall() == [(1,)]
assert directories(first) == directories(results["other"]) == ["/second"]It passes on the head and with the one scope change of the performance finding, and fails with CreateInstance. The same race makes the comment above ApplyExtensionSearchPath ("only for a database about to be created") slightly too strong: two connects can both run the hook, and the result of the one that did not create the database is discarded; "only when the cache holds no database for the path" is exact.
…xpected failure, skip a sys.path entry without a working directory
el-marvin
left a comment
There was a problem hiding this comment.
Changes requested.
Reviewed commit cedcaa2, one commit on top of e872206, the head of round 2 (the whole change is c2556ed..cedcaa2; the engine submodule is unchanged at 29b26cb1060). The commit answers round 2: one release of the GIL per connect path, the autoload limit as a strict expected failure, a skip for a sys.path entry whose working directory is gone, a test of a second connect creating the database during the lookup, and the release note sentences. I built the head (Release, the repository's install), kept the merge base build from round 1, rebuilt my variant clone at the head for the weakenings, and ran everything offline under unshare with an empty home. The premise holds as in rounds 1 and 2: on 2.0.0 the description's program prints [] twice; on the head it prints the default and the site-packages .duckdb_extensions, and duckdb_extensions() lists the file there. The approach sits at the right layer, the order rule is settled, and the round 2 asks are done; I measured the GIL change myself and it does what the description says. One thing must change before merge: the new removed working directory test fails on Windows, which the pull request's CI has not run yet. One minor coverage gap comes with a test you can take as it is.
Since the last review
- Round 2, finding 1 (two releases of the GIL on a cache hit), addressed. With a second thread spinning in pure Python, 300 connects to a file another connection holds open: merge base 0, 0, 0 and 0 of 300 at 10 ms or more over four runs (two with the pip directory, two without), head 0, 0, 0 and 0; medians 5.16 to 5.28 ms on the merge base and 5.17 to 5.24 ms here. A
:memory:connect is one switch interval above the merge base (29.0 to 31.8 ms against 23.7 to 28.1) and a connect to a new file two (35.0 to 39.1 against 25.0 to 29.5), which is what the description says. One thread, a connect and close to an open file: merge base 72.6 and 74.4 microseconds, head 74.2 and 74.9 without the directory and 74.2 and 74.2 with it (5,000 iterations, best of five). - Round 2, finding 2 (the autoload test pinned the documented limit), addressed.
test_autoload_finds_the_wheel_copyasserts the wanted behaviour under a strict mark and xfails on the head; with its fixture's connection givenautoinstall_known_extensions = false, so that autoload no longer installs before it loads, as the engine change would make it, the mark turns into a strict XPASS failure.test_load_finds_the_wheel_copykeepsLOADas a plain test. The reason names the engine behaviour in words and leaves the engine pull request number to the description, which is better than what I asked: a committed file should not carry a pull request number. - Round 2, finding 3 (a removed working directory failed every connect), addressed: the round 2 reproducer now connects twice and runs
duckdb.sql, and the new test fails when_absolutelets theOSErrorthrough. The test has a Windows problem of its own, finding 1 below. - Round 2, finding 4 (no test for another connect creating the database between the lookup and the creation), addressed. I ran what the reply says was not run: with
CreateInstancein place ofGetOrCreateInstanceon the miss path, the new race test is the one failure of the file. The decline ofCreateInstanceholds:CreateInstanceInternalonly asserts that the key is absent (db_instance_cache.cpp:132, a no-op in a release build) and then overwrites the entry (line 137), whileGetOrCreateInstancelooks again under its lock (line 191). - Round 2, finding 5 (release note on
UPDATE EXTENSIONS), addressed. With a copy only in.duckdb_extensions,UPDATE EXTENSIONSreturns no rows andUPDATE EXTENSIONS (tpch)says "the extension is not installed!" whileduckdb_extensions()lists it, as the new sentence says. - Round 2, smaller things: the opening sentence, the
autoinstall_known_extensionsclause and its test, theposix_onlyreason and the duckdb-cli wrapper are done. Thesearch_pathReturns line is better but not yet right in one case (below). - Withdrawn: I called the
counting.cache_clear = real.cache_clearline dead in round 2. I was wrong. With it removed, the two tests that count lookups end in teardown errors,AttributeError: 'function' object has no attribute 'cache_clear', raised from the autouselocated_afreshfixture's teardown at test file line 124, which runs while the patch is still in place. - Round 1: every reproducer rerun on this head gives what it gave in round 2: the autoload order with the empty repository (
pip,home,both,none, the read-only pip directory, andautoinstall_known_extensions = false), the user site holding.duckdb_extensionswhile duckdb sits in a read-onlyPYTHONPATHdirectory (found; left out with-s), a virtual environment with system site packages (its own.duckdb_extensionsfound),duckdb_extensions()againstLOADwith two copies, the ADBC driver ([], as the description states), and the symlink into an unsearchable directory (connect succeeds).
Findings
-
test_removed_working_directory_on_sys_path_is_skipped(tests/fast/api/test_pip_extension_directory.py:278) cannot pass on Windows, which refuses to remove a process's working directory, and the Windows wheel test jobs run it.
Severity: should fix. Category: correctness. Evidence: traced. Fix: root-cause. Origin: duckdb-python. Base: introduced.
Mark it@posix_only. Evidence inline. -
No test pins that the instance cache lookup in
FetchOrCreateInstance(src/pyconnection.cpp:1698) runs without the GIL; removing that release passes the file, and makes a connect hang while another thread shuts down the last connection of the same file with Python work in the shutdown.
Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: widened.
Take the test inline, or say in the comment why the lookup must release. Evidence and the test inline.
Smaller things
- The comment above
FetchOrCreateInstance(src/pyconnection.cpp:1689) says a creation releases the GIL twice; an in-memory creation, plainduckdb.connect()included, skips the lookup and releases once here (the hook'sstatadds one more Python level release to every creation). "Once for a connect to an open file or an in-memory creation, twice for the creation of a file" would hold. - The
search_pathReturns line (duckdb/_extension_directories.py:31) says "or the engine default when there are none"; with only the deprecatedextension_directoryset there is no default (line 38, andtest_deprecated_directory_setting_gets_no_defaultexpects the pip directory alone). "When neither list nor the deprecated directory is set" would hold. - The strict mark on
test_autoload_finds_the_wheel_copyhas noraises=, so any exception counts as the expected failure. Today the failure is theAssertionErrorof a message that does not match (the engine says "Failed to install local extension"). Withraises=AssertionError, an engine change that lets the autoload succeed or fail some other way ("DID NOT RAISE", another exception type) would turn the test red instead of passing as expected. - The split of the old autoload test dropped the check that
LOADon the same connection still finds the wheel copy after the autoload failed, which is the way out the release note gives users. It holds today (my autoload probe: "load same connection: ... is not a DuckDB extension"), and one more line in the strict test's sibling would keep it. test_autoload_without_autoinstall_finds_the_wheel_copytakestmp_pathand does not use it, and theTestEngineUsedocstring says every case pointsautoinstall_extension_repositoryat an empty directory, which this one does not: it stays offline because autoinstall is off.- The description says the strict expected failure "names the engine's first directory install check and duckdb/duckdb#28480"; the mark's reason names the behaviour only, and rightly so. The sentence should say the engine pull request is cited here.
Outside findings not taken
- The code-review skill's remark that the name
AttachDatabaserecallsDatabaseManager::AttachDatabase: naming, left to you. - Everything else both outside reviews raised is in the findings or the smaller things above. Codex found no correctness, lifetime, exception safety or locking defect in the C++ change, and neither did I.
What I verified
- Head cedcaa2, range c2556ed..cedcaa2, delta e872206..cedcaa2 (three files:
duckdb/_extension_directories.py,src/pyconnection.cpp, the test file). The pull request diff matches the local range apart from hunk headers. - Builds, all Release, Python 3.13: the head (the coordinator's build plus one import that finished it), the merge base (from round 1), and a variant clone checked out at the head and rebuilt for each weakening of the C++ code. Probes ran under
unsharewithout network, with an empty home, from copies of each build that never rebuild. - The new test file on the head: 57 passed, 1 skipped (the non-editable install case), 1 xfailed.
- Weakenings of
_extension_directories.py, each run against the file: 28 of 30 make it fail, among them_absolutewithout itsexcept; the two that pass skip only the literal''entry or leave the candidate loop unguarded, which behave the same for every path the site module gives. Weakenings ofsrc/pyconnection.cpp, each rebuilt and run: dropping the finalizing gate, putting the result into the user options, running the hook before the options are set, dropping the cache lookup, running the hook before the lookup, andCreateInstanceon the miss path all fail the file; holding the GIL during the lookup passes it (finding 2). - The busy thread and one thread figures above, alternating merge base and head, with and without the directory.
- The
cache_clearline removed: 2 teardown errors. The strict mark with autoinstall off in its fixture: a strict XPASS failure. - The C++ files the change touches (
src/pyconnection.cpp,src/duckdb_python.cpp) compile underPy_LIMITED_API=0x030C0000with-fsyntax-only. - Full
tests/fastat-n 8with a pip directory present, offline: 1 failed, 17657 passed, 114 skipped, 165 xfailed in 663.64s. The failure istest_query_progress("query finished too quick"), a timing test the change does not touch; alone it passed twice on the head and failed once in two runs on the merge base. An earlier full run ended in out of memory errors (bad allocation) while the machine's memory was taken by other work, and is not counted. - Outside opinions: Codex at reasoning effort xhigh on the delta (two findings, both taken); the code-review skill on the delta, which ran at effort max instead of the high I asked for, because it reads the level only at the start of its arguments, and in one pass without its reviewer agents (one finding taken as finding 1, five as smaller things, one not taken).
- CI on cedcaa2: 10 checks passed and none failed when I looked (20:02 UTC); the two macOS seed jobs have been queued for runners since 17:49, so the
Test abi3jobs, the ones that run the tests on Windows and macOS, had not started. The Windows seed jobs that passed build wheels and skip the tests (CIBW_TEST_SKIP: *).
What held
- One release per path: a connect to an open file releases once, as on the merge base, and the figures match it. Read, not run: an exception from the lookup, the hook, the creation or the setup leaves with the GIL held again before the half built connection is destroyed, since each release scope closes before
res, and the connection's lock guards an object no other thread can see yet. - A second connect racing the first to create a file shares one database and carries the list of the connect that created it; a connect to an open file still runs no lookup; the cache still compares only the user options, so the hook's list never makes a connect fail with "different configuration".
- The removed working directory skip covers both the
sys.pathloop and the site candidates, and on Linux a child whose working directory is gone connects and lists the default and the pip directory. - Everything rounds 1 and 2 checked about the order, the site directories, the paths and the engine's use of the list, rerun on this head.
| """ | ||
|
|
||
|
|
||
| def test_removed_working_directory_on_sys_path_is_skipped(pip_directory, site, tmp_path): |
There was a problem hiding this comment.
test_removed_working_directory_on_sys_path_is_skipped runs on Windows, where a process cannot remove its own working directory, so the child stops at os.rmdir(gone) and the test fails in the Windows wheel test jobs.
Severity: should fix. Category: correctness. Evidence: traced. Fix: root-cause. Origin: duckdb-python. Base: introduced.
The child script (line 262) does:
gone = tempfile.mkdtemp(dir=sys.argv[2])
os.chdir(gone)
os.rmdir(gone)and the test asserts proc.returncode == 0 (line 286). On Windows the current directory of a process is held open without delete sharing, so os.rmdir raises OSError and the child exits 1 before it imports duckdb. CPython's own test of the same situation skips there for that reason (Lib/test/test_importlib/import_/test_path.py, test_deleted_cwd: "EINVAL on Solaris, EBUSY on AIX, ENOTEMPTY on Windows ... platform does not allow the deletion of the cwd"). The test has no platform mark, and for this pull request on_pr.yml calls the wheel workflow with minimal: true and testsuite: all, so test_abi3_wheels in packaging_wheels.yml runs ./tests on windows-2022 and windows-11-arm with Python 3.12 and 3.15. I could not run Windows: on cedcaa2 the two Windows seed jobs passed, but they set CIBW_TEST_SKIP: * (job log of Seed: cp312-win_amd64), and the Test abi3 jobs had not started when I looked, waiting on the macOS seeds.
On Linux the test does what it says: it passes on the head (57 passed, 1 skipped, 1 xfailed for the file) and fails when _absolute lets the OSError through (the only failure of the file with that weakening).
Ask: mark it @posix_only, and let the mark's reason cover it ("POSIX file names, symlinks and a removable working directory"). The situation it guards cannot arise on Windows, so nothing is lost there.
| DUCKDB_PY_ASSERT_GIL_HELD(); | ||
| auto &instance_cache = GetModuleState().instance_cache; | ||
| if (cache_instance) { | ||
| nb::gil_scoped_release release; |
There was a problem hiding this comment.
No test pins that the cache lookup runs without the GIL; with the lookup holding it, a connect hangs while another thread shuts down the last connection of the same file and that shutdown runs Python.
Severity: minor. Category: missing coverage. Evidence: ran. Fix: root-cause. Origin: duckdb-python. Base: widened.
The release here is load-bearing, as the reply to round 2 says: GetInstanceInternal busy-waits while a database of the same path shuts down (external/duckdb/src/main/db_instance_cache.cpp:85, while (!weak_cache_entry.expired()) {}), the entry is dropped only at the end of ~DatabaseInstance (database.cpp:120), and before that ResetDatabases closes attached databases, which for one read through a registered Python filesystem drops a PythonFileHandle whose destructor takes the GIL (src/pyfilesystem.cpp:24 to 27). The merge base had the same wait inside its one release around GetOrCreateInstance, also untested; this change adds a second wait site, and the next edit that tries to save this function's switch interval is the one that would remove the release.
Weakening checked: with nb::gil_scoped_release release; removed from this scope, the whole file still passes (57 passed, 1 skipped, 1 xfailed). The test below hangs on that build (two runs, both ended by its 60 second timeout with subprocess.TimeoutExpired) and passes on the head in under a second (three runs):
LOOKUP_DURING_SHUTDOWN_SCRIPT = """
import os, sys, threading, time
import duckdb
from fsspec.implementations.memory import MemoryFileSystem
finalizing = threading.Event()
closer = None
class SlowlyFinalizedFile:
def __init__(self, inner):
self._inner = inner
def __getattr__(self, name):
return getattr(self._inner, name)
def __del__(self):
if threading.current_thread() is closer:
finalizing.set()
time.sleep(0.5)
class SlowlyFinalizingFS(MemoryFileSystem):
def _open(self, path, mode="rb", **kwargs):
inner = super()._open(path, mode, **kwargs)
return SlowlyFinalizedFile(inner) if mode.startswith("r") else inner
CONFIG = {"memory_limit": "8GB", "threads": 8}
folder = sys.argv[1]
path = os.path.join(folder, "main.duckdb")
seed = duckdb.connect(os.path.join(folder, "attached.duckdb"), config=CONFIG)
seed.execute("CREATE TABLE t AS SELECT 1 AS i")
seed.close()
fs = SlowlyFinalizingFS()
with open(os.path.join(folder, "attached.duckdb"), "rb") as source:
fs.pipe("memory://attached.duckdb", source.read())
con = duckdb.connect(path, config=CONFIG)
con.register_filesystem(fs)
con.execute("ATTACH 'memory://attached.duckdb' AS a (READ_ONLY)")
closer = threading.Thread(target=con.close)
del con
closer.start()
assert finalizing.wait(30)
duckdb.connect(path, config=CONFIG).close()
closer.join(30)
print("connected", not closer.is_alive())
"""
def test_connect_while_the_last_connection_shuts_down_with_python_work(tmp_path):
"""The cache lookup waits for a dying database of the same path; it must not hold the GIL the shutdown needs."""
pytest.importorskip("fsspec")
proc = subprocess.run(
[sys.executable, "-c", textwrap.dedent(LOOKUP_DURING_SHUTDOWN_SCRIPT), str(tmp_path)],
capture_output=True,
text=True,
timeout=60,
check=False,
)
assert proc.returncode == 0, proc.stdout + proc.stderr
assert [line for line in proc.stdout.splitlines() if line.startswith("connected ")] == ["connected True"]The CONFIG line only keeps each database small on a large machine. The file handle's finalizer arms only in the closing thread, because the engine also drops a handle of the attached file during ATTACH, in one of its own threads. The child process turns the hang into a timeout instead of a stuck pytest worker.
Ask: add a test of this kind (this one, or your own form of it), or, if you would rather not, one sentence in the comment above FetchOrCreateInstance saying why the lookup must not hold the GIL.
…home directory in the engine use tests, pin the lookup without the GIL
el-marvin
left a comment
There was a problem hiding this comment.
This is ready to merge.
Reviewed commit 7f5fdc6, one commit on top of cedcaa2, the head of round 3 (the whole change is c2556ed..7f5fdc6; the engine submodule is unchanged at 29b26cb1060). The commit changes the test file, one docstring in duckdb/_extension_directories.py and the comment above FetchOrCreateInstance in src/pyconnection.cpp; the C++ hunk is comment lines only, so the module it builds is the one round 3 measured. It answers both round 3 findings and all six smaller things, and it fixes three Windows failures that round 3 did not foresee. I rebuilt the head (Release, the repository's install), rebuilt my variant clone at the head and once more without the GIL release around the cache lookup, read the four red Windows wheel test logs of cedcaa2, and ran everything offline under unshare with an empty home. The premise and the approach stand as rounds 1 to 3 settled them. One thing is left, outside the code: the wheel workflow on 7f5fdc6 had not reached its Test abi3 jobs when I looked (05:10 UTC: the Windows and Linux seeds and the full API build had passed, the macOS seeds were still queued), and those are the only runs of the new marks, the home_directory change and the new shutdown test on Windows and macOS. Merge when the four Windows legs and the macOS legs are green. The one item below the ledger is a comment nit you may take or leave.
Since the last review
- Round 3, finding 1 (the removed working directory test cannot pass on Windows), addressed. The Windows legs of cedcaa2 bear the finding out:
Test abi3: py3.12-win_amd64,py3.15-win_amd64,py3.12-win_ARM64andpy3.15-win_ARM64each fail it in the child atos.rmdir(gone)withPermissionError: [WinError 32] The process cannot access the file because it is being used by another process. The test now carries@posix_only(test file line 289), whose reason (line 28) names the removable working directory. On Linux it still runs and passes, and it still fails when_absolutelets theOSErrorthrough. - Correction to round 3: I named one Windows failure; each of those four legs fails four tests, the same four on every leg (
4 failed, 17588 passedon py3.12-win_amd64,4 failed, 8498 passedon py3.15-win_ARM64). I missedTestEngineUse::test_copy_in_the_default_directory_wins_a_load,test_update_extensions_reads_the_default_directory[default]andtest_install_writes_into_the_default_directory. In all three the engine did not use the test's home: the load opened the.duckdb_extensionscopy ("Regex pattern did not match", the message names thesite-packagesfile),UPDATE EXTENSIONSreturned[], and the installed file was not under<test home>/.duckdb/extensions. So thehome_directorychange fixes failures that CI showed; it is not a precaution. Your diagnosis holds: the package builds with the static C runtime (CMakeLists.txt:11,set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")), the engine reads the home with_wgetenvfrom that copy (external/duckdb/src/common/file_system.cpp:188), and amonkeypatch.setenvupdates only the interpreter's runtime, so the engine kept the runner's profile.FileSystem::GetHomeDirectory(opener)returns thehome_directorysetting before it looks at any variable (file_system.cpp:366to375), and the client file system thatLOAD,INSTALLandUPDATE EXTENSIONSuse passes its opener (external/duckdb/src/include/duckdb/common/opener_file_system.hpp:169to175). I reproduced the Windows situation on Linux: a copy of the test file whosehomefixture leavesHOMEalone, run with the processHOMEpointed at a second empty directory, so the engine expands~there while the tests expect the fixture's home. The cedcaa2 file fails the same three tests, plus the[pip]case, because the install test's file landed in the shared process home; the 7f5fdc6 file passes (59 passed, 1 skipped, 1 xfailed) and leaves the process home empty. The three tests still test what round 3 verified: with the pip directory put first (P01) or without the default directory (P08), all three and the[pip]case fail at this head. - Round 3, finding 2 (no test pinned that the cache lookup runs without the GIL), addressed.
test_connect_while_the_last_connection_of_the_file_shuts_down(line 353) is my sketch in your form. I rebuilt my variant at 7f5fdc6 withnb::gil_scoped_release release;removed from the lookup scope: the test is the one failure of the file (1 failed, 58 passed, 1 skipped, 1 xfailed), and run 8 times at-n 8it fails 8 of 8 withsubprocess.TimeoutExpiredat its 120 second bound, also with 16 busy loops on the 8 processors the run is pinned to. On the head it passed 48 of 48 at-n 8(each under 1.1 seconds), 24 of 24 with 16 busy loops on 8 processors (slowest 2.4 seconds) and 16 of 16 with 32 busy loops on 4 processors (slowest 4.7 seconds). On a timeoutsubprocess.runkills the child, and no child outlived a run. The new sentence of the comment says why the lookup must release, which is what I asked for if you did not take the test; you took both. - Round 3, smaller things, all taken. The comment above
FetchOrCreateInstanceuses the wording I suggested (one case it misses, below). Thesearch_pathReturns line now names both settings. The strict mark carriesraises=AssertionError: with the fixture's connection givenautoinstall_known_extensions = false, so that autoload behaves as the engine change would make it, the test turns into a strict XPASS failure, and with its query replaced by one that does not fail, pytest's "DID NOT RAISE" now turns it red, where without the argument it counted as the expected failure.test_load_finds_the_wheel_copy_after_a_failed_autoload(line 698) pins the way out the release note gives and fails when no pip directory is found (P27). The unusedtmp_pathand the class docstring are fixed, and the description now says the engine pull request is cited there and not in the test. - Rounds 1 and 2: the revert set of
_extension_directories.py(P01toP30) rerun against the new file gives what it gave in round 3: 28 of 30 fail it, and the two that pass,P29andP30, behave the same as the head for every path the site module gives. The C++ revert set was not rerun: the C++ change in this round is a comment, and no test that caught those reverts was edited apart from the three engine use tests, whichP01andP08show still catch the order and the default.
Smaller things
- The comment above
FetchOrCreateInstance(src/pyconnection.cpp:1689) says an in-memory creation releases the GIL once. That holds for:memory:and the empty path, but a named in-memory database goes through the lookup, sincecache_instance(line 1694) is false only for those two, so its creation releases twice, like a file's. Counted with a debugger breakpoint onPyEval_SaveThreadduring one connect each::memory:1, a new file 2, a file already open 1, a new:memory:named2,:memory:namedagain 1. The wording was my suggestion in round 3. "Once for a connect to an open database or a creation of:memory:, twice for the creation of a file or a named in-memory database" would hold.
What I verified
- Head 7f5fdc6, range c2556ed..7f5fdc6, delta cedcaa2..7f5fdc6 (three files: the test file,
duckdb/_extension_directories.py,src/pyconnection.cpp, the last two docstring and comment lines only). - Builds, all Release, Python 3.13: the head (the coordinator's build plus one import that finished it), the merge base from round 1, and the variant clone at 7f5fdc6, rebuilt for the head, for the lookup without its release, and back to the head.
- The test file on the head: 59 passed, 1 skipped (the non-editable install case), 1 xfailed, the same on the variant at the head before and after the weakening. On the weakened build: 1 failed, 58 passed, 1 skipped, 1 xfailed in 121.26s.
tests/fast/apiat-n 8on the head, offline: 1965 passed, 3 skipped, 1 xfailed in 50.97s.- The full fast suite was not rerun: the delta cannot change the binary and changes one test file, which passes. Round 3's run on cedcaa2 stands (1 failed, 17657 passed, 114 skipped, 165 xfailed; the failure the timing test
test_query_progress, flaky on the merge base too). - The four red Windows legs of cedcaa2, read through their job logs: the four failures above and no other; the two XPASS lines on py3.12-win_amd64 are ADBC tests the change does not touch.
fsspecis not installed on the py3.15-win_ARM64 leg (itstest_fsspec.pyis skipped there), so the new shutdown test runs on the other three Windows legs and skips on that one. - No outside review this round: the delta is tests, a docstring and a comment, and I found no defect in the C++.
- CI on 7f5fdc6: at 05:10 UTC, 8 checks passed and none failed; 4 were pending (the two macOS seeds, queued for runners, and the two Linux sanity jobs), and the
Test abi3jobs, which wait for every seed, had not been created. The full API build with assertions passed and rantests/faston Linux with the new tests among them: 18226 passed, 114 skipped, 165 xfailed. The Windows legs that were red on cedcaa2 have not run on this head yet.
What held
- The
home_directoryroute is the right fix and not a weaker test: the engine and the test agree on one directory on every platform, the assertions are unchanged, and on Linux, whereHOMEand the setting name the same directory, the tests behave as before. - No test in the file makes the engine write into the process home any more: after the run with the process home pointed elsewhere, that home holds nothing.
- The shutdown test needs no luck to catch a lookup that holds the GIL: under three and eight times more busy threads than processors it still told the two builds apart every time, and its child is bounded, so a hang becomes a failure instead of a stuck worker.
An extension installed from PyPI is not found when
LOADasks for it. Theduckdb-core-ext-<name>wheels, one project per DuckDB extension, put the binary under.duckdb_extensions/<version>/<platform>/in site-packages, butduckdb.connect()leavesextension_directoriesat its default, so the engine never looks there:LOAD tpchwith the tpch wheel installed and no network says the extension is not installed, and online the engine downloads a second copy into~/.duckdb/extensions. The program below needs no network: it puts a file where an extension wheel puts its binary and reads the setting back.On 2.0.0 both lines print
[]. Now the first prints['~/.duckdb/extensions', '<site-packages>/.duckdb_extensions']and the second the file's path under that directory.DuckDBPyConnection::Connectnow asksduckdb._extension_directories.search_path, a Python function the package registers with the extension module at import through_duckdb._set_extension_search_path, for the directories of the database it is about to create. It runs afterSetOptionsByName, so it sees the configured list as the engine parsed it, whether that came as a Python list, as a list literal string or under an upper-case key. The result is the configured list as given, followed by every.duckdb_extensionsthat exists and the list does not already name. With nothing configured the list starts from~/.duckdb/extensions, because setting the list replaces the engine default. The engine searches the deprecatedextension_directory, when set, before the whole list; with it set the list holds the.duckdb_extensionsdirectories alone, and no default is added. When no.duckdb_extensionsexists, the configuration is left as given.The directories always come last, so nothing that the engine does with its first directory changes:
INSTALL,FORCE INSTALL,UPDATE EXTENSIONSand autoinstall keep writing into~/.duckdb/extensions, or into the first directory of a configured list, and an extension installed there keeps precedence over a wheel's copy. The engine searches every directory of the list for an explicitLOAD, so a wheel's extension loads withLOAD <name>. Autoload is different: before loading, the engine installs the extension unless its first directory already holds it, so a wheel's extension that is not also installed there is downloaded online and fails offline. That is a limit of this change, not a workaround around it: the engine change that looks for an installed extension in every configured directory before installing is duckdb/duckdb#28480, and once the wheel pins an engine with it, autoload finds the wheel's copy with this same list.The directories are the
.duckdb_extensionsof the site directories the interpreter uses:site.getsitepackages()and, when the user site is enabled,site.getusersitepackages(), each counted only when it is onsys.path, plus the directory holding the duckdb dist-info. pip installs an extension wheel into the platlib of the scheme it picks for that wheel, which is not always the one holding duckdb: when site-packages is read only pip falls back to a user install on its own, so the binary lands in~/.local/lib/python3.x/site-packages/.duckdb_extensions, and a virtual environment with system site packages sees both its own site-packages and the system interpreter's. The dist-info directory covers a target directory or anotherPYTHONPATHentry that holds duckdb and the wheels together, which no site function names; among the duckdb distributions thatimportlib.metadatafinds onsys.path, the one in the directory holding the importedduckdbpackage wins, so a stray dist-info earlier on the path cannot redirect it, and an editable install, whose code lives in the checkout while its dist-info sits in site-packages, has none there and takes the first. One directory under two spellings, as a virtual environment of an interpreter built withlib64puts its site-packages onsys.paththrough alib64andlibsymlink pair, is one entry, decided by the directory's identity on disk; the entries keep their order onsys.path, spelled assys.pathspells them. A site module that cannot name its directories, as a stand-in in a frozen or embedded interpreter may not, adds none, and the dist-info directory still counts. The duckdb-cli wrapper, which does the same for the command line, uses the package's parent directory alone and puts the directory first when it can write into it. The directories are located once per process; whether each exists is checked whenever a database is created, so an extension wheel installed into a running session is picked up by the next database the package creates, while a connect to a file that is still open keeps the list it was opened with. Existence is read withos.path.isdir, which treats a lookup error as "not a directory", so a.duckdb_extensionsthat is a symlink into a directory the process cannot search leaves the configuration alone instead of failing the connect.A configured list that already names one of these directories, as one built with the extension wheels'
get_extensions_dir()helper does, is kept as given, in its order and spelling, and the directory is not appended again; paths are compared afteros.path.normpathandos.path.normcase, so a trailing separator does not make a second entry. A deprecatedextension_directorynaming the same directory is left as it is; the engine then searches it twice and finds nothing new. Two kinds of path are left out because the engine cannot use them: one that is not valid UTF-8, possible only on Linux, since the engine takes paths as UTF-8 strings and the conversion would fail every connect, and, on POSIX, one that holds a backslash, since the engine's path parser treats a backslash as a separator on every platform and would search another directory.Connectlooks the database file up in the instance cache first and asks for the directories only when the cache holds no database for the path. The cache compares the user options, which the result never enters, so its answer is the same before and after the call: a second connect to an open file returns that database with the list it was opened with, without running the function, and cannot fail with "different configuration". When two connects race to create one file, the function runs for both and the creation looks in the cache again, so both share the database with the list of the connect that created it. The lookup and the connection setup share one release of the GIL, as do the creation and the setup, because with another Python thread running every release waits a switch interval of 5 milliseconds. The lookup does release it: it waits while a database of the same path shuts down, and that shutdown may run Python, for one when it drops a file handle of a registered Python filesystem, so a lookup holding the GIL would wait for a shutdown that waits for the GIL. One thread, a connect and close to a file another connection holds open took 60 to 61 microseconds on the base and 60 to 63 here on the final build (5,000 iterations, best of five, two runs each). With a second thread running pure Python (switch interval 5 ms), a connect to an open file took a median of 5.2 to 5.5 milliseconds here against 5.3 to 5.4 on the base, with 0 and 0 of 300 connects at 10 milliseconds or more on the base and 0 and 1 here; a:memory:connect 25 to 26 milliseconds against 20 to 21, one extra wait, which is thestatthat checks for the directory at every database creation; a connect to a new file 34 to 36 milliseconds against 24, two extra waits, the cache lookup that spares the open file path and thatstat. Two runs each, 300 connects to the open file and 100 of the other two per run. An error raised while the directories are chosen, other than a site module failing to name its directories, reaches the caller ofconnectunchanged, and a result other than a list of str or None raises a TypeError that names the function. While the interpreter finalizes, no Python runs: the function is not called, gated by the same check the default connection holder uses, because module globals may already be gone. A finalizer that opens a database at exit keeps its configuration; anatexithandler runs earlier and still gets the directory.Every way the package creates a database gets the list:
duckdb.connectto memory or a file, read only or not, cursors, which share their parent's database, and the default connection behindduckdb.sql, which the sameConnectcreates.connectkeeps its signature.RESET extension_directoriesdrops.duckdb_extensionswith the rest of the list, as the engine resets it. The ADBC driver opens databases through the C API and is not covered. Locating the directories costs a few hundred microseconds once per process (480 to 630 in a fresh process with one site directory on a release build), and each database creation then spends 2 microseconds in the function when no.duckdb_extensionsexists and 3 when one does, against 16 to 18 milliseconds for a:memory:connect and close on the same machine. The engine keeps its default directories private, so the package names~/.duckdb/extensions, the default of every known build; a build configured with anotherEXTENSION_DIRECTORIESloses its default while.duckdb_extensionsexists.For the release notes:
duckdb.connect()now appends the.duckdb_extensionsdirectory that theduckdb-core-ext-<name>wheels install into site-packages toextension_directories, after the configured directories or the engine default~/.duckdb/extensions; without that directory nothing changes. An extension installed from a wheel loads with an explicitLOAD <name>, also offline. Nothing installed in~/.duckdb/extensionschanges behaviour: it keeps precedence over a wheel's copy, andINSTALL,FORCE INSTALL,UPDATE EXTENSIONSand autoinstall keep writing there. Autoload of a wheel's extension that is not also installed in the first directory still downloads it online and fails offline, because the engine checks only its first directory before installing, unlessautoinstall_known_extensionsis false, in which case autoload opens the wheel's copy; that limit lifts when the engine looks for an installed extension in every configured directory (duckdb/duckdb#28480).UPDATE EXTENSIONScovers only the first directory, so a wheel's extension is updated by pip, not by DuckDB, andUPDATE EXTENSIONS (<name>)for one reports it as not installed althoughduckdb_extensions()lists it. With two directories in the list,duckdb_extensions()reports the later directory's copy of an extension present in both, whileLOADopens the copy in the first.The tests build a stand-in for site-packages in a temporary directory, a duckdb dist-info first on
sys.pathwith.duckdb_extensionsbeside it, point the package location at it, empty the interpreter's own site directories through the site module, and read the setting back, withHOMEpointed at an empty directory and, where the engine must expand its default directory, itshome_directorysetting pointed there too, since on Windows the engine reads the home through its own C runtime; nothing is downloaded. They cover the directory appended after the default and after a configured list in each form, an explicit empty list, the deprecated setting, and a list that already names the directory, with and without a trailing separator or in first position, kept as given; the dist-info beside the package winning over an earlier one, the first one counting when none is beside the package, the location found once per process, a directory created after a connect and a relativesys.pathentry; the user site holding.duckdb_extensionswhile duckdb's dist-info sits elsewhere, withduckdb_extensions()listing a file placed there, a disabled user site and a site directory offsys.pathleft out, two site directories kept insys.pathorder after the default or a configured list, one directory named bygetsitepackages(), the user site and the dist-info at once kept once, a site directory reached through a symlink kept once under its first spelling, the dist-info directory kept at itssys.pathposition ahead of a later site directory, agetsitepackages()spelling with a parent step matched to itssys.pathentry, a dist-info a finder reports from outsidesys.pathcounted last, and a site module whose functions are missing or raise adding nothing; paths with spaces, commas, quotes, brackets and non-ASCII letters, each withduckdb_extensions()listing a file placed there; every kind of connection, including a second connect to an open file, which runs no lookup, and a connect after the file was closed, which does; an error from the lookup reaching the caller ofconnectand ofduckdb.sql, and a wrong result type; a subprocess that connects from anatexithandler and from a finalizer at exit; and, offline against stand-in binaries with the autoinstall repository pointed at an empty local directory, what the engine does with the directory last:LOADopens a copy that is only in.duckdb_extensions; autoload of the same copy without an install is the wanted behaviour, written as a strict expected failure whose reason names the engine's first directory install check in words (the engine change is duckdb/duckdb#28480, cited here and not in the test) and that accepts only the assertion of the message mismatch, so it turns red once the engine pin includes the fix or the failure changes shape;LOADon a connection whose autoload of the copy just failed still opens it; autoload withautoinstall_known_extensionsfalse opens that copy today; a second connect racing the first to create a file shares its database; a child process whose working directory was removed still connects (POSIX, since Windows does not let a process remove its working directory); a connect to a file whose last connection is shutting down on another thread, with Python work in that shutdown, completes; a copy in~/.duckdb/extensionswins aLOADover the wheel's,UPDATE EXTENSIONSlists a copy in~/.duckdb/extensionsand not one only in.duckdb_extensions, andINSTALLof a local file writes into~/.duckdb/extensionsand leaves.duckdb_extensionsuntouched; these skip whentpchis linked into the build. On a non-editable install, one more test checks without any patching that the directory is located beside the imported package's own dist-info. Protection cases pin that nothing changes without the directory, with a file in its place, with a symlink into a directory the process cannot search, without duckdb metadata onsys.pathwhile a stand-in site holds the directory, and with a path that is not UTF-8 or holds a backslash. The tests reach into the new module to place the package, so the merge base cannot collect them.