Re: [v9.5] Custom Plan API

> On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> > Do we need to match the prototype of wrapper function with callback?
>
> Yes.
>
OK, I fixed up the patch part-2, to fit declaration of GetSpecialCustomVar()
with corresponding callback.

Also, a noise in the part-3 patch, by git-pull, was removed.

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

On 30 September 2014 07:27, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:

> > On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
> wrote:
> > > Do we need to match the prototype of wrapper function with callback?
> >
> > Yes.
> >
> OK, I fixed up the patch part-2, to fit declaration of
> GetSpecialCustomVar()
> with corresponding callback.
>
> Also, a noise in the part-3 patch, by git-pull, was removed.

FYI, patch v12 part 2 no longer applies cleanly.

--
Thom

> 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.
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: thombrown(at)gmail(dot)com [mailto:thombrown(at)gmail(dot)com] On Behalf Of Thom
> Brown
> Sent: Sunday, October 26, 2014 9:22 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; Kohei KaiGai; Tom Lane; Alvaro Herrera; Shigeru Hanada;
> Simon Riggs; Stephen Frost; Andres Freund; PgHacker; Jim Mlodgenski; Peter
> Eisentraut
> Subject: Re: [HACKERS] [v9.5] Custom Plan API
>
> On 30 September 2014 07:27, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
>
> > On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai
> <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> > > Do we need to match the prototype of wrapper function with
> callback?
> >
> > Yes.
> >
> OK, I fixed up the patch part-2, to fit declaration of
> GetSpecialCustomVar()
> with corresponding callback.
>
> Also, a noise in the part-3 patch, by git-pull, was removed.
>
>
> FYI, patch v12 part 2 no longer applies cleanly.
>
> --
> Thom

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. I am not sure that this feature is
sufficiently non-experimental that it deserves to be documented, but
if we're thinking of doing that then the documentation needs a lot
more work. I think part 3 of the patch is mostly useful as a
demonstration of how this API can be used, and is not something we
probably want to commit. So I'm not planning, at this point, to spend
any more time on this patch series, and will mark it Committed in the
CF app.

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

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I've committed parts 1 and 2 of this, without the documentation, and
> with some additional cleanup. I am not sure that this feature is
> sufficiently non-experimental that it deserves to be documented, but
> if we're thinking of doing that then the documentation needs a lot
> more work. I think part 3 of the patch is mostly useful as a
> demonstration of how this API can be used, and is not something we
> probably want to commit. So I'm not planning, at this point, to spend
> any more time on this patch series, and will mark it Committed in the
> CF app.

I've done some preliminary cleanup on this patch, but I'm still pretty
desperately unhappy about some aspects of it, in particular the way that
it gets custom scan providers directly involved in setrefs.c,
finalize_primnode, and replace_nestloop_params processing. I don't
want any of that stuff exported outside the core, as freezing those
APIs would be a very nasty restriction on future planner development.
What's more, it doesn't seem like doing that creates any value for
custom-scan providers, only a requirement for extra boilerplate code
for them to provide.

ISTM that we could avoid that by borrowing the design used for FDW
plans, namely that any expressions you would like planner post-processing
services for should be stuck into a predefined List field (fdw_exprs
for the ForeignScan case, perhaps custom_exprs for the CustomScan case?).
This would let us get rid of the SetCustomScanRef and FinalizeCustomScan
callbacks as well as simplify the API contract for PlanCustomPath.

I'm also wondering why this patch didn't follow the FDW lead in terms of
expecting private data to be linked from specialized "private" fields.
The design as it stands (with an expectation that CustomPaths, CustomPlans
etc would be larger than the core code knows about) is not awful, but it
seems just randomly different from the FDW precedent, and I don't see a
good argument why it should be. If we undid that we could get rid of
CopyCustomScan callbacks, and perhaps also TextOutCustomPath and
TextOutCustomScan (though I concede there might be some argument to keep
the latter two anyway for debugging reasons).

Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism
added to ruleutils.c is anything but dead weight that we'll have to
maintain forever. It seems somewhat unlikely that anyone will figure
out how to use it, or indeed that it can be used for anything very
interesting. I suppose the argument for it is that you could stick
"custom vars" into the tlist of a CustomScan plan node, but you cannot,
at least not without a bunch of infrastructure that isn't there now;
in particular how would such an expression ever get matched by setrefs.c
to higher-level plan tlists? I think we should rip that out and wait
to see a complete use-case before considering putting it back.

Comments?

regards, tom lane

PS: with no documentation it's arguable that the entire patch is just
dead weight. I'm not very happy that it went in without any.

