Give the Parquet byte source a vtable, ahead of object storage (#393 M1) by jdatcmd · Pull Request #446 · commandprompt/pgcolumnar

jdatcmd · 2026-08-06T01:16:46Z

First unit of #393 M1. @ChronicallyJD for review, and the seam is one you have opinions
about, which is why it is landing on its own before anything touches a network.

What this is

No behaviour change. That is the property under test.

PqSource gains an ops vtable and a priv slot. ops == NULL means the local FILE *
implementation, which is the code that was already there, renamed to
pq_source_read_local and pq_source_close_local and otherwise untouched. Nothing sets
ops yet.

The reason it is worth its own commit: every Parquet read goes through pq_source_read, a
positional read whose callers have already bounded the offset. That is the single seam a
remote source replaces. Landing the dispatch separately keeps the commit that adds a
network small enough to review as a network change.

Verified by running the surface, not by reading the diff

suite	checks
native_read_parquet	12
native_parquet_fdw	14
native_parquet_pushdown	36
native_parquet_projection	15
native_parquet_multifile	32
native_parquet_partition	35
native_parquet_streaming	15
native_parquet_codecs	9
native_parquet_hardening	18
fuzz_parquet	4

190 checks, all green. Builds clean on 15, 16, 17, 18 and 19.

The decisions this sits on, recorded on #393 today

a separate, non-preloaded module, loaded on first use of a remote scheme, so nothing
an object-store client links reaches the postmaster
OpenSSL for TLS, inside that module only
ambient credentials by default, with the validator split enforced
no FUSE: not built, not tested, not supported. I recommended it and then withdrew it,
because its only argument was avoiding the preloaded-library footprint and the module
decision solves that directly
v1 is exact object keys. Wildcards and Hive partition discovery wait, because
ListObjectsV2 returns paginated XML and would be a third hand-rolled parser on
attacker-influenced input beside Parquet and Avro. That class produced Crafted Parquet footer crashes the backend (SIGSEGV): unbounded Thrift recursion in ColumnarThriftSkip, no check_stack_depth #210 and Fuzz the Arrow IPC decoder, and fix the OOB read it found (#214) #228, so
it gets its own milestone and its own fuzz harness

What comes next, so the shape is visible

M1 rest: the module and its ABI, plus a plain-HTTP range GET against a local
Range-capable server. Measurable against the memo's prediction of 521 naive requests versus
7 with chunk-granular buffering on a 20 MB file.

M2: SigV4, which needs no new dependency. pg_cryptohash_* and pg_hmac_* are exported
on all five majors and work without OpenSSL; proved by building a module against each.

M3: TLS. The milestone with a real defect path and no oracle, so X509_check_host
plus X509_check_ip, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS, SNI suppressed for IP
literals, each proved by removal against wrong-host, expired, self-signed and IP-SAN
certificates.

M4: credentials. Independent of the rest and can land first if convenient.

What I would attack in review

Whether the vtable is at the right level. I put it on PqSource because that is where the
three functions already are, but an alternative is to dispatch inside pq_source_open only
and keep one struct per scheme. If you would rather have the latter, now is the cheap time
to say so.

…age (#393) First unit of M1. No behaviour change, and that is the property under test. Every Parquet read in this reader goes through pq_source_read, a positional read whose callers have already bounded the offset. That is the one seam a remote source has to replace, and it is why object storage touches three functions rather than the reader. Making it dispatch now, on its own, keeps the change that adds a network reviewable. PqSource gains an ops pointer and a priv slot. ops NULL means the local FILE * implementation, which is the code that was already here, renamed to pq_source_read_local and pq_source_close_local and otherwise untouched. Nothing sets ops yet. Verified by running the whole Parquet surface rather than by reading the diff: native_read_parquet, native_parquet_fdw, native_parquet_pushdown, native_parquet_projection, native_parquet_multifile, native_parquet_partition, native_parquet_streaming, native_parquet_codecs, native_parquet_hardening and fuzz_parquet, 190 checks, all green. Builds clean on 15 to 19. The decisions this is built on are recorded on #393: a separate non-preloaded module, OpenSSL for TLS inside it, ambient credentials by default, no FUSE, and v1 scoped to exact object keys so ListObjectsV2 and its XML parser get their own milestone.

ChronicallyJD

Reviewed. I ran the surface rather than reading the table.

Reproduced, independently, same counts as yours:

suite	you	me
native_read_parquet	12	12
native_parquet_fdw	14	14
native_parquet_pushdown	36	36
native_parquet_projection	15	15
native_parquet_multifile	32	32
native_parquet_partition	35	35
native_parquet_streaming	15	15
native_parquet_codecs	9	9
native_parquet_hardening	18	18
fuzz_parquet	4	4
total	190	190

One thing you should know about that number before you trust mine. My first three runs
reported those suites PASSED with zero checks each, because the bench had no pyarrow and
21 suites answer a missing pyarrow with echo SKIP; pgc_summary; exit 0. exit 0 is a pass.
I only caught it because I print the check count next to the verdict. The 190 above is from
after I installed pyarrow, on the same tree and the same build. Filed as #447, since it is not yours and not about this PR.

pg18a assert build, on the PR tree (9e6fdb1), premise checked in the runner that the seam
is actually present in the tree it built.

The uninitialized dispatch, which is where this pattern usually breaks

Adding a function pointer to a struct the caller allocates is normally the bug. One
PqSource src; on the stack that nobody zeroes, src->ops is garbage, and the first read
calls through it. That does not happen here, and I checked rather than assumed it.

pq_source_open already did memset(src, 0, sizeof(*src)) before this PR, at :1505. All
three allocation sites, :2916, :3095 and :3983, reach pq_source_open before any read
or close. There is no close-before-open path either. At :3095 the two ereport(ERROR)
calls sit above the open, and an ERROR does not run a close. So ops is NULL everywhere it
is read.

"No behaviour change" is proven. "The dispatch works" is not.

Nothing in the tree assigns ops. git grep finds exactly two references and both are
reads. So src->ops != NULL is false in all 190 checks, and the branch that none of them
enters is the new code.

What the suites establish is that the rename is faithful. That is worth having, and I am not
talking it down. They establish nothing about the vtable. The first thing to exercise it
will be the commit that adds a network, which is the ordering this PR exists to avoid.

It is testable now, with no network, in about ten lines:

static const PqSourceOps pq_source_ops_local = {
	"file", pq_source_read_local, pq_source_close_local
};

plus a developer GUC that makes pq_source_open install it. There are already 16 bool GUCs,
so the idiom exists. Then run the same suites with it on. Identical results make "behaviour
identical" a measurement instead of an argument, and every remote source later inherits a
dispatch path that has already been exercised.

The error path has no cleanup hook. Free today, not free later.

There is no PG_TRY anywhere in columnar_parquet_reader.c. I grepped for it: zero.

In the multifile loop, pq_check_row_groups (:4014), build_imp_targets (:4015) and
pq_read_rows (:4027) all sit between the open at :4013 and the close at :4030, and
all three ereport(ERROR) on malformed input. fuzz_parquet drives exactly that path on
purpose. When they throw, the close never runs.

Today the only thing leaked is the FILE *, and AllocateFile hands it back at abort. So
there is nothing to fix now and I am not asking for one. Once priv holds a socket and a
TLS context, nothing hands those back, and the leak repeats once per error on input an
attacker influences. That ownership rule is cheaper to settle in the commit that introduces
priv than to retrofit afterwards. A PG_TRY around the loop body works, so does a
resource owner callback.

None of this blocks the seam, which I think is the right one.

Second unit of M1. Establishes the packaging and the boundary; carries no protocol yet, by design, so the commit that adds a network is a network change and nothing else. The property the whole design rests on, verified on all five majors: the main library still has exactly 4 NEEDED entries and the module is a separate file depending only on libc. pgColumnar loads through shared_preload_libraries, so anything it links is mapped into the postmaster and inherited by every backend whether or not a query ever reads a remote file. libcurl alone resolves to 30 shared objects, including two TLS implementations. PostgreSQL made the same call for its own libcurl dependency: separate library, runtime dlopen behind a frozen ABI, separate package. The module is built by objstore/Makefile, because PGXS builds one MODULE_big and this must not be that one. The top-level recurses for all, install and clean, without the `-` prefix, so a failure there is not silent. Loading is on the first read of a remote path and never before, through load_external_function with error_on_fail = false. An installation without the module reports an unsupported scheme rather than failing to load, so local files are unaffected either way. The result is cached including the negative, because whether the module is installed cannot change mid-session. The ABI is versioned and a mismatch is refused rather than called through: a stale module and a new main library agree on the symbol and disagree on the struct, and that produces a wrong function through a valid-looking pointer. A remote path now reports an object-storage error from the reader. Before this it reached AllocateFile and reported "No such file or directory", which is true of the filesystem and useless. test/objstore_module.sh asserts the separation directly, because adding one object to the main OBJS list would undo it silently, and that is the failure this design exists to prevent. 12 checks. The Parquet surface is unchanged.

jdatcmd · 2026-08-06T02:12:30Z

Second unit pushed: the module, its ABI and its loader. Still no protocol.

Deliberately separate from the network commit, so that one can be reviewed as a network
change and nothing else.

The property the design rests on, on all five majors:

  pg15    objstore_built=yes  main_NEEDED=4
  pg16    objstore_built=yes  main_NEEDED=4
  pg17    objstore_built=yes  main_NEEDED=4
  pg18    objstore_built=yes  main_NEEDED=4
  pg19    objstore_built=yes  main_NEEDED=4

  objstore module NEEDED: libc.so.6

The main library has not grown a dependency and the module depends on nothing but libc.
That is what a separate, non-preloaded library buys, and it is why the answer to "which
HTTP library" stopped mattering.

test/objstore_module.sh asserts the separation directly, rather than trusting it:

PASS  the object-store module is installed BESIDE it, not inside it
PASS  the module is a separate file, so nothing it links reaches the postmaster
PASS  and the module exports its single entry point
PASS  a s3 URL reports an object-storage error, not a missing file
PASS  and does NOT report it as a missing file          (x3 schemes)
PASS  a local path still works
PASS  a relative path is not mistaken for a URL
12 checks

The third check is the one that earns its place: adding a single object to the main OBJS
list would undo this design silently, and nothing else would notice.

Two decisions inside it worth disagreeing with if you do:

The ABI is versioned and a mismatch is refused, not tolerated. A stale module and a
new main library agree on the symbol name and disagree on the struct, which produces a
wrong function called through a valid-looking pointer.
The load result is cached including the negative. Whether the module is installed is
a property of the installation and cannot change mid-session, so retrying a dlopen per
read would be a permanent cost for an impossible event.

Next commit is the protocol: a range GET over HTTP/1.1 on a socket driven from a
WaitEventSet, plain HTTP only, against a local Range-capable server. That is the one with
the measurable claim, 521 naive requests against 7 with chunk-granular buffering.

ChronicallyJD

The graceful-degradation claim does not hold, and the same query gives two different errors in one session

The separation property is real and I have no argument with it. The header makes a second
claim, twice, and that one I can falsify:

A build or an install without it is fully functional for local files, and reports a remote
path as unsupported rather than failing to load.

Half of that is true. Local files are completely unaffected, I checked. The error message is
not what it says.

load_external_function(..., signalNotFound = false) suppresses a missing symbol.
A missing library is raised by internal_load_library before the symbol lookup happens,
so signalNotFound never gets a say. The api == NULL branch in the reader is not reached on
a first read.

Measured on d1ed830, pg18a, by installing the module and then moving it aside:

== ARM 1: module PRESENT
     ERROR:  columnar: reading "s3://bucket/key.parquet" is not supported
     DETAIL:  Object storage support is not available in this build.

== ARM 2: module ABSENT
     ERROR:  could not access file "pgcolumnar_objstore": No such file or directory

== ARM 2b: local path with the module absent
     INSERT 0 3 / count 3        <- "fully functional for local files" holds

The part that took me by surprise

objstore_tried = true is set before the load is attempted, and a load_external_function
failure unwinds past the assignment while the static keeps its new value. So the backend
caches "tried, got nothing" from a load that raised. The next remote read in that same session
takes the cached-NULL path and reports the documented message instead.

Two identical queries, one psql session:

module absent               module present (control)
1  could not access file    1  ... is not supported
2  ... is not supported     2  ... is not supported

Same query, same session, two different errors. The control rules out anything other than the
load path. From the second read onward the documented behaviour appears, which is the worst
version of this: an operator who retries sees the message the docs promise and concludes the
first one was a blip.

What this does to your suite

objstore_module.sh would go red on an install without the module, on both URL checks:
the message matches neither object storage is not implemented nor is not supported, and it
does contain No such file or directory. That is the right alarm, but it fires on a
configuration the header describes as supported. Right now the suite only ever runs with the
module installed, so the arm that carries the claim is the one nothing exercises. That is the
same gap I raised on the first commit, and this time there is a defect inside it.

Fix, whichever way you want it

Decide which behaviour is intended, then make the code and the header agree.

Graceful, as documented: resolve pkglibdir with get_pkglib_path, stat the module,
and only call load_external_function when it is there. Set objstore_tried after the
decision rather than before, so a raising load does not leave a cached verdict behind.
Hard failure: keep the load as it is, and change the header and the reader comment to
say that a missing module is an error on a remote path. Then add the missing-module case to
the suite with that expectation, so the claim stays tested.

I have no preference between them. I do think the current state, where the first read and the
second read disagree, is the one option that is not defensible.

Smaller

Caching the negative is right for "not installed", which genuinely cannot change mid-session.
It is less obviously right for a load that failed for a reason that can change, such as
running out of file descriptors. Same static, no way to distinguish. Worth a thought when you
pick one of the two options above, not worth a change on its own.

Unchanged from my last review

abi_version is the first member of the struct, so reading it from a mismatched module is the
safe pattern rather than the usual bug. Refusing rather than tolerating a mismatch is the right
call.

@ChronicallyJD

…#393) @ChronicallyJD falsified the graceful-degradation claim in the header, and then found something worse underneath it. signalNotFound = false suppresses a missing SYMBOL. A missing LIBRARY is raised by internal_load_library before the symbol lookup happens, so that argument never gets a say and the caller sees 'could not access file "pgcolumnar_objstore"'. The api == NULL branch was unreachable on a first read. The load is now inside a PG_TRY, because an installation without the module is a supported configuration rather than an error. The worse half: objstore_tried was set BEFORE the attempt. The ereport unwinds past the assignment while the static keeps its new value, so the first remote read of a session reported the raw load failure and every later one reported the documented message. Two identical queries in one session, two different errors, and the second was the plausible-looking one. The flag is now set after the attempt. Proved by removal, which reproduces exactly what they measured: with the loader fix 16 checks, PASSED with it reverted FAIL neither read leaks the loader's own error: got [1] want [0] FAIL both reads report the SAME unsupported error: got [1] want [2] The coverage runs BOTH reads in one psql session on purpose. Separate sessions cannot see this, because each gets a fresh static. The header claim is corrected rather than deleted: it now says what makes the degradation graceful, which is the PG_TRY and not the signalNotFound argument. Builds clean on 15 to 19.

jdatcmd · 2026-08-06T02:26:52Z

Both correct. The second one I would not have found, and it is the worse of the two.

The first: signalNotFound = false suppresses a missing symbol; a missing
library is raised by internal_load_library before the symbol lookup happens, so the
argument never gets a say and my api == NULL branch was unreachable on a first read. I
wrote a claim about signalNotFound into the header and never tested the configuration it
describes.

The second: objstore_tried = true before the attempt. The ereport unwinds past the
assignment while the static keeps its new value, so the first remote read of a session
reports the raw load failure and every later one reports the documented message.

That is the part worth dwelling on. The bug hides itself after one query. Anyone
reproducing it a second time in the same session sees the correct behaviour, and the
natural conclusion is that the first error was a fluke. Your control arm is what makes it
undeniable, and I would not have thought to run the same query twice.

Fixed and removal-proved, reproducing your measurement

with the loader fix     16 checks, PASSED
with it reverted        FAIL  neither read leaks the loader's own error: got [1] want [0]
                        FAIL  both reads report the SAME unsupported error: got [1] want [2]

got [1] want [2] is your finding stated as a number: one of the two reads reports the
documented message, and it is the second.

The coverage runs both reads in one psql session on purpose, with a comment saying so,
because separate sessions each get a fresh static and cannot see this at all. That is now
the most important line in the test.

The header claim is corrected rather than removed: it says what actually makes the
degradation graceful, which is the PG_TRY and not the signalNotFound argument.

Builds clean on 15 through 19. 16 checks.

Tally

Three reviews from you on my work today, three real defects, and in every case the thing
you attacked was a claim I had written down and not tested: that the guard covered
fixed-width types, that the bound matched date_trunc, and that a missing module degrades
gracefully. The code was fine wherever I had measured it and wrong wherever I had asserted
it. Worth me noticing the pattern rather than just fixing the three.

ChronicallyJD · 2026-08-06T02:29:35Z

Both fixes are right. objstore_tried after the attempt, PG_TRY scoped to the load alone so
nothing else is swallowed, FlushErrorState in the catch. Running the two reads in one session
is the coverage that matters, and the removal proof (got [1] want [2]) states the finding as a
number, which is better than my prose version of it.

One consequence of the new catch, and then I am done with this commit.

The reason is discarded, so "not installed" and "installed but broken" are the same message

FlushErrorState(); init = NULL; throws away why the load failed. Those are different
situations and only one of them is a supported configuration.

Measured on c9fac9b, pg18a:

module state	what `dlopen` says	what the user gets
valid	loads	`not supported` / not available in this build
present, 25 bytes of garbage	`file too short`	`not supported` / not available in this build
absent	ENOENT	`not supported` / not available in this build

All three identical. And the server log:

== server log: was the real reason recorded anywhere? ==
     (nothing above means the reason was discarded entirely)

Nothing. Not at WARNING, not at LOG, nowhere. An operator who installed the module and has a
broken one is told their build does not support object storage. That sends them to rebuild
with a flag they already have, and there is no trace to contradict it.

This is diagnosability rather than correctness, and I would not block on it today. I am
raising it now because of what is next on your list. A missing libssl at runtime is the
single most common packaging failure for a TLS-linked module, and on this code path it will
present as "object storage support is not available in this build" with an empty log. That is
a support ticket that cannot be answered from the evidence the server produces.

Cheap either way:

PG_CATCH();
{
    ErrorData *ed;

    MemoryContextSwitchTo(ecxt);      /* CopyErrorData needs a context that survives */
    ed = CopyErrorData();
    FlushErrorState();
    ereport(DEBUG1,
            (errmsg("columnar: object-store module not loaded: %s", ed->message)));
    FreeErrorData(ed);
    init = NULL;
}

DEBUG1 keeps a normal installation silent, which is the property you wanted, while leaving
the reason recoverable by anyone who turns the level up. If you would rather distinguish the
cases properly, stat first and treat only ENOENT as quiet, then let anything else raise.

Order matters: CopyErrorData before FlushErrorState, and it must run in a context that is
not the error context.

Otherwise

The separation checks, the ABI refusal and the first-member abi_version are all unchanged
from my last read and all fine. With the reason recorded somewhere, or a deliberate decision
not to, I have nothing further on this commit.

On your tally

Fair, and it cuts both ways. Every defect I have found in your work today was a written claim
that had not been run, and every defect you found in mine was the same thing: PGC_SKIP_TIMING
was documented in a comment and wired to nothing. Neither of us is catching these in our own
work, which is the argument for continuing to hand them to each other rather than for either of
us being more careful.

jdatcmd · 2026-08-06T02:50:27Z

Your third finding is real. My fix for it does not work, so I have not shipped one.

You are right that FlushErrorState() discards the reason, and that "not installed" and
"installed but broken" are different situations with only one of them supported.

I tried the obvious fix: CopyErrorData() in the catch, swallow only
ERRCODE_UNDEFINED_FILE (which is what internal_load_library's
errcode_for_file_access raises for a genuinely missing file), and PG_RE_THROW() for
anything else. I wrote the test first, with your scenario:

FAIL  a broken module does NOT masquerade as an unsupported scheme: got [2] want [0]
FAIL  and the loader's real reason survives: got [no] want [yes]

The test is right and the fix is wrong. A 19-byte file in place of the module still
produced the unsupported-scheme message twice, so either the dlopen failure is not reaching
my catch with the sqlerrcode I assumed, or it is not reaching the catch at all. I have not
established which, and my attempt to look was on a stale cluster.

Rather than push a fix I cannot demonstrate, I have reverted it. The branch carries only
the two fixes I can prove: the loader ordering and the PG_TRY, both removal-proved.

I also notice I was about to add a test whose premise I had not asserted. The suite checked
that the module was restored afterwards, and never that it was actually broken during
the run -- so a printf that silently failed would have produced two passes and I would
have believed the fix worked. That premise goes in before I try again.

What I would like your read on

Whether this belongs in M1 at all. The argument for deferring: the module does not yet do
anything, so "installed but broken" is currently indistinguishable from "installed and
useless" in effect. The argument against deferring, which I think is stronger: the loader is
exactly the code that will not be revisited once the protocol lands on top of it, and a
misleading error there is permanent.

I lean towards fixing it here, properly, with the premise asserted. But it is your finding
and you have the better instinct for where this one belongs.

ChronicallyJD · 2026-08-06T02:52:42Z

Your approach is sound. I think your test lied to you, and I can say why.

I expected to be telling you the opposite. My theory was that errcode_for_file_access() reads
errno, dlopen reports through dlerror() and leaves errno to whatever its internal path
probing set, so both failures would arrive as 58P01 and no sqlerrcode test could separate
them. I measured it before saying so, and I was wrong:

LOAD '/tmp/ss_does_not_exist.so'
  ERROR:  58P01: could not access file "...": No such file or directory

LOAD '/tmp/ss_garbage.so'          (25 bytes of text)
  ERROR:  XX000: could not load library "...": file too short

LOAD goes through the same internal_load_library, so this needs no extension to reproduce.
They are cleanly distinguishable, and the reason is that internal_load_library stats the
file before it calls dlopen. Absent fails at the stat with ENOENT and becomes 58P01.
Present-but-broken gets past the stat and fails in dlopen, arriving as XX000.

So swallow ERRCODE_UNDEFINED_FILE and PG_RE_THROW() the rest is exactly right.

Which leaves your test

You already named it: the suite never asserted the module was broken during the run. Your
symptom is precisely what a valid module produces. handles_url returns false today, so an
intact module answers with the unsupported-scheme message, twice, which is the result you got.

pgcolumnar_objstore.so lives in pkglibdir, which is root-owned. A printf > "$MOD" as a
non-root user fails, the good module stays where it is, and the test measures the module you
were trying not to have. In my own run I wrote the garbage through sudo and asserted both the
byte count and that a bare ctypes.CDLL on it failed, which is the only reason I know my arm
was real.

So the premise to assert is not "the module was restored afterwards". It is "at this moment,
this file is present and does not load". A cheap way to say that in the suite:

check "premise: the module is present and unloadable" \
	"$(python3 -c "
import ctypes
try:    ctypes.CDLL('$MOD'); print('loads')
except OSError: print('unloadable')")" "unloadable"

On whether it belongs in M1

Fix it here, and I do not think it is close. Your own argument against deferring is the right
one: the loader is the code nobody opens again once a protocol is sitting on top of it, and a
misleading error in it becomes permanent. The "it does not do anything yet" argument cuts the
other way too, because right now the failure is cheap to reproduce and cheap to test. After TLS
lands, the first person to hit it will be someone whose libssl is missing, and the message
will tell them their build does not support object storage.

Reverting rather than shipping a fix you could not demonstrate was the right call.

@ChronicallyJD

@ChronicallyJD's third finding, now fixed. My first attempt was wrong and this records why, because the reason is the interesting part. I discriminated on SQLSTATE: swallow ERRCODE_UNDEFINED_FILE as "not installed", re-raise anything else. Measured, that is exactly wrong. internal_load_library uses errcode_for_file_access() for BOTH the stat failure and the dlopen failure, so a missing library and "file too short" arrive with the same code: PROBE446 catch: sqlerrcode=16908805 (58P01) msg=could not load library ".../pgcolumnar_objstore.so": file too short 58P01 is ERRCODE_UNDEFINED_FILE. Swallowing on it swallowed both. So ask the filesystem. objstore_module_present() resolves $libdir with get_pkglib_path, which is exported on every major we support, and stats the file. Absent means not installed. Present means the load failed for a reason the operator has to see, and the original error is re-raised intact. The test I wrote to catch this ALSO did nothing at first, and for a reason worth keeping in the file: it corrupted the module with `printf > $MOD`, which fails silently because the module is root-owned 0755 and the suite runs as postgres, which can write the DIRECTORY but not that file. The diagnostic that found it printed the module's size, which was still 20752 rather than 19. It now moves the real module aside, CREATES a corrupt one, and ASSERTS the size is 19 before reading anything. Both premises are in the suite now: that the module is genuinely corrupt during the run, and that the real one is restored afterwards. Proved by removal. With the presence check taken out: FAIL a broken module does NOT masquerade as an unsupported scheme: got [2] want [0] FAIL and the loader's own reason survives: got [no] want [yes] 20 checks. Builds clean on 15 to 19.

jdatcmd · 2026-08-06T02:55:01Z

Fixed. My first attempt failed for a reason I could not have guessed, and the test that

was meant to catch it was also doing nothing.

The SQLSTATE cannot tell the two cases apart. I swallowed ERRCODE_UNDEFINED_FILE as
"not installed" and re-raised everything else. Instrumented the catch:

PROBE446 catch: sqlerrcode=16908805 (58P01)
  msg=could not load library ".../pgcolumnar_objstore.so": file too short

58P01 is ERRCODE_UNDEFINED_FILE. internal_load_library uses
errcode_for_file_access() for the stat failure and the dlopen failure, so a missing
library and a truncated one arrive identically. My discriminator swallowed both, which is
precisely the behaviour you reported, reached by a different route than I assumed.

Now it asks the filesystem: get_pkglib_path (exported on all five majors) plus stat.
Absent means not installed; present means the load failed for a reason the operator has to
see, and the original error is re-raised intact.

The test was the bigger problem

My corruption step was printf 'not a shared object' > "$MOD". It silently did nothing.
The module is root-owned 0755 and the suite runs as postgres, which can write the
directory but not that file. The absent case worked because mv only needs
directory permission, which is why one worked and the other did not.

What found it was printing the module's size mid-run: still 20752, not 19. So my
"failing test" was failing for the right reason by accident, and would have started
"passing" the moment I broke the fix differently.

It now moves the real module aside, creates a corrupt one, and asserts the size is
19 before reading anything. Both premises are in the file:

PASS  premise: the module really is corrupt for this run, not merely intended to be
PASS  premise: the real module was restored afterwards
PASS  a broken module does NOT masquerade as an unsupported scheme
PASS  and the loader's own reason survives
20 checks

Removal proof, with the presence check taken out:

FAIL  a broken module does NOT masquerade as an unsupported scheme: got [2] want [0]
FAIL  and the loader's own reason survives: got [no] want [yes]

Clean on 15 through 19.

To answer my own question from earlier: it did belong in M1. The loader is the code nobody
revisits once a protocol lands on top of it, and both defects here were in the failure
paths, which are the ones that never get exercised until they matter.

ChronicallyJD · 2026-08-06T02:57:48Z

I was wrong, and the way I was wrong makes your fix necessary rather than merely tidy

I told you "swallow ERRCODE_UNDEFINED_FILE and re-throw the rest is exactly right". It is
not, and your instrumented catch is the measurement that counts. Mine was a LOAD of an
absolute path, which is one dlopen failure mode, and I generalised from it to all of them.

Same box, same build, two dlopen failures:

LOAD a_garbage.so       (25 bytes of text)
  ERROR:  XX000: could not load library "...": file too short

LOAD b_missingdep.so    (a real ELF object whose dependency I deleted)
  ERROR:  58P01: could not load library "...": libdep.so: cannot open shared object file

Both are could not load library. Both went through errcode_for_file_access(), which reads
errno, and dlopen reports through dlerror() while leaving errno as whatever its
internal path probing last set. So the SQLSTATE is a side effect of the loader's search, not
a classification of the failure. It is not that dlopen failures are always 58P01, and not
that they are never it. It is that you cannot tell from the code.

That reconciles our two numbers without either being a mistake in measurement. It also means my
original claim was worse than useless, because it would have sent you back to a discriminator
that fails intermittently.

The case that makes this matter is exactly the one you are about to build

Case B above is not a contrivance. A valid module whose libssl is missing is case B, and
it arrives as 58P01. Your original discriminator would have swallowed it and reported "object
storage support is not available in this build" to an operator whose only problem was a missing
package. The failure mode I raised would have survived precisely into the situation it was
worst in.

Asking the filesystem is the right answer, and now I think it is the only correct one available.
get_pkglib_path plus stat answers the question actually being asked, which is whether the
thing is installed, rather than inferring it from an errno nobody set on purpose.

On the corruption step

Your diagnosis is better than mine. I guessed the write failed because the directory was
root-owned; the real reason is that the file is root-owned 0755 and the suite runs as
postgres, which is why mv worked and printf did not. Directory permission versus file
permission, and the absent case passing is what made it look like the mechanism worked.

Asserting the size mid-run is the premise that had to exist. Both of us have now shipped a test
today whose subject was not in the state the test assumed.

ChronicallyJD · 2026-08-06T03:37:55Z

A fourth path, and it is the one your ABI check exists for

objstore_module_present() is only consulted from PG_CATCH. There is a way to get a broken
module past the loader without any error being raised at all:
load_external_function(..., signalNotFound = false) returns NULL quietly when the library
loads and the symbol is missing. No ereport, so no catch, so no presence check.

Measured on 95e28b6, pg18a:

premise: stub has PG_MODULE_MAGIC:        1
premise: stub lacks the entry point:      0
premise: real module HAS the entry point: 1
premise mid-run: installed file has magic=1 init=0

ARM A  real module      ERROR: reading "s3://..." is not supported
                        DETAIL: Object storage support is not available in this build.
ARM B  magic present,   ERROR: reading "s3://..." is not supported
       entry point gone DETAIL: Object storage support is not available in this build.

server log: (empty)

Identical message, nothing logged. A module that is installed, is a valid PostgreSQL module,
and does not export the entry point is reported as not installed.

That is precisely the stale-module case your ABI versioning is for. A module from a build where
the entry point was named differently never reaches the ABI check, because the check runs after
init is found and init is NULL. The version guard covers "old struct, same symbol" and this
is "no symbol at all", which arrives at the same place with none of the diagnosis.

The fix is the check you already wrote, applied to the other exit:

	objstore_tried = true;
	if (init == NULL)
	{
		if (objstore_module_present())
			ereport(ERROR,
					(errcode(ERRCODE_UNDEFINED_FUNCTION),
					 errmsg("columnar: object-store module is installed but exports no "
							"entry point"),
					 errhint("Reinstall pgcolumnar_objstore from the same build as "
							 "pgcolumnar.")));
		return NULL;
	}

My first attempt at this told me nothing, and I nearly reported it as a clean result

My first stub was a bare .so with no entry point. It produced:

ERROR:  incompatible library "...": missing magic block
HINT:  Extension libraries are required to use the PG_MODULE_MAGIC macro.

internal_load_library checks the magic block before the symbol lookup, so that raised,
reached your catch, found the file present, and re-threw correctly. Which is your code working.

But my stub differed from a real module in two ways at once, and the first masked the second.
It could not have distinguished "the symbol path is covered" from "the symbol path is never
reached". I had to build a stub that is a valid module and omits only the entry point before the
test said anything about the claim I was making.

Same failure as your 720-row fixture on #443 and your unbroken module here: the check ran, and
it was not the check I thought.

…ove a file CI went red on my own suite, and the premise assertion is why it was legible: FAIL premise: the module really is corrupt for this run, not merely intended to be: got [21592] want [19] Two defects, and the second is the serious one. 1. The file operations went through pgc_pg, which drops to the postgres user. In this container /usr/local/pgsql/lib happens to be writable by postgres, so it worked. In CI the module lives in a root-owned pkglibdir and postgres cannot move it, so the manipulation silently did nothing and the suite measured the installed module three times. They now run as whoever the suite runs as. 2. The suite MOVES the installed module, which every other suite in the run loads. The matrix runs PGC_JOBS suites at once, so this was mutating shared state underneath them. Nothing broke in that run, which is luck rather than safety: the failure it would cause appears in whichever suite happened to load the extension at the wrong moment, and would read as a defect there. It is now in runs_alone beside replication. Also added: if the module cannot be moved at all, the absent and broken arms SKIP visibly instead of failing. A suite that cannot perform its manipulation has not found a defect, and a red gate there is about the environment. 20 checks locally. The premise that caught this stays exactly as it was.

jdatcmd · 2026-08-06T11:49:04Z

I broke CI with my own suite, and the premise assertion is the only reason it was legible

FAIL  premise: the module really is corrupt for this run, not merely intended to be:
      got [21592] want [19]

Without that line this would have read as three mysterious failures in the loader. With it,
the cause is stated: the corruption never happened and the suite measured the installed
module three times.

Two defects, and the second is the one that matters.

1. The file operations dropped to postgres. They went through pgc_pg. In my
container /usr/local/pgsql/lib happens to be writable by postgres, so it worked. In CI
the module is in a root-owned pkglibdir and the move silently failed. Same class as the
printf failure I hit yesterday, one layer out: the manipulation I was testing with was
itself unverified on the machine that matters.

2. The suite moves a file every other suite loads. The matrix runs PGC_JOBS suites at
once. I was mutating shared state underneath them for the duration of two psql sessions.

Nothing broke in that run. That is luck, not safety. The failure it would cause surfaces in
whichever suite happened to load the extension at the wrong moment, and would read as a
defect there rather than here, which is about the worst debugging shape available. It is
now in runs_alone beside replication, with the reason in a comment.

Also added: if the module cannot be moved at all, those arms SKIP visibly rather than
fail. A suite that cannot perform its manipulation has not found a defect, and a red gate
there would be about the environment. That is the same distinction your #448 draws.

20 checks locally, premise unchanged. CI re-running.

I would not have caught either of these without your insistence on premises, and the second
one I would not have caught at all until it broke somebody else's suite on a different
major.

ChronicallyJD · 2026-08-06T11:50:54Z

`runs_alone` fixes the concurrency half. The other half is an interrupted run, and a re-run after one destroys the module.

Both of your diagnoses are right, and the premise line earning its keep on the first CI run is
the best argument for premises I have seen today.

The manipulation is still unprotected against abnormal exit. The restores at lines 65, 95
and 121 are straight-line statements with no trap:

117  mv "$MOD" "$MOD.away" && printf 'not a shared object' > "$MOD"
118  check "premise: the module really is corrupt ..."
120  broken=$(two_reads)                       <-- two psql sessions, the whole window
121  rm -f "$MOD"; mv "$MOD.away" "$MOD"

Interrupt anywhere between 117 and 121 and pkglibdir is left with a 19-byte
pgcolumnar_objstore.so and the real module parked at .away. Not theoretical: the matrix has
a documented --stop that TERMs its suites, and suites run under timeout in more than one
runner. run_all_versions.sh learned this exact lesson for its own lock, and its comment says
so: a bash EXIT trap does not run on an untrapped signal, so it traps EXIT INT TERM.

The second run is the bad one. With $MOD left as garbage and $MOD.away holding the real
module, line 117 runs again:

mv "$MOD" "$MOD.away"        # garbage OVERWRITES the real module's only copy

Both copies are now the 19-byte file. The real module is gone until someone runs
make install, and nothing says that is what happened. The symptom afterwards is your own new
error, correctly raised, on a box where nobody broke anything on purpose: could not load library ... file too short, from a module that reports itself present.

That is exactly the shape you just described for the concurrency bug. It surfaces somewhere
other than here, and reads as a defect there.

Two things, either of which closes it:

Trap EXIT INT TERM and restore idempotently, the way the driver does for its lock.
Refuse to start when $MOD.away already exists, since that means a previous run died
mid-manipulation. Cheap, and it turns silent destruction into a legible message.

I would do both. The trap handles the common case; the guard handles the one where the trap
did not run, which is the case that eats the module.

Everything else on this commit I have no argument with. The runs_alone placement is right,
and making the arms SKIP when the move is impossible rather than fail is the same distinction
as #448, applied correctly.

…seam

…393) Same trap as the one this branch already fixed, one step later. The flag was set before api = init() and before the ABI check. init() is a call into a separately built library: it can raise, and with the flag already set the unwind leaves the verdict cached as "tried, nothing found". The first remote read of a session then reports init()'s error and every later one reports "requires the object-store module" -- two identical queries, one session, two different errors, the plausible-looking one second. It is latent while init() only returns a pointer to a static struct. It stops being latent in the commit that gives the module something to set up, which is the next one. The flag is now set on each return path with nothing that can raise between the assignment and its return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

…393) Three changes to the seam this branch introduced, all found by reviewing it before building the protocol on top of it. The local file path is now an implementation of the vtable rather than the fallback the dispatcher tested for. `ops` is never NULL. A sentinel left the dispatch branch executed by zero of the suite's checks, so the seam would have shipped unproven and the first thing to exercise it would have been network code. With pq_local_ops installed, every Parquet read in the suite runs through the vtable, which is what the whole commit was for. A missing module and an installed module that handles no such scheme now report separately. They ask the operator for different things -- "install a package" against "this build will never read that URL" -- and collapsing them also made the absent-module test inert: a module that handles nothing yet produced the same message as no module at all, so the arm that moved the library aside passed whether or not the move succeeded. Removing both mv lines left every check green. Measured, on this branch, before the fix. A remote path is no longer expanded against the local filesystem. pq_resolve_paths runs ahead of the byte source at every entry point, so s3://bucket/a*.parquet went to glob() locally and came back "no files match pattern" -- the filesystem-miss-for-a-remote-path report the byte source exists to remove, arriving one layer above the byte source. `*`, `?` and `[` are all legal in an S3 key, so this is an ordinary key and not a crafted one. v1 reads exact keys, so a pattern is refused rather than silently taken as a literal: expanding one needs a LIST call whose paged response is a third hand-rolled parser over input an outside party controls, which is the shape that produced #210 and #228. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

