Hi Chao,
I have read v1 (both patches), reproduced both problems
on current master, and tested the fix; my reading agrees
with yours and I think the patches are ready.
The mechanism, as I understand it:
get_extension_control_directories() is the one place
that splits extension_control_path into directories.
An empty element is passed through
psprintf("%s/extension", piece) like any other element,
so it produces the literal directory "/extension", which
is then searched and can supply extensions. Duplicate
directories are likewise kept as separate list entries
and rescanned on every lookup; the duplication only
becomes invisible later, when the SRFs dedup by
extension name. Your fix works at the right layer:
empty elements are skipped while the list is built, and
directories are compared after macro expansion and
canonicalization, keeping the first occurrence and its
display metadata.
Reproduction on unpatched master (c07545df, a few
commits past the patch's base eaa5961; v1 applies
cleanly there and I tested your exact commits from the
CFBot branch). I planted a dummy extension in a real
/extension directory at the filesystem root. With
extension_control_path = '$system:' :
SELECT name, default_version, location
FROM pg_available_extensions
WHERE name = 'dummyext';
dummyext | 1.0 | /extension
CREATE EXTENSION dummyext; -- succeeds
The same listing appears for ':$system', for ':'
alone, and for '$system::$system' -- every empty
element adds /extension to the search path. For the
duplicate problem I attached strace to the postmaster
during one pg_available_extensions call with
'$system:$system': the system extension directory was
opened for scanning exactly twice in that single call.
With v1 applied, the identical probes give:
SELECT name, default_version, location
FROM pg_available_extensions
WHERE name = 'dummyext';
(0 rows)
CREATE EXTENSION dummyext;
ERROR: extension "dummyext" is not available
for all four path shapes above, and the strace count
for '$system:$system' drops to exactly one directory
open. The documented special case is preserved:
extension_control_path = '' still means $system
(plpgsql lists with location $system, and the dummy
extension is not found). ':' alone now searches no
directories, which matches the sentence the patch adds
to the documentation, and I confirmed the doc wording
against the behaviour. I also checked duplicates
spelled differently: a custom directory repeated with
a trailing '/.' canonicalizes to one entry, and
'$system' listed alongside the literal system share
directory dedups to one entry that still reports its
location as $system -- the first occurrence really is
kept, as the second commit message says. One boundary
I noted: an element must equal "$system" exactly to get
macro substitution, so '$system/.' behaves as a literal
path, exactly as it did before the patch; and a
whitespace-only element is not treated as empty. Both
are pre-existing semantics, not regressions.
Scope check: I grepped the tree for every user of the
Extension_control_path value. extension.c is the only
code that reads or splits it, and all four consumers --
find_extension_control_filename() behind CREATE
EXTENSION, pg_available_extensions,
pg_available_extension_versions, and
extension_file_exists() -- go through the one function
you patch. The fix is complete as it stands.
On the DEBUG3 logging: I think it earns its place. It
is one elog per surviving directory, invisible unless
someone asks for debug3, and it is also the only
observability into which directories are actually
searched -- without it, the dedup half of this patch
would have almost no testable surface. One caveat
about the tests rather than the code: the TAP test's
"empty elements should not search /extension" checks
are unlike() assertions on that DEBUG3 output, so on
unpatched code they pass vacuously (master emits no
such messages at all). The companion like() checks and
the duplicate-count checks do fail on master, and my
probes above pin the actual behaviour independently,
so this is a small test-strength note, not an objection.
Tests: the new TAP cases cover both
pg_available_extensions and
pg_available_extension_versions for dedup (in both
directory orders) and the empty-element shapes via the
DEBUG3 output; CREATE EXTENSION itself goes through
the same function and I verified it by probe in both
directions. With the patched test file run against
unpatched code, the test fails 6 of its 25 subtests --
exactly the new assertions -- and passes 25/25 with the
patch. In check mode (meson test) the core regression,
isolation, and test_extensions suites pass on both
unpatched master and the patched build. I also ran
make installcheck-world against an installed patched
build: it completed with no failures, 658 tests across
54 suites, including core regression 239/239 and
isolation 134/134. That build had TAP tests disabled,
so the TAP suites did not run there (they did run, and
pass, in the meson build above), and the
pg_stat_statements and test_decoding installcheck
targets were no-ops. CFBot is green on v1 as well.
Small notes, none blocking: the two commit messages
explain the changes well and the split into two commits
is clean, but neither carries a Discussion: trailer.
And since extension_control_path is new in PostgreSQL
18 (commit 4f7f7b03758), both bugs also exist in 18 and
19; how far to backpatch I will leave to the committer.
For the hands-on part I used AI assistance: the builds,
the probe battery, and the test-suite runs were done
with AI assistance, and I reviewed the results and the
patch itself as described above.
Verdict: I am marking this Ready for Committer.
Regards,
Prashant Chinnam
