Skip to content

Add site-packages/.duckdb_extensions to every connection's extension search path - #759

Open
el-marvin wants to merge 5 commits into
duckdb:v2.0-cyanopterafrom
evertlammerts:fix-connect-extension-directory
Open

el-marvin wants to merge 5 commits into
duckdb:v2.0-cyanopterafrom
evertlammerts:fix-connect-extension-directory

Conversation

@el-marvin

@el-marvin el-marvin commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

An extension installed from PyPI is not found when LOAD asks for it. The duckdb-core-ext-<name> wheels, one project per DuckDB extension, put the binary under .duckdb_extensions/<version>/<platform>/ in site-packages, but duckdb.connect() leaves extension_directories at its default, so the engine never looks there: LOAD tpch with 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.

import importlib.metadata
import pathlib

import duckdb

con = duckdb.connect()
version, source_id = con.execute("SELECT library_version, source_id FROM pragma_version()").fetchone()
platform = con.execute("PRAGMA platform").fetchone()[0]
site = pathlib.Path(str(importlib.metadata.distribution("duckdb").locate_file("")))
installed = site / ".duckdb_extensions" / (source_id if "-dev" in version else version) / platform
installed.mkdir(parents=True, exist_ok=True)
(installed / "fakeext.duckdb_extension").write_bytes(b"\0" * 1024)

con = duckdb.connect()
print(con.execute("SELECT current_setting('extension_directories')").fetchone()[0])
print(con.execute("SELECT install_path FROM duckdb_extensions() WHERE extension_name = 'fakeext'").fetchall())

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::Connect now asks duckdb._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 after SetOptionsByName, 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_extensions that 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 deprecated extension_directory, when set, before the whole list; with it set the list holds the .duckdb_extensions directories alone, and no default is added. When no .duckdb_extensions exists, 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 EXTENSIONS and 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 explicit LOAD, so a wheel's extension loads with LOAD <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_extensions of the site directories the interpreter uses: site.getsitepackages() and, when the user site is enabled, site.getusersitepackages(), each counted only when it is on sys.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 another PYTHONPATH entry that holds duckdb and the wheels together, which no site function names; among the duckdb distributions that importlib.metadata finds on sys.path, the one in the directory holding the imported duckdb package 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 with lib64 puts its site-packages on sys.path through a lib64 and lib symlink pair, is one entry, decided by the directory's identity on disk; the entries keep their order on sys.path, spelled as sys.path spells 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 with os.path.isdir, which treats a lookup error as "not a directory", so a .duckdb_extensions that 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 after os.path.normpath and os.path.normcase, so a trailing separator does not make a second entry. A deprecated extension_directory naming 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.

Connect looks 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 the stat that 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 that stat. 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 of connect unchanged, 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; an atexit handler runs earlier and still gets the directory.

Every way the package creates a database gets the list: duckdb.connect to memory or a file, read only or not, cursors, which share their parent's database, and the default connection behind duckdb.sql, which the same Connect creates. connect keeps its signature. RESET extension_directories drops .duckdb_extensions with 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_extensions exists 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 another EXTENSION_DIRECTORIES loses its default while .duckdb_extensions exists.

For the release notes: duckdb.connect() now appends the .duckdb_extensions directory that the duckdb-core-ext-<name> wheels install into site-packages to extension_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 explicit LOAD <name>, also offline. Nothing installed in ~/.duckdb/extensions changes behaviour: it keeps precedence over a wheel's copy, and INSTALL, FORCE INSTALL, UPDATE EXTENSIONS and 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, unless autoinstall_known_extensions is 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 EXTENSIONS covers only the first directory, so a wheel's extension is updated by pip, not by DuckDB, and UPDATE EXTENSIONS (<name>) for one reports it as not installed although duckdb_extensions() lists it. With two directories in the list, duckdb_extensions() reports the later directory's copy of an extension present in both, while LOAD opens 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.path with .duckdb_extensions beside it, point the package location at it, empty the interpreter's own site directories through the site module, and read the setting back, with HOME pointed at an empty directory and, where the engine must expand its default directory, its home_directory setting 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 relative sys.path entry; the user site holding .duckdb_extensions while duckdb's dist-info sits elsewhere, with duckdb_extensions() listing a file placed there, a disabled user site and a site directory off sys.path left out, two site directories kept in sys.path order after the default or a configured list, one directory named by getsitepackages(), 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 its sys.path position ahead of a later site directory, a getsitepackages() spelling with a parent step matched to its sys.path entry, a dist-info a finder reports from outside sys.path counted last, and a site module whose functions are missing or raise adding nothing; paths with spaces, commas, quotes, brackets and non-ASCII letters, each with duckdb_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 of connect and of duckdb.sql, and a wrong result type; a subprocess that connects from an atexit handler 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: LOAD opens 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; LOAD on a connection whose autoload of the copy just failed still opens it; autoload with autoinstall_known_extensions false 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/extensions wins a LOAD over the wheel's, UPDATE EXTENSIONS lists a copy in ~/.duckdb/extensions and not one only in .duckdb_extensions, and INSTALL of a local file writes into ~/.duckdb/extensions and leaves .duckdb_extensions untouched; these skip when tpch is 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 on sys.path while 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.