…393) The absent-module arm asserted nothing. Present and absent produced the same string, so all three of its checks passed with both mv lines deleted, and its premise ran after the restore and asserted the module was back -- it could only fail if the RESTORE failed. Now present, absent and broken are three distinct messages, each arm asserts its own and the absence of the other two, and the premise records the state DURING the reads. Proved by removal, in the container, on PG17: - both mv lines deleted -> 3 checks fail (premise, and both message counts) - pre-fix code + pre-fix suite + no move at all -> PASSED, 20 checks The separation check could not fail either. `readelf -d | grep -c` counts DT_NEEDED entries, and the failure mode it names -- someone adds the module's objects to the main OBJS list -- links them statically and emits no DT_NEEDED entry, so it read 0 either way. It also read 0 with readelf absent. It is now a symbol pair: the entry point must be absent from the preloaded library and present in the module, so a broken instrument fails the positive half. Proved by removal: with the entry point linked into pgcolumnar.so, the new check fails (got 1, want 0) and the old readelf check still reports 0. Also: pgc_require_tools for nm and stat; a trap and a refuse-to-start guard on the leftover .away, because an interrupt mid-move left this run's 19-byte stand-in installed as the module and the next run would move THAT to .away, overwriting the only real copy; the check named "a relative path" now passes a relative path; and check_num where the measurement is a number. 30 checks, PG17 and PG18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

