fix: replace conflicting registrations instead of aborting the LOAD - #5
Merged
Merged
Conversation
Registering a name the catalog already holds aborts the whole extension load:
`ExtensionLoader::RegisterFunction(PragmaFunction)` and the
`CreateTableFunctionInfo` overload both leave `CreateInfo::on_conflict` at its
ERROR_ON_CONFLICT default. (The TableFunctionSet overload sets ALTER, which is
why this only bites the registrations that build their own info to attach a
FunctionDescription.)
That is about to matter. erpl is dropping its bundled SSH tunnel and leaving
deprecation stubs under these same names, pointing callers here. Without this,
a user who does `LOAD erpl`, hits the stub, and follows its advice --
INSTALL erpl_tunnel FROM 'http://get.erpl.io';
LOAD erpl_tunnel;
-- gets `Pragma Function with name "tunnel_create" already exists!` and a
half-registered extension reporting loaded = false. The migration path the
message recommends was the one path that did not work.
Both were verified against a real erpl build with the stubs in place. Before:
LOAD aborted on `tunnel_create`, and after fixing only the pragmas it aborted on
the `tunnels` table function instead, silently dropping tunnel_peers and
tunnel_self. After: both load orders register all nine functions and the real
implementation serves them.
REPLACE is the right resolution rather than IGNORE: this extension owns these
names, and skipping would leave a stub that throws in place of a working
function.
The v1.4.5 leg pinned vcpkg ce613c41, whose vcpkg_acquire_msys fetches msys2-runtime-3.5.4-2. msys2 has purged that from every mirror -- all six return 404 -- so the Windows build fails deterministically and re-running never helps. Same rot as DataZooDE/erpl#115. The v1.5.5 leg already pins 84bab45d and is green on Linux amd64/arm64, macOS and Windows, so the newer pin is proven in this repo rather than assumed. (erpl needs a separate Windows-only pin because bumping it breaks its manylinux_2_28 Linux container; this repo builds Linux on ubuntu-24.04 runners and has no such constraint.) Unrelated to the registration fix on this branch -- it was failing before it and would fail on main today.
The bump fixed v1.4.5 Windows and broke v1.4.5 Linux arm64. The newer vcpkg ships openssl 3.6.0, which fails to build in that leg's arm64 container -- "building openssl:arm64-linux-release failed with: BUILD_FAILED" -- the same incompatibility erpl hit in its manylinux_2_28 container. The justification was that the pin is "proven in this repo" because the v1.5.5 leg uses it and is green. That established it for the v1.5.5 container only, not this one. Stated with more confidence than the evidence supported. Upstream's _extension_distribution.yml exposes a single vcpkg_commit with no per-platform override, so Windows and arm64 cannot both be satisfied on this leg. arm64 works today, Windows does not, and v1.5.5 covers Windows -- so the old pin is the better trade. The constraint is now a comment at the pin so the next person does not repeat the bump. v1.4.5 Windows therefore stays red. It is pre-existing, unrelated to the registration fix on this branch, and equally red on main.
The two failures had opposing fixes, and neither pin satisfied both:
- ce613c41 (old): vcpkg_acquire_msys fetches msys2-runtime-3.5.4-2, purged from
every msys2 mirror -- all 404, so Windows fails deterministically.
- 84bab45d (upstream default): ships openssl 3.6.0, which now requires Linux
kernel headers the arm64 container does not carry ("openssl requires Linux
kernel headers from the system package manager"), so arm64 fails at configure.
Upstream's _extension_distribution.yml exposes a single vcpkg_commit with no
per-platform override, so an earlier attempt to fix Windows by bumping the pin
simply moved the breakage to arm64.
Taking the newer pin for the msys2 fix and holding openssl at 3.5.0 in
vcpkg.json resolves both: 3.5.0 is exactly what ce613c41 shipped, so it is the
version arm64 was already building successfully. Verified with a vcpkg dry-run
that the override resolves (openssl@3.5.0) and leaves libssh2 and zlib on the
versions they already used.
Both halves are required -- changing either alone turns one platform red -- and
that is recorded at both sites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Registering a name the catalog already holds aborts the whole extension load.
ExtensionLoader::RegisterFunction(PragmaFunction)and theCreateTableFunctionInfooverload both leaveCreateInfo::on_conflictat itsERROR_ON_CONFLICTdefault. (TheTableFunctionSetoverload setsALTER, which is why this only bites the registrations that build their own info in order to attach aFunctionDescription.)Why now
erpl is dropping its bundled SSH tunnel (DataZooDE/erpl#119) and leaving deprecation stubs under these same names, pointing callers here. Without this change, a user who does
LOAD erpl, hits the stub, and follows its advice:gets:
The migration path the message recommends was the one path that did not work.
Verified against a real erpl build with the stubs present
LOAD erpl_tunnelaborted ontunnel_create.tunnelstable function, leaving the extension half-registered —loaded = false, andtunnel_peers/tunnel_selfsilently missing. That second failure is the reason the table-function registrations are included here; fixing the pragmas alone looks like it works until you count the functions.erplthenerpl_tunnel, or the reverse — registers all nine functions, andPRAGMA tunnel_close_allis served by the real implementation.Why REPLACE and not IGNORE
This extension owns these names.
IGNORE_ON_CONFLICTwould leave erpl's stub in place, so the name would resolve to something that only knows how to raise "this moved" — worse than the abort, because it would look like it worked.No behaviour change when no other extension holds the names, which is every case except the migration window.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.