@el-marvin el-marvin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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_extensions first, extensions installed earlier into ~/.duckdb/extensions no longer autoload offline (probe: merge base loads the stand-in from ~/.duckdb/extensions; head tries to download tpch and fails), and online they are downloaded again into site-packages.
  • INSTALL, FORCE INSTALL and autoinstall write into site-packages, which pip neither tracks nor removes, and UPDATE EXTENSIONS covers only that directory (finding 3).
  • A list the user configured on purpose loses its install target: with a writable pip directory, INSTALL writes into site-packages instead of the first configured directory (probe p02_autoload_order.py with a configured one-entry list that holds a stand-in tpch, 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_extensions last, autoload of a pip-installed extension downloads online and fails offline; an explicit LOAD, or autoload with autoinstall_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.md in duckdb/duckdb-python-extensions ("The duckdb Python module does not read site-packages/.duckdb_extensions yet") 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/extensions drops a build's own default directories. Not taken: the description states the limit; the engine keeps DefaultExtensionFolders private (external/duckdb/src/include/duckdb/main/extension_helper.hpp:278 to 284), and conda-forge's python-duckdb recipe builds the sdist without EXTENSION_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]) and InstallExternalExtension (extension_install_dynamic.cpp:454, fs.FileExists(local_extension_path) && !options.force_install). Upstream main is unchanged; duckdb/duckdb#28480 is open.
  • UpdateExtensions lists 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 step uv sync install on Python 3.13, Release. A variant clone in 759/variant for 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 (or unshare -cn where a read-only directory must hold, since the mapped root of -r writes 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 fail duckdb_extensions() the same way); SET and RESET of the list; a database opened through the ADBC driver. Scripts and outputs are in round-1/probes/.
  • Tests: pytest tests/fast/api/test_pip_extension_directory.py -n 8 with 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 8 with a 600 second timeout, offline, non-root, on the head with a writable .duckdb_extensions present, 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, exists for is_dir, deprecated setting ignored, no deduplication, deduplication without normalising, no default, no cache, no UTF-8 check, no backslash check, first dist-info always, no absolute, unlink after close, no unlink, os.access, only open errors caught, every exception caught) and 3 of src/pyconnection.cpp (no finalizing check, the result written into the user options, the hook called before SetOptionsByName). 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. Script round-1/probes/b01_connect_cost.py, figures in round-1/bench-connect.txt.
  • Limited API: src/pyconnection.cpp and src/duckdb_python.cpp, compiled from the build's own command lines with Py_LIMITED_API=0x030C0000 and 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_directory given 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 atexit handler 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: and duckdb.sql. SET replaces the list and RESET empties 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 connect and of duckdb.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:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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()")