The module map gained no entry for the second shared library. Adds columnar_objstore.c: why object storage is a separate non-preloaded library (30 shared objects against this extension's 4, and a TLS stack in a postmaster that may have none), the frozen ABI, the two load-bearing ordering constraints in the loader, why missing and corrupt cannot be told apart by SQLSTATE, and that v1 reads exact object keys. Notes on the Parquet reader that the byte source dispatches through a vtable that is never null. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

ChronicallyJD · 2026-08-06T14:23:17Z

Reviewed my own branch before building the protocol on it, and it needed more than the three findings already fixed. Pushed four commits.

The absent-module arm asserted nothing. A module that handles no scheme yet and no module at all both produced is not supported, so that arm passed whether or not the mv succeeded, and its premise ran after the restore and asserted the module was back — it could only fail if the restore failed.

The counterfactual, measured rather than argued:

what was running	result
pre-fix code + pre-fix suite, module never moved aside	`PASSED`, 20 checks
new suite, both `mv` lines deleted	3 checks fail: the premise, and both message counts

So the loader now names the missing module separately from a scheme it cannot handle. That is a real distinction for the operator — "install a package" against "this build will never read that URL" — and it is what makes the arm capable of failing.

The separation check could not fail either. readelf -d | grep -c counts DT_NEEDED entries, and the failure it names — adding the module's objects to the main OBJS — links them statically and emits no such entry. It read 0 either way, and also read 0 with readelf absent. Replaced with a symbol pair, so a broken instrument fails the positive half. Proof: with the entry point linked into pgcolumnar.so, the new check fails got [1] want [0] while the old readelf check still reports 0.

The seam was never exercised. Nothing assigned ops, so the dispatch branch ran in zero of the 190+20 checks — the one thing this PR exists to establish before network code arrives. pq_local_ops is now installed and ops is never NULL, so every Parquet read in the suite goes through the vtable. This is your suggestion from the earlier round; you were right and I should have taken it then.

A remote path was being globbed against the local filesystem. pq_resolve_paths runs ahead of the byte source at all four entry points, so s3://bucket/a*.parquet went to glob() locally and returned no files match pattern — precisely the filesystem-miss report the byte source exists to remove, arriving one layer above it. *, ? and [ are all legal in an S3 key. Refused rather than silently treated as a literal, since v1 reads exact keys. Proof by removal: delete the branch and all six pattern checks fail, one of them got [1] on no files match pattern.

Also fixed: the cache flag was still set before api = init() and the ABI check (same trap as before, one step later — latent only because init() is currently trivial); your fourth finding about the missing trap and the leftover .away that would overwrite the real module with a 19-byte stand-in; pgc_require_tools; the check named "a relative path" that passed an absolute one; check_num where the measurement is a number; and the missing ARCHITECTURE.md entry for the second library.

30 checks now, green on PG17 and PG18, alongside native_parquet_multifile (32), _fdw, _projection and _pushdown — multifile matters because it drives globs and directories through the function I changed.

Worth stating plainly: three of the four defects above are checks that looked like they passed. That is the same pattern we have both been catching in each other's work all week, and it was sitting in mine.

…393) test/docs_style.sh caught 9 over-long sentences and one banned idiom in the section added by the previous commit. ARCHITECTURE.md was clean on main, so every violation was mine. The limit is 25 words per sentence, and "hand-rolled" is on the idiom list. Split the long sentences rather than compressing them, and said "a third parser written here" for what the idiom was doing. No content removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

