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.
>
Indeed, this if-block does not have assertion unlike other special-varno.

> 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.
> */
>
I made a small comment that introduces only extension knows the mapping
between these special varno and underlying expression, thus, it queries
providers the expression being tied with this special varnode.
Does it make sense?

> 2.
> +void
> +register_custom_path_provider(CustomPathMethods *cpp_methods)
> {
> ..
> }
>
> Shouldn't there be unregister function corresponding to above register
> function?
>
Even though it is not difficult to implement, what situation will make
sense to unregister rather than enable_xxxx_scan GUC parameter added by
extension itself?
I initially thought prepared statement with custom-scan node is problematic
if provider got unregistered / unloaded, however, internal_unload_library()
actually does nothing. So, it is at least harmless even if we implemented.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>