Comment thread duckdb/_extension_directories.py Outdated
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():

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 el-marvin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment thread src/pyconnection.cpp Outdated
config_dict["duckdb_api"] = Value("python/" + DuckDBPyConnection::FormattedPythonVersion() + " jupyter");
}
config.SetOptionsByName(config_dict);
ApplyExtensionSearchPath(config);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@el-marvin el-marvin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

  1. The directory was found only beside duckdb's own dist-info. Addressed: with duckdb in a read-only directory on PYTHONPATH and a stand-in in the user site, the head lists <userbase>/lib/python3.13/site-packages/.duckdb_extensions after the default and duckdb_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.
  2. No test ran an autoload. Addressed: TestEngineUse runs 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.
  3. UPDATE EXTENSIONS stopped seeing ~/.duckdb/extensions. Addressed by decision: with the directories last, a stand-in in ~/.duckdb/extensions gives MISSING_INSTALL_INFO for UPDATE EXTENSIONS and for UPDATE EXTENSIONS (tpch) on the head, as on the merge base.
  4. 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.
  5. 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/extensions copy.
  6. Path.is_dir raised for a symlink into an unsearchable directory and failed every connect. Addressed: the connect now succeeds with [], and putting Path.is_dir back fails the new test.
  7. The sys.path test passed with a lookup that ignores sys.path. Addressed: that weakening now fails it.
  8. 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. Saying LOAD in the first sentence keeps the reader from expecting autoload to work.
  • With autoinstall_known_extensions set to false, autoload does find the wheel's copy (probe: the stand-in in .duckdb_extensions is 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_only skip reason still says "POSIX file names and descriptors"; it was written for the write probe tests (test_probe_is_removed_before_it_is_closed used it), which e872206 removed.
  • counting.cache_clear = real.cache_clear in count_lookups never runs: the monkeypatch is undone before the autouse fixture clears the cache, and neither test that counts calls cache_clear while patched.
  • The Returns: line of search_path says "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.md in 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.path makes every connect fail. Not taken: importlib.metadata.distributions raises TypeError on 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_directories drops 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.path entry 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 on PYTHONPATH while 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' RECORD would be a fine later change.
  • code-review: one try around both site functions, so a missing getsitepackages skips 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 keeps DefaultExtensionFolders private 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_directory naming 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.
  • UpdateExtension and UpdateExtensions read the first directory only (extension_helper.cpp:341 and 362), and duckdb_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 bytes sys.path entry.
  • Tests: the file on the head, non-root, offline, -n 8 with a 600 second timeout: 54 passed, 1 skipped from the build tree (the skip is the location test for a non-editable install), and 55 passed on a non-editable copy run from outside the tree. Full tests/fast on the head, -n 8, with a .duckdb_extensions present 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 is test_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.cpp and src/duckdb_python.cpp compiled from the build's own command lines with Py_LIMITED_API=0x030C0000 and 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; exists or Path.is_dir for os.path.isdir; deprecated setting ignored; no deduplication; deduplication without normalising; no default; no cache; no UTF-8 or backslash check; first dist-info always; no absolute; site candidates off sys.path kept; no user site; user site when disabled; no getsitepackages; no identity deduplication; unnormalised candidates; no sys.path order; dist-info directory always last; dist-info off sys.path dropped; every exception caught; only AttributeError caught; sys.path ignored for the dist-info; the candidate's spelling; no dist-info directory) and 5 of src/pyconnection.cpp (no finalizing check; the result in the user options; the hook before SetOptionsByName; no cache lookup; CreateInstance on a miss). Each fails between 1 and 35 tests, except CreateInstance on 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_64 failed in its dependency step: uv sync of the test group timed out fetching https://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.path order; 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, a lib64 and lib pair, a spelling with a parent step, a dist-info from a finder off sys.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 atexit handler still gets the directory.
  • INSTALL of a local file writes into ~/.duckdb/extensions and leaves the pip directory empty; a copy in ~/.duckdb/extensions wins a LOAD; UPDATE EXTENSIONS lists only what ~/.duckdb/extensions holds.

Comment thread src/pyconnection.cpp
DUCKDB_PY_ASSERT_GIL_HELD();
auto &instance_cache = GetModuleState().instance_cache;
shared_ptr<DuckDB> database;
if (cache_instance) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment thread duckdb/_extension_directories.py Outdated

entries: dict[str, tuple[int, str]] = {}
for index, entry in enumerate(sys.path):
entry = str(Path(entry).absolute())

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 el-marvin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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_copy asserts the wanted behaviour under a strict mark and xfails on the head; with its fixture's connection given autoinstall_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_copy keeps LOAD as 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 _absolute lets the OSError through. 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 CreateInstance in place of GetOrCreateInstance on the miss path, the new race test is the one failure of the file. The decline of CreateInstance holds: CreateInstanceInternal only 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), while GetOrCreateInstance looks again under its lock (line 191).
  • Round 2, finding 5 (release note on UPDATE EXTENSIONS), addressed. With a copy only in .duckdb_extensions, UPDATE EXTENSIONS returns no rows and UPDATE EXTENSIONS (tpch) says "the extension is not installed!" while duckdb_extensions() lists it, as the new sentence says.
  • Round 2, smaller things: the opening sentence, the autoinstall_known_extensions clause and its test, the posix_only reason and the duckdb-cli wrapper are done. The search_path Returns line is better but not yet right in one case (below).
  • Withdrawn: I called the counting.cache_clear = real.cache_clear line 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 autouse located_afresh fixture'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, and autoinstall_known_extensions = false), the user site holding .duckdb_extensions while duckdb sits in a read-only PYTHONPATH directory (found; left out with -s), a virtual environment with system site packages (its own .duckdb_extensions found), duckdb_extensions() against LOAD with two copies, the ADBC driver ([], as the description states), and the symlink into an unsearchable directory (connect succeeds).

