Re: [v9.5] Custom Plan API

On Sat, Nov 8, 2014 at 4:16 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Oct 27, 2014 at 2:35 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>
wrote:
> >> FYI, patch v12 part 2 no longer applies cleanly.
> >>
> > Thanks. I rebased the patch set according to the latest master branch.
> > The attached v13 can be applied to the master.
>
> I've committed parts 1 and 2 of this, without the documentation, and
> with some additional cleanup.

Few observations/questions related to this commit:

1.
@@ -5546,6 +5568,29 @@ get_variable(Var *var, int levelsup, bool
istoplevel, deparse_context *context)
colinfo = deparse_columns_fetch(var->varno, dpns);
attnum = var->varattno;
}
+ else if (IS_SPECIAL_VARNO(var->varno) &&
+ IsA(dpns->planstate, CustomScanState) &&
+ (expr = GetSpecialCustomVar((CustomScanState *) dpns->planstate,
+ var, &child_ps)) != NULL)
+ {
+ deparse_namespace save_dpns;
+
+ if (child_ps)
+ push_child_plan(dpns, child_ps, &save_dpns);
+ /*
+ * Force parentheses because our caller probably assumed a Var is a
+ * simple expression.
+ */
+ if (!IsA(expr, Var))
+ appendStringInfoChar(buf, '(');
+ get_rule_expr((Node *) expr, context, true);
+ if (!IsA(expr, Var))
+ appendStringInfoChar(buf, ')');
+
+ if (child_ps)
+ pop_child_plan(dpns, &save_dpns);
+ return NULL;
+ }

a. It seems Assert for netlelvelsup is missing in this loop.
b. Below comment in function get_variable can be improved
w.r.t handling for CustomScanState. The comment indicates
that if varno is OUTER_VAR or INNER_VAR or INDEX_VAR, it handles
all of them similarly which seems to be slightly changed for
CustomScanState.

/*
* Try to find the relevant RTE in this rtable. In a plan tree, it's
* likely that varno is
OUTER_VAR or INNER_VAR, in which case we must dig
* down into the subplans, or INDEX_VAR, which is
resolved similarly. Also
* find the aliases previously assigned for this RTE.
*/

2.
+void
+register_custom_path_provider(CustomPathMethods *cpp_methods)
{
..
}

Shouldn't there be unregister function corresponding to above
register function?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com