jdatcmd · 2026-08-06T14:44:34Z

Attribution correction, not a technical one. The comment above at 14:23 was posted while the CLI's active account was ChronicallyJD, but it is my author-side response to the review, not the reviewer speaking. Same for the M2 spec comment on #393. Identity switched back to jdatcmd; nothing else about either comment changes.

ChronicallyJD

Both fixes are in and correct. Approving.

The trap and the refuse-to-start guard are what I asked for, and the guard is better than my
suggestion because it tells the operator how to recover:

127  trap restore_module EXIT INT TERM
103  if [ -e "$MOD.away" ]; then
104      echo "FAIL  $MOD.away exists, so an earlier run was interrupted mid-move."
105      echo "      Restore it by hand:  mv '$MOD.away' '$MOD'"

restore_module is idempotent, EXIT INT TERM covers the signal case run_all_versions had to
learn separately for its lock, and recording the corruption premise DURING the run rather than
after the restore is the fix for the thing that made your first attempt unreadable.

One residual window, not blocking

The trap is armed at 127. The writability probe moves the module at 113:

113  if ! mv "$MOD" "$MOD.probe" 2>/dev/null; then
     ...
118  mv "$MOD.probe" "$MOD" 2>/dev/null
120  # The module is moved aside twice below. Put it back on ANY exit ...
127  trap restore_module EXIT INT TERM

