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>

On Mon, Nov 10, 2014 at 4:18 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)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.

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

On Mon, Nov 10, 2014 at 6:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.

I wouldn't bother. As KaiGai says, if you want to shut the
functionality off, the provider itself can provide a GUC. Also, we
really have made no effort to ensure that loadable modules can be
safely unloaded, or hooked functions safely-unhooked.
ExecutorRun_hook is a good example. Typical of hook installation is
this:

prev_ExecutorRun = ExecutorRun_hook;
ExecutorRun_hook = pgss_ExecutorRun;

Well, if multiple extensions use this hook, then there's no hope of
unloading them exception in reverse order of installation. We
essentially end up creating a singly-linked list of hook users, but
with the next-pointers stored in arbitrarily-named, likely-static
variables owned by the individual extensions, so that nobody can
actually traverse it. This might be worth fixing as part of a
concerted campaign to make UNLOAD work, but unless somebody's really
going to do that I see little reason to hold this to a higher standard
than we apply elsewhere. The ability to remove extensions from this
hook won't be valuable by itself.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 10, 2014 at 6:33 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Nov 10, 2014 at 6:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > 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.
>
> I wouldn't bother. As KaiGai says, if you want to shut the
> functionality off, the provider itself can provide a GUC. Also, we
> really have made no effort to ensure that loadable modules can be
> safely unloaded, or hooked functions safely-unhooked.
> ExecutorRun_hook is a good example. Typical of hook installation is
> this:
>
> prev_ExecutorRun = ExecutorRun_hook;
> ExecutorRun_hook = pgss_ExecutorRun;
>

In this case, Extension takes care of register and unregister for
hook. In _PG_init(), it register the hook and _PG_fini() it
unregisters the same. So if for custom scan core pg is
providing API to register the methods, shouldn't it provide an
API to unregister the same?

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

On Tue, Nov 11, 2014 at 12:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Nov 10, 2014 at 6:33 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Nov 10, 2014 at 6:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> > 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.
>>
>> I wouldn't bother. As KaiGai says, if you want to shut the
>> functionality off, the provider itself can provide a GUC. Also, we
>> really have made no effort to ensure that loadable modules can be
>> safely unloaded, or hooked functions safely-unhooked.
>> ExecutorRun_hook is a good example. Typical of hook installation is
>> this:
>>
>> prev_ExecutorRun = ExecutorRun_hook;
>> ExecutorRun_hook = pgss_ExecutorRun;
>>
>
> In this case, Extension takes care of register and unregister for
> hook. In _PG_init(), it register the hook and _PG_fini() it
> unregisters the same.

The point is that there's nothing that you can do _PG_fini() that will
work correctly. If it does ExecutorRun_hook = prev_ExecutorRun, that's
fine if it's the most-recently-installed hook. But if it isn't, then
doing so corrupts the list.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company