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

Hello Aleksander,

On 25.08.26 15:51, Aleksander Alekseev wrote:
> Many thanks for all the great feedback! I applied all refactorings

Thanks for the updated (v4) version of the patch. It applies cleanly to
master (2f4df67), passes check-world, and shows 94.29% coverage for
extensible.c on my system.

My comments for v3 regarding the missing .gitignore, STRICT, and
parallel_safe are fixed.

I think I found a small issue when table inheritance is used.
test_set_rel_pathlist() does not check rte->inh. For example, in the
following, no scan is performed on the child table (and a wrong result
is produced).

CREATE TABLE test_extensible_tbl(id int, val text);
CREATE TABLE tet_kid() INHERITS (test_extensible_tbl);
INSERT INTO test_extensible_tbl VALUES (1,'parent_row');
INSERT INTO tet_kid VALUES (2,'child_row');

jan=# EXPLAIN (COSTS OFF) SELECT * FROM test_extensible_tbl;
QUERY PLAN
-----------------------------------------------------
Custom Scan (TestCustomScan) on test_extensible_tbl
(1 row)

jan=# SELECT * FROM test_extensible_tbl;
id | val
----+------------
1 | parent_row
1 | parent_row
(2 rows)

Changing:

if (rte->tablesample != NULL)
return;

to:

if (rte->tablesample != NULL || rte->inh)
return;

in test_set_rel_pathlist() should fix the problem.

> except for the following.
>
>> - 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.
>
> Perhaps I'm not fully understand the proposal but it doesn't seem to
> work. Particularly after `git apply test.txt` I get:
>
> ```
> TRAP: failed Assert("numoptions <= num_relopt_elems"), File:
> "../src/backend/access/common/reloptions.c", Line: 2246, PID: 2077291

I can reproduce the issue when test.txt is applied. Also, on my system I
see the assertion being hit: Assert(numoptions <= num_relopt_elems).

The assert fires because parseRelOptions() counts every option
registered for the kind, while heap_reloptions() still passes a fixed
parse table, so numoptions exceeds num_relopt_elems, and I did not find
an easy way to fix that.

Apart from this, the patch looks good to me.

Best regards
Jan

--
Jan Nidzwetzki
PlanetScale Postgres Core Team