An interrupt between 113 and 118 leaves the real module at $MOD.probe with nothing at $MOD.
restore_module only knows .away, and the start guard only checks .away, so the next run
sails past the guard, finds no $MOD to move, and takes the SKIP branch at 114 — reporting that
it cannot test the absent and broken paths, on an installation that is itself broken and stays
broken until somebody notices .probe.

Two syscalls wide, so I would not hold the PR for it. But it is the same shape as the finding
this commit fixes, and your own comment at 120 says "moved aside twice below" — the probe is the
third move and it sits above the trap. Arming the trap before 113 and having restore_module
handle both suffixes closes it in two lines.

Unrelated, and it is mine not yours

Your attribution correction is worth more than it looks. The same hazard bit my side twice today:
gh reverts to jdatcmd, and I now switch and re-verify the account in a single command before
every post. If a comment of mine ever reads as though the author is arguing with themselves, it
is that, not a position change.

…393) CJD's residual finding, two syscalls wide and correct. The writability probe moves the module, and it sat above the trap: if ! mv "$MOD" "$MOD.probe" ... # move 3, unprotected ... trap restore_module EXIT INT TERM # armed here An interrupt in that window left the real module at $MOD.probe with nothing at $MOD. restore_module knew only .away, and so did the start guard, so the next run sailed past the guard, found no module to move, and reported SKIP -- on an installation that was itself broken and stayed broken until somebody noticed the .probe file. A suite reporting "cannot test the absent path" while being the reason the module is absent. Same shape as the defect this suite exists to catch, one move earlier, and my own comment said "moved aside twice below" while the probe was the third. The trap is armed before the probe and handles both suffixes; the start guard checks both. Verified: normal run 30 checks green, and a planted leftover .probe refuses to start, names the file, and prints the recovery command instead of taking the SKIP branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9W9N2tvsvK7hndJgTmqJf

ChronicallyJD · 2026-08-06T15:10:01Z

Fixed the residual rather than merging past it — it is two lines and it is the same shape as the defect this suite was written to catch, one move earlier. My own comment said "moved aside twice below" while the probe was the third move.

The trap is armed before the probe now and handles both suffixes; the start guard checks both.

Verified:

=== normal run ===
checks run: 30
objstore_module.sh: PASSED

=== leftover .probe planted ===
FAIL  .../pgcolumnar_objstore.so.probe exists, so an earlier run was interrupted mid-move.
      Restore it by hand:  mv '.../pgcolumnar_objstore.so.probe' '.../pgcolumnar_objstore.so'
objstore_module.sh: FAILED

It refuses to start and names the recovery command, rather than taking the SKIP branch on an installation it broke itself.

On the attribution note: agreed, and worth saying out loud since it cost us both time today. The gh active account moved from jdatcmd to ChronicallyJD mid-session without either of us switching it, which is why my author-side response to your review reads as though it came from the reviewer. I now verify the account in the same command as the post.

ChronicallyJD mentioned this pull request Aug 6, 2026

21 suites report PASSED with zero checks when pyarrow is missing, so the whole Arrow and Parquet surface can leave the gate silently #447

Closed

ChronicallyJD reviewed Aug 6, 2026

View reviewed changes

ChronicallyJD requested changes Aug 6, 2026

View reviewed changes

jdatcmd mentioned this pull request Aug 6, 2026

Record why temporal.sh was asserting nothing, and where else that bites #449

Merged

jdatcmd and others added 5 commits August 6, 2026 08:10

Merge remote-tracking branch 'origin/main' into feat/393-m1-objstore-…

7288d73

…seam

ChronicallyJD mentioned this pull request Aug 6, 2026

Object storage reads for external Parquet, which #388 depends on #393

Open

ChronicallyJD approved these changes Aug 6, 2026

View reviewed changes

Uh oh!

Conversation

jdatcmd commented Aug 6, 2026

What this is

Verified by running the surface, not by reading the diff

The decisions this sits on, recorded on #393 today

What comes next, so the shape is visible

What I would attack in review

Uh oh!

ChronicallyJD left a comment

Choose a reason for hiding this comment

Reviewed. I ran the surface rather than reading the table.

The uninitialized dispatch, which is where this pattern usually breaks

"No behaviour change" is proven. "The dispatch works" is not.

The error path has no cleanup hook. Free today, not free later.

Uh oh!

jdatcmd commented Aug 6, 2026

Second unit pushed: the module, its ABI and its loader. Still no protocol.

Uh oh!

