On Mon, Sep 8, 2026, at 3:14 AM, Michael Paquier wrote:
> I am not much a fan of spreading pg_strtok() calls more than
> necessary in this module. How about extending
> test_ext_node_next_token() with an "expected" argument to enforce some
> validation?
Agreed, one chokepoint for pg_strtok() is better. Worth being explicit
in the comment that the two modes fail differently, though: with
expected == NULL the function ereports on a short read, and with a
non-NULL expected it has to hand a mismatch back to the caller so
text_to_test_ext_node() can keep emitting its single "argument is not a
serialized %s" error rather than four separate ones. That's a bit of
behavior keyed off whether an argument is NULL, so it should be
spelled out rather than inferred.
> I am not convinced that we need to check all the input function calls
> in 0001 and 0002. [...] Let's just pick up one, reduce the duplicates.
+1, and for test_bitmapset there's a concrete reason it's safe: every
SQL wrapper decodes through the same PG_ARG_GETBITMAPSET ->
text_to_bitmapset() path, 48 call sites, and there's no second decode
route. So one function exercises the whole thing and the rest is
duplicate coverage. I'd fold test_bms_copy('{VAR}') into
test_bms_num_members() with the others.
I'd keep test_bms_copy('<>'), though. That one isn't testing rejection,
it's checking that an empty set survives the round trip and comes back
out as '<>', which num_members can't show you since it just returns 0.
Different coverage, cheap to keep.
I did confirm the bug is real before the fix: with 0002 reverted,
SELECT test_bms_num_members('{QUERY}') takes the backend down and the
cluster goes into recovery, no privileges needed. So this is worth
fixing even for a test module. Thanks for catching it, Zsolt.
One thing not yet mentioned: after the fix, bad input surfaces as
elog(ERROR) out of _readBitmapset(), so you get
ERROR: XX000: unrecognized token: "{"
XX000 normally means "we hit a bug," not "you passed garbage." Those
elogs are internal-error paths in readfuncs.c that were never meant to
be user-reachable, and the SQL functions here make them reachable. I
don't think this patch should try to fix readfuncs.c, and I'm fine
with it as-is for a test module, but if we're holding this up as a
template for extension authors then the honest version catches the bad
input in text_to_bitmapset() and reports it with
ERRCODE_INVALID_TEXT_REPRESENTATION, the way test_extensible already
does for its own node. Happy to leave that as a follow-up if you'd
rather keep these patches tight.
best,
-greg