On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I've done some preliminary cleanup on this patch, but I'm still pretty
> desperately unhappy about some aspects of it, in particular the way that
> it gets custom scan providers directly involved in setrefs.c,
> finalize_primnode, and replace_nestloop_params processing. I don't
> want any of that stuff exported outside the core, as freezing those
> APIs would be a very nasty restriction on future planner development.
> What's more, it doesn't seem like doing that creates any value for
> custom-scan providers, only a requirement for extra boilerplate code
> for them to provide.
>
> ISTM that we could avoid that by borrowing the design used for FDW
> plans, namely that any expressions you would like planner post-processing
> services for should be stuck into a predefined List field (fdw_exprs
> for the ForeignScan case, perhaps custom_exprs for the CustomScan case?).
> This would let us get rid of the SetCustomScanRef and FinalizeCustomScan
> callbacks as well as simplify the API contract for PlanCustomPath.

Ah, that makes sense. I'm not sure I really understand what's so bad
about the current system, but I have no issue with revising it for
consistency.

> I'm also wondering why this patch didn't follow the FDW lead in terms of
> expecting private data to be linked from specialized "private" fields.
> The design as it stands (with an expectation that CustomPaths, CustomPlans
> etc would be larger than the core code knows about) is not awful, but it
> seems just randomly different from the FDW precedent, and I don't see a
> good argument why it should be. If we undid that we could get rid of
> CopyCustomScan callbacks, and perhaps also TextOutCustomPath and
> TextOutCustomScan (though I concede there might be some argument to keep
> the latter two anyway for debugging reasons).

OK.

> Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism
> added to ruleutils.c is anything but dead weight that we'll have to
> maintain forever. It seems somewhat unlikely that anyone will figure
> out how to use it, or indeed that it can be used for anything very
> interesting. I suppose the argument for it is that you could stick
> "custom vars" into the tlist of a CustomScan plan node, but you cannot,
> at least not without a bunch of infrastructure that isn't there now;
> in particular how would such an expression ever get matched by setrefs.c
> to higher-level plan tlists? I think we should rip that out and wait
> to see a complete use-case before considering putting it back.

I thought this was driven by a suggestion from you, but maybe KaiGai
can comment.

> PS: with no documentation it's arguable that the entire patch is just
> dead weight. I'm not very happy that it went in without any.

As I said, I wasn't sure we wanted to commit to the API enough to
document it, and by the time you get done whacking the stuff above
around, the documentation KaiGai wrote for it (which was also badly in
need of editing by a native English speaker) would have been mostly
obsolete anyway. But I'm willing to put some effort into it once you
get done rearranging the furniture, if that's helpful.

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

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > I've committed parts 1 and 2 of this, without the documentation, and
> > with some additional cleanup. I am not sure that this feature is
> > sufficiently non-experimental that it deserves to be documented, but
> > if we're thinking of doing that then the documentation needs a lot
> > more work. I think part 3 of the patch is mostly useful as a
> > demonstration of how this API can be used, and is not something we
> > probably want to commit. So I'm not planning, at this point, to spend
> > any more time on this patch series, and will mark it Committed in the
> > CF app.
>
> I've done some preliminary cleanup on this patch, but I'm still pretty
> desperately unhappy about some aspects of it, in particular the way that
> it gets custom scan providers directly involved in setrefs.c,
> finalize_primnode, and replace_nestloop_params processing. I don't want
> any of that stuff exported outside the core, as freezing those APIs would
> be a very nasty restriction on future planner development.
> What's more, it doesn't seem like doing that creates any value for
> custom-scan providers, only a requirement for extra boilerplate code for
> them to provide.
>
> ISTM that we could avoid that by borrowing the design used for FDW plans,
> namely that any expressions you would like planner post-processing services
> for should be stuck into a predefined List field (fdw_exprs for the
> ForeignScan case, perhaps custom_exprs for the CustomScan case?).
> This would let us get rid of the SetCustomScanRef and FinalizeCustomScan
> callbacks as well as simplify the API contract for PlanCustomPath.
>
If core backend can know which field contains expression nodes but
processed by custom-scan provider, FinalizedCustomScan might be able
to rid. However, rid of SetCustomScanRef makes unavailable a significant
use case I intend.
In case when tlist contains complicated expression node (thus it takes
many cpu cycles) and custom-scan provider has a capability to compute
this expression node externally, SetCustomScanRef hook allows to replace
this complicate expression node by a simple Var node that references
a value being externally computed.

Because only custom-scan provider can know how this "pseudo" varnode
is mapped to the original expression, it needs to call the hook to
assign correct varno/varattno. We expect it assigns a special vano,
like OUTER_VAR, and it is solved with GetSpecialCustomVar.

One other idea is, core backend has a facility to translate relationship
between the original expression and the pseudo varnode according to the
map information given by custom-scan provider.

> I'm also wondering why this patch didn't follow the FDW lead in terms of
> expecting private data to be linked from specialized "private" fields.
> The design as it stands (with an expectation that CustomPaths, CustomPlans
> etc would be larger than the core code knows about) is not awful, but it
> seems just randomly different from the FDW precedent, and I don't see a
> good argument why it should be. If we undid that we could get rid of
> CopyCustomScan callbacks, and perhaps also TextOutCustomPath and
> TextOutCustomScan (though I concede there might be some argument to keep
> the latter two anyway for debugging reasons).
>
Yep, its original proposition last year followed the FDW manner. It has
custom_private field to store the private data of custom-scan provider,
however, I was suggested to change the current form because it added
a couple of routines to encode / decode Bitmapset that may lead other
encode / decode routines for other data types.

I'm neutral for this design choice. Either of them people accept is
better for me.

> Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism added
> to ruleutils.c is anything but dead weight that we'll have to maintain
> forever. It seems somewhat unlikely that anyone will figure out how to
> use it, or indeed that it can be used for anything very interesting. I
> suppose the argument for it is that you could stick "custom vars" into the
> tlist of a CustomScan plan node, but you cannot, at least not without a
> bunch of infrastructure that isn't there now; in particular how would such
> an expression ever get matched by setrefs.c to higher-level plan tlists?
> I think we should rip that out and wait to see a complete use-case before
> considering putting it back.
>
As I described above, as long as core backend has a facility to manage the
relationship between a pseudo varnode and complicated expression node, I
think we can rid this callback.

> PS: with no documentation it's arguable that the entire patch is just dead
> weight. I'm not very happy that it went in without any.
>
I agree with this. Is it a good to write up a wikipage to brush up the
documentation draft?

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

Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> I've done some preliminary cleanup on this patch, but I'm still pretty
>> desperately unhappy about some aspects of it, in particular the way that
>> it gets custom scan providers directly involved in setrefs.c,
>> finalize_primnode, and replace_nestloop_params processing. I don't want
>> any of that stuff exported outside the core, as freezing those APIs would
>> be a very nasty restriction on future planner development.

> If core backend can know which field contains expression nodes but
> processed by custom-scan provider, FinalizedCustomScan might be able
> to rid. However, rid of SetCustomScanRef makes unavailable a significant
> use case I intend.
> In case when tlist contains complicated expression node (thus it takes
> many cpu cycles) and custom-scan provider has a capability to compute
> this expression node externally, SetCustomScanRef hook allows to replace
> this complicate expression node by a simple Var node that references
> a value being externally computed.

That's a fine goal to have, but this is not a solution that works for
any except trivial cases. The problem is that that complicated expression
isn't going to be in the CustomScan's tlist in the first place unless you
have a one-node plan. As soon as you have a join, for example, the
planner is going to delay calculation of anything more complex than a
plain Var to above the join. Aggregation, GROUP BY, etc would also defeat
such an optimization.

This gets back to the remarks I made earlier about it not being possible
to do anything very interesting in a plugin of this nature. You really
need cooperation from other places in the planner if you want to do things
like pushing down calculations into an external provider. And it's not
at all clear how that would even work, let alone how we might make it
implementable as a plugin rather than core code.

Also, even if we could think of a way to do this from a CustomScan plugin,
that would fail to cover some very significant use-cases for pushing
down complex expressions, for example:
* retrieving values of expensive functions from expression indexes;
* pushing down expensive functions into FDWs so they can be done remotely.
And I'm also worried that once we've exported and thereby frozen the APIs
in this area, we'd be operating with one hand tied behind our backs in
solving those use-cases. So I'm not very excited about pursuing the
problem in this form.

So I remain of the opinion that we should get the CustomScan stuff out
of setrefs processing, and also that having EXPLAIN support for such
special variables is premature. It's possible that after the dust
settles we'd wind up with additions to ruleutils that look exactly like
what's in this patch ... but I'd bet against that.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> As I said, I wasn't sure we wanted to commit to the API enough to
> document it, and by the time you get done whacking the stuff above
> around, the documentation KaiGai wrote for it (which was also badly in
> need of editing by a native English speaker) would have been mostly
> obsolete anyway. But I'm willing to put some effort into it once you
> get done rearranging the furniture, if that's helpful.

I thought of another API change we should consider. It's weird that
CustomPathMethods includes CreateCustomScanPath, because that's not
a method you apply to a CustomPath, it's what creates them in the first
place. I'm inclined to think that we should get rid of that and
register_custom_path_provider() altogether and just provide a function
hook variable equivalent to create_customscan_paths, which providers can
link into in the usual way. The register_custom_path_provider mechanism
might have some use if we were also going to provide deregister-by-name
functionality, but as you pointed out upthread, that's not likely to ever
be worth doing.

The hook function might better be named something like
editorialize_on_relation_paths, since in principle it could screw around
with the Paths already made by the core code, not just add CustomPaths.
There's an analogy to get_relation_info_hook, which is meant to let
plugins editorialize on the relation's index list. So maybe
set_plain_rel_pathlist_hook?

regards, tom lane

> Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> >> I've done some preliminary cleanup on this patch, but I'm still
> >> pretty desperately unhappy about some aspects of it, in particular
> >> the way that it gets custom scan providers directly involved in
> >> setrefs.c, finalize_primnode, and replace_nestloop_params processing.
> >> I don't want any of that stuff exported outside the core, as freezing
> >> those APIs would be a very nasty restriction on future planner
> development.
>
> > If core backend can know which field contains expression nodes but
> > processed by custom-scan provider, FinalizedCustomScan might be able
> > to rid. However, rid of SetCustomScanRef makes unavailable a
> > significant use case I intend.
> > In case when tlist contains complicated expression node (thus it takes
> > many cpu cycles) and custom-scan provider has a capability to compute
> > this expression node externally, SetCustomScanRef hook allows to
> > replace this complicate expression node by a simple Var node that
> > references a value being externally computed.
>
> That's a fine goal to have, but this is not a solution that works for any
> except trivial cases. The problem is that that complicated expression
> isn't going to be in the CustomScan's tlist in the first place unless you
> have a one-node plan. As soon as you have a join, for example, the planner
> is going to delay calculation of anything more complex than a plain Var
> to above the join. Aggregation, GROUP BY, etc would also defeat such an
> optimization.
>
> This gets back to the remarks I made earlier about it not being possible
> to do anything very interesting in a plugin of this nature. You really
> need cooperation from other places in the planner if you want to do things
> like pushing down calculations into an external provider. And it's not
> at all clear how that would even work, let alone how we might make it
> implementable as a plugin rather than core code.
>
> Also, even if we could think of a way to do this from a CustomScan plugin,
> that would fail to cover some very significant use-cases for pushing down
> complex expressions, for example:
> * retrieving values of expensive functions from expression indexes;
> * pushing down expensive functions into FDWs so they can be done remotely.
> And I'm also worried that once we've exported and thereby frozen the APIs
> in this area, we'd be operating with one hand tied behind our backs in solving
> those use-cases. So I'm not very excited about pursuing the problem in
> this form.
>
I count understand your concern; only available on a one-node plan and
may needs additional interaction between core and extension to push-
down complicated expression.
So, right now, I have to admit to rid of this hook for this purpose.

On the other hand, I thought to use similar functionality, but not
same, to implement join-replacement by custom-scan. I'd like to see
your comment prior to patch submission.

Let assume a custom-scan provider that runs on a materialized-view
(or, something like a query cache in memory) instead of join.
In this case, a reasonable design is to fetch a tuple from the
materialized-view then put it on the ecxt_scantuple of ExprContext
prior to evaluation of qualifier or tlist, unlike usual join takes
two slots - ecxt_innertuple and ecxt_outertuple.
Also, it leads individual varnode has to reference exct_scantuple,
neither ecxt_innertuple nor ecxt_outertuple.
The tuple in exct_scantuple contains attributes come from both
relations, thus, it needs to keep relationship a varattno of the
scanned tuple and its source relation where does it come from.

I intended to use the SetCustomScanRef callback to adjust varno
and varattno of the varnode that references the custom-scan node;
as if set_join_references() doing.
It does not mean a replacement of general expression by varnode,
just re-mapping of varno/varattno.

> So I remain of the opinion that we should get the CustomScan stuff out of
> setrefs processing, and also that having EXPLAIN support for such special
> variables is premature. It's possible that after the dust settles we'd
> wind up with additions to ruleutils that look exactly like what's in this
> patch ... but I'd bet against that.
>
So, I can agree with rid of SetCustomScanRef and GetSpecialCustomVar.
However, some alternative functionality to implement the varno/varattno
remapping is needed soon.
How about your thought?

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

Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> Let assume a custom-scan provider that runs on a materialized-view
> (or, something like a query cache in memory) instead of join.
> In this case, a reasonable design is to fetch a tuple from the
> materialized-view then put it on the ecxt_scantuple of ExprContext
> prior to evaluation of qualifier or tlist, unlike usual join takes
> two slots - ecxt_innertuple and ecxt_outertuple.
> Also, it leads individual varnode has to reference exct_scantuple,
> neither ecxt_innertuple nor ecxt_outertuple.

OK, that's possibly a reasonable way to do it at runtime. You don't
*have* to do it that way of course. It would be only marginally
less efficient to reconstruct two tuples that match the shapes of the
original join inputs.

> I intended to use the SetCustomScanRef callback to adjust varno
> and varattno of the varnode that references the custom-scan node;
> as if set_join_references() doing.

I think this is really fundamentally misguided. setrefs.c has no
business doing anything "interesting" like making semantically important
substitutions; those decisions need to be made much earlier. An example
in the context of your previous proposal is that getting rid of expensive
functions without any adjustment of cost estimates is just wrong; and
I don't mean that you forgot to have your setrefs.c hook hack up the
Plan's cost fields. The cost estimates need to change at the Path stage,
or the planner might not even select the right path at all.

I'm not sure where would be an appropriate place to deal with the kind of
thing you're thinking about here. But I'm really not happy with the
concept of exposing the guts of setrefs.c in order to enable
single-purpose kluges like this. We have fairly general problems to solve
in this area, and we should be working on solving them, not on freezing
relevant planner APIs to support marginally-useful external plugins.

regards, tom lane

> Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > Let assume a custom-scan provider that runs on a materialized-view
> > (or, something like a query cache in memory) instead of join.
> > In this case, a reasonable design is to fetch a tuple from the
> > materialized-view then put it on the ecxt_scantuple of ExprContext
> > prior to evaluation of qualifier or tlist, unlike usual join takes two
> > slots - ecxt_innertuple and ecxt_outertuple.
> > Also, it leads individual varnode has to reference exct_scantuple,
> > neither ecxt_innertuple nor ecxt_outertuple.
>
> OK, that's possibly a reasonable way to do it at runtime. You don't
> *have* to do it that way of course. It would be only marginally less
> efficient to reconstruct two tuples that match the shapes of the original
> join inputs.
>
> > I intended to use the SetCustomScanRef callback to adjust varno and
> > varattno of the varnode that references the custom-scan node; as if
> > set_join_references() doing.
>
> I think this is really fundamentally misguided. setrefs.c has no business
> doing anything "interesting" like making semantically important
> substitutions; those decisions need to be made much earlier. An example
> in the context of your previous proposal is that getting rid of expensive
> functions without any adjustment of cost estimates is just wrong; and I
> don't mean that you forgot to have your setrefs.c hook hack up the Plan's
> cost fields. The cost estimates need to change at the Path stage, or the
> planner might not even select the right path at all.
>
Because we right now have no functionality to register custom-scan path
instead of join, I had to show another use scenario...

> I'm not sure where would be an appropriate place to deal with the kind of
> thing you're thinking about here. But I'm really not happy with the concept
> of exposing the guts of setrefs.c in order to enable single-purpose kluges
> like this. We have fairly general problems to solve in this area, and we
> should be working on solving them, not on freezing relevant planner APIs
> to support marginally-useful external plugins.
>
From my standpoint, varnode remapping on relations join is higher priority
than complicated expression node. As long as the core backend handles this
job, yes, I think a hook in setrefs.c is not mandatory.
Also, it means the role to solve special vernode on EXPLAIN is moved from
extension to the code, GetSpecialCustomVar can be rid.

Let me explain the current idea of mine.
CustomScan node will have a field that hold varnode mapping information
that is constructed by custom-scan provider on create_customscan_plan,
if they want. It is probably a list of varnode.
If exists, setrefs.c changes its behavior; that updates varno/varattno of
varnode according to this mapping, as if set_join_references() does
based on indexed_tlist.
To reference exct_scantuple, INDEX_VAR will be a best choice for varno
of these varnodes, and index of the above varnode mapping list will
be varattno. It can be utilized to make EXPLAIN output, instead of
GetSpecialCustomVar hook.

So, steps to go may be:
(1) Add custom_private, custom_exprs, ... instead of self defined data
type based on CustomXXX.
(2) Rid of SetCustomScanRef and GetSpecialCustomVar hook for the current
custom-"scan" support.
(3) Integration of above varnode mapping feature within upcoming join
replacement by custom-scan support.

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

Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> Let me explain the current idea of mine.
> CustomScan node will have a field that hold varnode mapping information
> that is constructed by custom-scan provider on create_customscan_plan,
> if they want. It is probably a list of varnode.
> If exists, setrefs.c changes its behavior; that updates varno/varattno of
> varnode according to this mapping, as if set_join_references() does
> based on indexed_tlist.
> To reference exct_scantuple, INDEX_VAR will be a best choice for varno
> of these varnodes, and index of the above varnode mapping list will
> be varattno. It can be utilized to make EXPLAIN output, instead of
> GetSpecialCustomVar hook.

> So, steps to go may be:
> (1) Add custom_private, custom_exprs, ... instead of self defined data
> type based on CustomXXX.
> (2) Rid of SetCustomScanRef and GetSpecialCustomVar hook for the current
> custom-"scan" support.
> (3) Integration of above varnode mapping feature within upcoming join
> replacement by custom-scan support.

Well ... I still do not find this interesting, because I don't believe
that CustomScan is a solution to anything interesting. It's difficult
enough to solve problems like expensive-function pushdown within the
core code; why would we tie one hand behind our backs by insisting that
they should be solved by extensions? And as I mentioned before, we do
need solutions to these problems in the core, regardless of CustomScan.

I think that a useful way to go at this might be to think first about
how to make use of expensive functions that have been cached in indexes,
and then see how the solution to that might translate to pushing down
expensive functions into FDWs and CustomScans. If you start with the
CustomScan aspect of it then you immediately find yourself trying to
design APIs to divide up the solution, which is premature when you
don't even know what the solution is.

The rough idea I'd had about this is that while canvassing a relation's
indexes (in get_relation_info), we could create a list of precomputed
expressions that are available from indexes, then run through the
query tree and replace any matching subexpressions with some Var-like
nodes (or maybe better PlaceHolderVar-like nodes) that indicate that
"we can get this expression for free if we read the right index".
If we do read the right index, such an expression reduces to a Var in
the finished plan tree; if not, it reverts to the original expression.
(Some thought would need to be given to the semantics when the index's
table is underneath an outer join --- that may just mean that we can't
necessarily replace every textually-matching subexpression, only those
that are not above an outer join.) One question mark here is how to do
the "replace any matching subexpressions" bit without O(lots) processing
cost in big queries. But that's probably just a SMOP. The bigger issue
I fear is that the planner is not currently structured to think that
evaluation cost of expressions in the SELECT list has anything to do
with which Path it should pick. That is tied to the handwaving I've
been doing for awhile now about converting all the upper-level planning
logic into generate-and-compare-Paths style; we certainly cannot ignore
tlist eval costs while making those decisions. So at least for those
upper-level Paths, we'd have to have a notion of what tlist we expect
that plan level to compute, and charge appropriate evaluation costs.

So there's a lot of work there and I don't find that CustomScan looks
like a solution to any of it. CustomScan and FDWs could benefit from
this work, in that we'd now have a way to deal with the concept that
expensive functions (and aggregates, I hope) might be computed at
the bottom scan level. But it's folly to suppose that we can make it
work just by hacking some arms-length extension code without any
fundamental planner changes.

regards, tom lane

> Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > Let me explain the current idea of mine.
> > CustomScan node will have a field that hold varnode mapping
> > information that is constructed by custom-scan provider on
> > create_customscan_plan, if they want. It is probably a list of varnode.
> > If exists, setrefs.c changes its behavior; that updates varno/varattno
> > of varnode according to this mapping, as if set_join_references() does
> > based on indexed_tlist.
> > To reference exct_scantuple, INDEX_VAR will be a best choice for varno
> > of these varnodes, and index of the above varnode mapping list will be
> > varattno. It can be utilized to make EXPLAIN output, instead of
> > GetSpecialCustomVar hook.
>
> > So, steps to go may be:
> > (1) Add custom_private, custom_exprs, ... instead of self defined data
> > type based on CustomXXX.
> > (2) Rid of SetCustomScanRef and GetSpecialCustomVar hook for the current
> > custom-"scan" support.
> > (3) Integration of above varnode mapping feature within upcoming join
> > replacement by custom-scan support.
>
> Well ... I still do not find this interesting, because I don't believe that
> CustomScan is a solution to anything interesting. It's difficult enough
> to solve problems like expensive-function pushdown within the core code;
> why would we tie one hand behind our backs by insisting that they should
> be solved by extensions? And as I mentioned before, we do need solutions
> to these problems in the core, regardless of CustomScan.
>
I'd like to split the "anything interesting" into two portions.
As you pointed out, the feature to push-down complicated expression
may need a bit large efforts (for remaining two commit-fest at least),
however, what the feature to replace join by custom-scan requires is
similar to job of set_join_references() because it never involves
translation between varnode and general expression.

Also, from my standpoint, a simple join replacement by custom-scan has
higher priority; join acceleration in v9.5 makes sense even if full-
functionality of pushing down general expression is not supported yet.

> I think that a useful way to go at this might be to think first about how
> to make use of expensive functions that have been cached in indexes, and
> then see how the solution to that might translate to pushing down expensive
> functions into FDWs and CustomScans. If you start with the CustomScan
> aspect of it then you immediately find yourself trying to design APIs to
> divide up the solution, which is premature when you don't even know what
> the solution is.
>
Yep, it also seems to me remaining two commit fests are a bit tight
schedule to make consensus of overall design and to implement.
I'd like to focus on the simpler portion first.

> The rough idea I'd had about this is that while canvassing a relation's
> indexes (in get_relation_info), we could create a list of precomputed
> expressions that are available from indexes, then run through the query
> tree and replace any matching subexpressions with some Var-like nodes (or
> maybe better PlaceHolderVar-like nodes) that indicate that "we can get this
> expression for free if we read the right index".
> If we do read the right index, such an expression reduces to a Var in the
> finished plan tree; if not, it reverts to the original expression.
> (Some thought would need to be given to the semantics when the index's table
> is underneath an outer join --- that may just mean that we can't necessarily
> replace every textually-matching subexpression, only those that are not
> above an outer join.) One question mark here is how to do the "replace
> any matching subexpressions" bit without O(lots) processing cost in big
> queries. But that's probably just a SMOP. The bigger issue I fear is that
> the planner is not currently structured to think that evaluation cost of
> expressions in the SELECT list has anything to do with which Path it should
> pick. That is tied to the handwaving I've been doing for awhile now about
> converting all the upper-level planning logic into
> generate-and-compare-Paths style; we certainly cannot ignore tlist eval
> costs while making those decisions. So at least for those upper-level Paths,
> we'd have to have a notion of what tlist we expect that plan level to compute,
> and charge appropriate evaluation costs.
>
Let me investigate the planner code more prior to comment on...

> So there's a lot of work there and I don't find that CustomScan looks like
> a solution to any of it. CustomScan and FDWs could benefit from this work,
> in that we'd now have a way to deal with the concept that expensive functions
> (and aggregates, I hope) might be computed at the bottom scan level. But
> it's folly to suppose that we can make it work just by hacking some
> arms-length extension code without any fundamental planner changes.
>
Indeed, I don't think it is a good idea to start from this harder portion.
Let's focus on just varno/varattno remapping to replace join relation by
custom-scan, as an immediate target.

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

On Mon, Nov 24, 2014 at 6:57 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> Indeed, I don't think it is a good idea to start from this harder portion.
> Let's focus on just varno/varattno remapping to replace join relation by
> custom-scan, as an immediate target.

We still need something like this for FDWs, as well. The potential
gains there are enormous. Anything we do had better fit in nicely
with that, rather than looking like a separate hack.

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

On 7 November 2014 at 22:46, 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. I am not sure that this feature is
> sufficiently non-experimental that it deserves to be documented, but
> if we're thinking of doing that then the documentation needs a lot
> more work. I think part 3 of the patch is mostly useful as a
> demonstration of how this API can be used, and is not something we
> probably want to commit. So I'm not planning, at this point, to spend
> any more time on this patch series, and will mark it Committed in the
> CF app.

I'm very concerned about the state of this feature. No docs, no
examples, and therefore, no testing. This standard of code is much
less than I've been taught is the minimum standard on this project.

There are zero docs, even in README. Experimental feature, or not,
there MUST be documentation somewhere, in some form, even if that is
just on the Wiki. Otherwise how it will ever be used sufficiently to
allow it to be declared fully usable?

The example contrib module was not committed and I am advised no longer works.

After much effort in persuading academic contacts to begin using the
feature for open source research it now appears pretty much unusable.

This is supposed to be an open project. Whoever takes responsibility
here, please ensure that those things are resolved, quickly.

We're on a time limit because any flaws in the API need to be ironed
out before its too late and we have to decide to either remove the API
because its flaky, or commit to supporting it in production for 9.5.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> On 7 November 2014 at 22:46, 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. I am not sure that this feature is
> > sufficiently non-experimental that it deserves to be documented, but
> > if we're thinking of doing that then the documentation needs a lot
> > more work. I think part 3 of the patch is mostly useful as a
> > demonstration of how this API can be used, and is not something we
> > probably want to commit. So I'm not planning, at this point, to spend
> > any more time on this patch series, and will mark it Committed in the
> > CF app.
>
>
> I'm very concerned about the state of this feature. No docs, no examples,
> and therefore, no testing. This standard of code is much less than I've
> been taught is the minimum standard on this project.
>
> There are zero docs, even in README. Experimental feature, or not, there
> MUST be documentation somewhere, in some form, even if that is just on the
> Wiki. Otherwise how it will ever be used sufficiently to allow it to be
> declared fully usable?
>
The reason why documentation portion was not yet committed is, sorry, it
is due to quality of documentation from the standpoint of native English
speaker.
Now, I'm writing up a documentation stuff according to the latest code base,
please wait for several days and help to improve.

> The example contrib module was not committed and I am advised no longer
> works.
>
May I submit the contrib/ctidscan module again for an example?

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

On 27 November 2014 at 10:33, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:

> The reason why documentation portion was not yet committed is, sorry, it
> is due to quality of documentation from the standpoint of native English
> speaker.
> Now, I'm writing up a documentation stuff according to the latest code base,
> please wait for several days and help to improve.

Happy to help with that.

Please post to the Wiki first so we can edit it communally.

>> The example contrib module was not committed and I am advised no longer
>> works.
>>
> May I submit the contrib/ctidscan module again for an example?

Yes please. We have other contrib modules that exist as tests, so this
seems reasonable to me.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> -----Original Message-----
> From: Simon Riggs [mailto:simon(at)2ndQuadrant(dot)com]
> Sent: Thursday, November 27, 2014 8:48 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; Thom Brown; Kohei KaiGai; Tom Lane; Alvaro Herrera; Shigeru
> Hanada; Stephen Frost; Andres Freund; PgHacker; Jim Mlodgenski; Peter
> Eisentraut
> Subject: Re: [HACKERS] [v9.5] Custom Plan API
>
> On 27 November 2014 at 10:33, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
> > The reason why documentation portion was not yet committed is, sorry,
> > it is due to quality of documentation from the standpoint of native
> > English speaker.
> > Now, I'm writing up a documentation stuff according to the latest code
> > base, please wait for several days and help to improve.
>
> Happy to help with that.
>
> Please post to the Wiki first so we can edit it communally.
>
Simon,

I tried to describe how custom-scan provider interact with the core backend,
and expectations to individual callbacks here.

https://wiki.postgresql.org/wiki/CustomScanInterface

I'd like to see which kind of description should be added, from third person's
viewpoint.

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

On 27 November 2014 at 20:48, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 27 November 2014 at 10:33, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
>> The reason why documentation portion was not yet committed is, sorry, it
>> is due to quality of documentation from the standpoint of native English
>> speaker.
>> Now, I'm writing up a documentation stuff according to the latest code base,
>> please wait for several days and help to improve.
>
> Happy to help with that.
>
> Please post to the Wiki first so we can edit it communally.

I've corrected a spelling mistake, but it reads OK at moment.