Findings

  1. 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.

  2. 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, plain duckdb.connect() included, skips the lookup and releases once here (the hook's stat adds 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_path Returns line (duckdb/_extension_directories.py:31) says "or the engine default when there are none"; with only the deprecated extension_directory set there is no default (line 38, and test_deprecated_directory_setting_gets_no_default expects the pip directory alone). "When neither list nor the deprecated directory is set" would hold.
  • The strict mark on test_autoload_finds_the_wheel_copy has no raises=, so any exception counts as the expected failure. Today the failure is the AssertionError of a message that does not match (the engine says "Failed to install local extension"). With raises=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 LOAD on 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_copy takes tmp_path and does not use it, and the TestEngineUse docstring says every case points autoinstall_extension_repository at 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 AttachDatabase recalls DatabaseManager::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 unshare without 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 _absolute without its except; 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 of src/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, and CreateInstance on 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_clear line 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 under Py_LIMITED_API=0x030C0000 with -fsyntax-only.
  • Full tests/fast at -n 8 with a pip directory present, offline: 1 failed, 17657 passed, 114 skipped, 165 xfailed in 663.64s. The failure is test_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 abi3 jobs, 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.path loop 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):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment thread src/pyconnection.cpp
DUCKDB_PY_ASSERT_GIL_HELD();
auto &instance_cache = GetModuleState().instance_cache;
if (cache_instance) {
nb::gil_scoped_release release;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 el-marvin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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_ARM64 and py3.15-win_ARM64 each fail it in the child at os.rmdir(gone) with PermissionError: [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 _absolute lets the OSError through.
  • 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 passed on py3.12-win_amd64, 4 failed, 8498 passed on py3.15-win_ARM64). I missed TestEngineUse::test_copy_in_the_default_directory_wins_a_load, test_update_extensions_reads_the_default_directory[default] and test_install_writes_into_the_default_directory. In all three the engine did not use the test's home: the load opened the .duckdb_extensions copy ("Regex pattern did not match", the message names the site-packages file), UPDATE EXTENSIONS returned [], and the installed file was not under <test home>/.duckdb/extensions. So the home_directory change 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 _wgetenv from that copy (external/duckdb/src/common/file_system.cpp:188), and a monkeypatch.setenv updates only the interpreter's runtime, so the engine kept the runner's profile. FileSystem::GetHomeDirectory(opener) returns the home_directory setting before it looks at any variable (file_system.cpp:366 to 375), and the client file system that LOAD, INSTALL and UPDATE EXTENSIONS use passes its opener (external/duckdb/src/include/duckdb/common/opener_file_system.hpp:169 to 175). I reproduced the Windows situation on Linux: a copy of the test file whose home fixture leaves HOME alone, run with the process HOME pointed 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 with nb::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 8 it fails 8 of 8 with subprocess.TimeoutExpired at 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 timeout subprocess.run kills 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 FetchOrCreateInstance uses the wording I suggested (one case it misses, below). The search_path Returns line now names both settings. The strict mark carries raises=AssertionError: with the fixture's connection given autoinstall_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 unused tmp_path and 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 (P01 to P30) rerun against the new file gives what it gave in round 3: 28 of 30 fail it, and the two that pass, P29 and P30, 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, which P01 and P08 show 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, since cache_instance (line 1694) is false only for those two, so its creation releases twice, like a file's. Counted with a debugger breakpoint on PyEval_SaveThread during one connect each: :memory: 1, a new file 2, a file already open 1, a new :memory:named 2, :memory:named again 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/api at -n 8 on 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. fsspec is not installed on the py3.15-win_ARM64 leg (its test_fsspec.py is 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 abi3 jobs, which wait for every seed, had not been created. The full API build with assertions passed and ran tests/fast on 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_directory route 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, where HOME and 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.

@evertlammerts
evertlammerts added this pull request to the merge queue Nov 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 11, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants