On Mon, Nov 10, 2014 at 4:18 PM, Kouhei Kaigai <
kaigai@ak.jp.nec.com> wrote:
> >
> > 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.
>
Similar handling is required in function get_name_for_var_field().
Another point which I wanted to clarify is that in function
get_name_for_var_field(), for all other cases except the new
case added for CustomScanState, it calls get_name_for_var_field()
recursively to get the name of field whereas for CustomScanState,
it calls get_rule_expr() which doesn't look to be problematic in general,
but still it is better to get the name as other cases does unless there
is a special need for CustomScanState?
>
> > 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 thought that in general if user has the API to register the custom path
methods, it should have some way to unregister them and also user might
need to register some different custom path methods after unregistering
the previous one's. I think we should see what Robert or others have to
say about this point before trying to provide such an API.