Re: [PATCH] Add tests for src/backend/nodes/extensible.c

On Tue, Aug 18, 2026 at 06:03:01PM +0200, Jan Nidzwetzki wrote:
> Thanks for working on this. I have applied the v3 version of the patch
> on my system (gcc 14, --buildtype debug, cassert on) and checked the
> coverage. In my test, the test coverage for
> src/backend/nodes/extensible.c increased from 0% to 94.29%.

FWIW, I have wanted an example of module for custom nodes for ages.
We should have added that years ago for coverage as much as to provide
a simple example of what can be done. Count me in.

> - test_get_extensible_node_methods() and test_get_custom_scan_methods()
> are not marked as STRICT and passing a NULL argument crashes the backend
> (both call PG_GETARG_TEXT_PP(0) unconditionally). In contrast,
> test_ext_node_callbacks() is already STRICT, so this looks like an
> oversight.

Yep.

> I did a few experiments, and with the following modifications, I was
> able to create such a plan:
>
> Afterward, I could adjust test_set_rel_pathlist() and set:
>
> -+-
> cpath->path.parallel_safe = true;
> -+-

Manipulations of the plans with GUCs would be nice as part of the
regression tests. We don't have to show all the possible patterns,
but to demonstrate some of the capabilities of this facility,
parallelism is a good one to see a custom node pushed down in a plan
tree.

+ tnode->relid = rte->relid;
+ tnode->repeat_count = 2; /* each row will be returned twice */

Hmm. Let's make that parameterized to offer more variation. Having a
counter to trick the scan and have more tuples returns is a nice
concept for such a test module. Nice approach. But we could make
things slightly spicier:
- GUC to force a new per-query repeat_count.
- Much better for me: add_int_reloption() to force a custom scan to
return a number of rows you want with a CREATE TABLE .. WITH
(repeat_count = N). That's doable once the library is loaded, and
that should be a few extra lines of code.
So let's add the reloption to control the scans rather than hardcoding
it.

I am not much a fan of how test_ext_node_callbacks() is shaped; it
lacks extensibility. I'd like to think that we should be able to
define multiple TestExtNode with different repeat_count and then
compare them. It would be nice for the pluggability of the tests to
be able to pass around TestExtNode pointers (have an extra SQL
function to initialize a TestExtNode based on a repeat count), and
then have a set of SQL functions that act as thin wrappers on top of
the operations you want to do. You can see for example
test_bitmapset() for one idea. In short, I don't think that tests
like the incrementation of a repeat_count before a node compare offers
much value in itself. My idea would be to add one "init" function
that returns a TestExtNode pointer with a repeat_count in input,
passed around a few other functions to do more granular individual
operations (node print, node comparison, etc.).

Note: I am noting some missing trailing newlines, one in the 1.0.sql
script at least.
--
Michael