Hi,
This is about finding #19 in the defect report attached to Noah's
review of pg_plan_advice [1]: pgpa_relids() ignores
CustomScan.custom_relids. Robert wrote [2]:
> #19 might be a real bug, but needs validation, and can't be easily
> validated with in-core code.
I validated #19 with a small test-only custom scan provider (cjoin.c,
attached) and a script that uses it (cjoin.sql). The provider uses
set_join_pathlist_hook to replace the inner join of two tables named
cj_a and cj_b with one CustomScan that has scanrelid = 0. It only
builds the plan and cannot execute the join.
On master (852fd5b86e1), with cjoin loaded:
EXPLAIN (COSTS OFF, PLAN_ADVICE)
SELECT * FROM cj_a a JOIN cj_b b ON a.id = b.id
JOIN cj_c c ON c.id = a.id;
ERROR: XX000: plan node has no RTIs: 360
LOCATION: pgpa_build_scan, pgpa_scan.c:215
360 is T_CustomScan. The plan, from EXPLAIN without PLAN_ADVICE, is:
Hash Join
Hash Cond: (a.id = c.id)
-> Custom Scan (CustomJoin)
-> Hash
-> Seq Scan on cj_c c
When the custom scan is the topmost node, there is no error, but no
advice is generated for the two tables.
The same query without EXPLAIN also fails when
pg_plan_advice.always_store_advice_details is on, or when
pg_plan_advice.feedback_warnings is on and any advice is supplied.
The attached patch makes pgpa_relids() return custom_relids, in the
same way as fs_relids for a ForeignScan. With the patch, the query
above gives:
Generated Plan Advice:
JOIN_ORDER({a b} c)
HASH_JOIN(c)
SEQ_SCAN(c)
NO_GATHER(a b c)
This is the form that the README describes for a custom scan that
scans two tables.
With the patch, the pg_plan_advice, test_plan_advice and
test_extensible suites pass. The patch also applies to REL_19_STABLE,
but I tested it only on master.
The patch has no regression test, because a test needs a custom scan
provider that replaces a join. src/test/modules/test_extensible
(0e944fe3579) has a provider now, but only for a scan of one table.
Would it be acceptable to add a join mode to it for such a test? I
can write that.
Best regards,
Dongpo Liu