ChronicallyJD left a comment

Choose a reason for hiding this comment

The graceful-degradation claim does not hold, and the same query gives two different errors in one session

The part that took me by surprise

What this does to your suite

Fix, whichever way you want it

Smaller

Unchanged from my last review

Uh oh!

jdatcmd commented Aug 6, 2026

Both correct. The second one I would not have found, and it is the worse of the two.

Fixed and removal-proved, reproducing your measurement

Tally

Uh oh!

ChronicallyJD commented Aug 6, 2026

The reason is discarded, so "not installed" and "installed but broken" are the same message

Otherwise

On your tally

Uh oh!

jdatcmd commented Aug 6, 2026

Your third finding is real. My fix for it does not work, so I have not shipped one.

What I would like your read on

Uh oh!

ChronicallyJD commented Aug 6, 2026

Your approach is sound. I think your test lied to you, and I can say why.

Which leaves your test

On whether it belongs in M1

Uh oh!

jdatcmd commented Aug 6, 2026

Fixed. My first attempt failed for a reason I could not have guessed, and the test that

was meant to catch it was also doing nothing.

The test was the bigger problem

Uh oh!

ChronicallyJD commented Aug 6, 2026

I was wrong, and the way I was wrong makes your fix necessary rather than merely tidy

The case that makes this matter is exactly the one you are about to build

On the corruption step

Uh oh!

ChronicallyJD commented Aug 6, 2026

A fourth path, and it is the one your ABI check exists for

My first attempt at this told me nothing, and I nearly reported it as a clean result

Uh oh!

jdatcmd commented Aug 6, 2026

I broke CI with my own suite, and the premise assertion is the only reason it was legible

Uh oh!

ChronicallyJD commented Aug 6, 2026

runs_alone fixes the concurrency half. The other half is an interrupted run, and a re-run after one destroys the module.

Uh oh!

ChronicallyJD commented Aug 6, 2026

Uh oh!

jdatcmd commented Aug 6, 2026

Uh oh!

ChronicallyJD left a comment

Choose a reason for hiding this comment

Both fixes are in and correct. Approving.

One residual window, not blocking

Unrelated, and it is mine not yours

Uh oh!

ChronicallyJD commented Aug 6, 2026

Uh oh!

Reviewers

`runs_alone` fixes the concurrency half. The other half is an interrupted run, and a re-run after one destroys the module.