Improve error handling in test modules: test_extensible, test_bitmapset

On Sun, Sep 06, 2026 at 11:22:58AM +0100, Zsolt Parragi wrote:

The recently added test extensible module has a small oversight: it
lacks input validation, which made it easy to crash the backend with
invalid input with the test module loaded. While this doesn't seem
like a critical issue for a test module, the commit message also
mentions that this could be a template for extension developers, so I
think it is worth fixing.

+static bool
+test_ext_node_expect_token(ReadNodeContext *ctx, const char *expected)
+{
+	int			length;
+	const char *token = pg_strtok(ctx, &length);
+
+	return token != NULL && length == (int) strlen(expected) &&
+		memcmp(token, expected, length) == 0;

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? If "expected" is NULL, fall back to the default of
failing if the end has been reached and a NULL token is found, for the
read callback.

+   return token != NULL && length == (int) strlen(expected) &&
+       memcmp(token, expected, length) == 0;

And perhaps also split these in multiple lines, just because it would
feel slightly cleaner.. I know I'm picky on these things.

I am not convinced that we need to check all the input function calls
in 0001 and 0002. For 0001, I'd just pick up one for simplicity of
parsing the test. I can see that you are mostly doing that in 0002,
with test_bms_num_members(). Let's just pick up one, reduce the
duplicates.
--
Michael

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

On Tue, Sep 08, 2026 at 12:16:36PM -0400, Greg Burd wrote:

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

FWIW, I'm also finding the addition of readBitmapset() in
test_bitmapset() kind of ugly to rely on. I'd rather keep to zero
the number of calls outside readfuncs.c, as this is historically a
function kept for compatibility with external code, more efficient
than nodeRead(). I was tempted to eliminate it from src/include/ a
couple of weeks ago, until Matthias' arguments convinced me otherwise
so they have been moved to readfuncs.h instead, as there is still some
external code using it.
--
Michael

I addressed all comments in v2

0002 is now a bit longer with not relying on readBitmapset, but I
think it's still okay/readable.

On Wed, Sep 09, 2026 at 10:36:12AM +0100, Zsolt Parragi wrote:

0002 is now a bit longer with not relying on readBitmapset, but I
think it's still okay/readable.

Cleaned up a lot of the overly-noisy comments, documented a bit more
the sanity checks done in the text->Bitmapset function, cleaned up a
couple of tests, and applied on HEAD for both (don't see much a point
in touching v19).
--
Michael