>>> The example contrib module was not committed and I am advised no longer
>>> works.
>>>
>> May I submit the contrib/ctidscan module again for an example?
>
> Yes please. We have other contrib modules that exist as tests, so this
> seems reasonable to me.

I can't improve the docs without the example code. Is that available now?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon,

> > Yes please. We have other contrib modules that exist as tests, so this
> > seems reasonable to me.
>
> I can't improve the docs without the example code. Is that available now?
>
Please wait for a few days. The ctidscan module is not adjusted for the
latest interface yet.

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

> -----Original Message-----
> From: Simon Riggs [mailto:simon(at)2ndQuadrant(dot)com]
> Sent: Sunday, December 07, 2014 12:37 AM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; Thom Brown; Kohei KaiGai; Tom Lane; Alvaro Herrera; Shigeru
> Hanada; Stephen Frost; Andres Freund; PgHacker; Jim Mlodgenski; Peter
> Eisentraut
> Subject: Re: [HACKERS] [v9.5] Custom Plan API
>
> On 27 November 2014 at 20:48, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On 27 November 2014 at 10:33, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> >
> >> The reason why documentation portion was not yet committed is, sorry,
> >> it is due to quality of documentation from the standpoint of native
> >> English speaker.
> >> Now, I'm writing up a documentation stuff according to the latest
> >> code base, please wait for several days and help to improve.
> >
> > Happy to help with that.
> >
> > Please post to the Wiki first so we can edit it communally.
>
> I've corrected a spelling mistake, but it reads OK at moment.
>
>
> >>> The example contrib module was not committed and I am advised no
> >>> longer works.
> >>>
> >> May I submit the contrib/ctidscan module again for an example?
> >
> > Yes please. We have other contrib modules that exist as tests, so this
> > seems reasonable to me.
>
> I can't improve the docs without the example code. Is that available now?
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

On 12/6/14, 5:21 PM, Kouhei Kaigai wrote:
>>> > >Yes please. We have other contrib modules that exist as tests, so this
>>> > >seems reasonable to me.
>> >
>> >I can't improve the docs without the example code. Is that available now?
>> >
> Please wait for a few days. The ctidscan module is not adjusted for the
> latest interface yet.

I've made some minor edits, with an emphasis on not changing original intent. Each section was saved as a separate edit, so if anyone objects to something just revert the relevant change. Once the code is available more editing can be done.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 7 December 2014 at 08:21, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:

> Please wait for a few days. The ctidscan module is not adjusted for the
> latest interface yet.

I am in many ways a patient man. At this point it is 12 days since my
request for a working example.

Feedback I am receiving is that the API is unusable. That could be
because it is impenetrable, or because it is unusable. I'm not sure it
matters which.

We need a working example to ensure that the API meets the needs of a
wide section of users and if it does not, to give other users a chance
to request changes to the API so that it becomes usable. The window
for such feedback is approaching zero very quickly now and we need
action.

Thanks

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon,

The sample code is here:
https://github.com/kaigai/ctidscan

The code itself and regression tests shows how does it work and
interact with the core backend.

However, its source code comments are not updated and SGML document
is not ready yet, because of my schedule in earlier half of December.
I try to add the above stuff for a patch of contrib module, but will
take a few more days.

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

> -----Original Message-----
> From: Simon Riggs [mailto:simon(at)2ndQuadrant(dot)com]
> Sent: Tuesday, December 09, 2014 12:24 AM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; Thom Brown; Kohei KaiGai; Tom Lane; Alvaro Herrera; Shigeru
> Hanada; Stephen Frost; Andres Freund; PgHacker; Jim Mlodgenski; Peter
> Eisentraut
> Subject: Re: [HACKERS] [v9.5] Custom Plan API
>
> On 7 December 2014 at 08:21, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
> > Please wait for a few days. The ctidscan module is not adjusted for
> > the latest interface yet.
>
> I am in many ways a patient man. At this point it is 12 days since my request
> for a working example.
>
> Feedback I am receiving is that the API is unusable. That could be because
> it is impenetrable, or because it is unusable. I'm not sure it matters which.
>
> We need a working example to ensure that the API meets the needs of a wide
> section of users and if it does not, to give other users a chance to request
> changes to the API so that it becomes usable. The window for such feedback
> is approaching zero very quickly now and we need action.
>
> Thanks
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Dec 9, 2014 at 3:24 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Feedback I am receiving is that the API is unusable. That could be
> because it is impenetrable, or because it is unusable. I'm not sure it
> matters which.

It would be nice to here what someone is trying to use it for and what
problems that person is encountering. Without that, it's pretty much
impossible for anyone to fix anything.

As for sample code, KaiGai had a working example, which of course got
broken when Tom changed the API, but it didn't look to me like Tom's
changes would have made anything impossible that was possible before.
I'm frankly kind of astonished by the tenor of this entire
conversation; there is certainly plenty of code in the backend that is
less self-documenting than this is; and KaiGai did already put up a
wiki page with documentation as you requested. From his response, it
sounds like he has updated the ctidscan code, too.

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