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.
[1] https://postgr.es/m/[email protected]
[2]
https://postgr.es/m/ca+tgmoymxy-jip5qdhqneiyfebzqsaro6o2d9e8sznzqi1b...@mail.gmail.com
Best regards,
Dongpo Liu
v1-0001-pg_plan_advice-Handle-custom-scans-that-replace-a.patch
Description: Binary data
/*-------------------------------------------------------------------------
*
* cjoin.c
* Test-only custom scan provider that replaces a join.
*
* No in-core code produces a CustomScan that stands for a whole join, so
* this module does: it replaces the inner join of two tables named cj_a and
* cj_b with one CustomScan node.
*
* Only the planner's side is implemented, because only the shape of the plan
* matters here. The node keeps no join clauses and cannot be executed: it
* raises an error if the executor asks it for a row. Use EXPLAIN, or a
* query that is planned but never runs the node.
*
* To build it with PGXS, put this file next to a Makefile that contains
*
* MODULES = cjoin
* PG_CONFIG = pg_config
* PGXS := $(shell $(PG_CONFIG) --pgxs)
* include $(PGXS)
*
* run "make", and then LOAD the resulting library by its full path.
*
*-------------------------------------------------------------------------
*/
#include "postgres.h"
#include "executor/executor.h"
#include "fmgr.h"
#include "nodes/extensible.h"
#include "optimizer/pathnode.h"
#include "optimizer/paths.h"
#include "parser/parsetree.h"
#include "utils/lsyscache.h"
PG_MODULE_MAGIC;
#define CJ_NAME "CustomJoin"
/* ExecInitCustomScan() always calls this, but there is nothing to set up. */
static void
cj_begin(CustomScanState *node, EState *estate, int eflags)
{
}
/*
* Running the join would need real executor code. Refuse, so that nobody
* gets a wrong result from a node that has none.
*/
static TupleTableSlot *
cj_exec(CustomScanState *node)
{
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cjoin cannot execute a join"),
errhint("Use EXPLAIN to look at the plan.")));
return NULL; /* keep compiler quiet */
}
/* EndCustomScan and ReScanCustomScan: there is no state to free or reset. */
static void
cj_noop(CustomScanState *node)
{
}
static const CustomExecMethods cj_exec_methods =
{
.CustomName = CJ_NAME,
.BeginCustomScan = cj_begin,
.ExecCustomScan = cj_exec,
.EndCustomScan = cj_noop,
.ReScanCustomScan = cj_noop,
};
static Node *
cj_create_state(CustomScan *cscan)
{
CustomScanState *css = makeNode(CustomScanState);
css->methods = &cj_exec_methods;
/*
* ExecInitCustomScan() reads slotOps before it makes the scan slot. A
* join has no single table from which to take the slot type, so use
* virtual tuples.
*/
css->slotOps = &TTSOpsVirtual;
return (Node *) css;
}
static const CustomScanMethods cj_scan_methods =
{
.CustomName = CJ_NAME,
.CreateCustomScanState = cj_create_state,
};
static Plan *
cj_plan(PlannerInfo *root, RelOptInfo *rel, struct CustomPath *best_path,
List *tlist, List *clauses, List *custom_plans)
{
CustomScan *cscan = makeNode(CustomScan);
/*
* scanrelid = 0 is how a CustomScan says that it stands for a join and
* not for one base relation. setrefs.c and the executor then resolve
* Vars through custom_scan_tlist, because there is no scan relation to
* resolve them against, so that list must describe the join's output.
*
* create_customscan_plan() sets custom_relids to the relids of the join
* after this function returns.
*/
cscan->scan.scanrelid = 0;
cscan->scan.plan.targetlist = tlist;
cscan->custom_scan_tlist = copyObject(tlist);
cscan->methods = &cj_scan_methods;
return (Plan *) cscan;
}
static const CustomPathMethods cj_path_methods =
{
.CustomName = CJ_NAME,
.PlanCustomPath = cj_plan,
};
static bool
cj_is_table(PlannerInfo *root, RelOptInfo *rel, const char *name)
{
char *relname;
if (rel->reloptkind != RELOPT_BASEREL || rel->rtekind != RTE_RELATION)
return false;
relname = get_rel_name(planner_rt_fetch(rel->relid, root)->relid);
return relname != NULL && strcmp(relname, name) == 0;
}
static set_join_pathlist_hook_type prev_join_pathlist_hook = NULL;
static void
cj_join_pathlist(PlannerInfo *root, RelOptInfo *joinrel,
RelOptInfo *outerrel, RelOptInfo *innerrel,
JoinType jointype, JoinPathExtraData *extra)
{
CustomPath *cpath;
if (prev_join_pathlist_hook)
prev_join_pathlist_hook(root, joinrel, outerrel, innerrel,
jointype,
extra);
/*
* The planner calls this hook for both orders of an inner join, so it
is
* enough to act on one of them. A join that has lateral references
would
* need a parameterized path, which this module does not build.
*/
if (jointype != JOIN_INNER ||
!cj_is_table(root, outerrel, "cj_a") ||
!cj_is_table(root, innerrel, "cj_b") ||
joinrel->lateral_relids != NULL)
return;
cpath = makeNode(CustomPath);
cpath->path.pathtype = T_CustomScan;
cpath->path.parent = joinrel;
cpath->path.pathtarget = joinrel->reltarget;
cpath->path.rows = joinrel->rows;
/*
* A cost of zero makes the planner choose this path every time,
whatever
* the table sizes are. A real provider must give an honest estimate.
*/
cpath->path.startup_cost = 0;
cpath->path.total_cost = 0;
cpath->methods = &cj_path_methods;
add_path(joinrel, (Path *) cpath);
}
void
_PG_init(void)
{
RegisterCustomScanMethods(&cj_scan_methods);
prev_join_pathlist_hook = set_join_pathlist_hook;
set_join_pathlist_hook = cj_join_pathlist;
}
cjoin.sql
Description: application/sql
