Re: [v9.5] Custom Plan API

On 29 September 2014 09:48, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

On Wed, Sep 17, 2014 at 7:40 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

At this moment, I revised the above portion of the patches.
create_custom_plan() was modified to call "PlanCustomPath" callback
next to the initialization of tlist and clauses.

It's probably same as what you suggested.

create_custom_plan() is mis-named. It's actually only applicable to the
custom-scan case, because it's triggered by create_plan_recurse() getting
a path node with a T_CustomScan pathtype. Now, we could change that;
although in general create_plan_recurse() dispatches on pathtype, we could
make CustomPath an exception; the top of that function could say if
(IsA(best_path, CustomPath)) { /* do custom stuff */ }. But the problem
with that idea is that, when the custom path is specifically a custom scan,
rather than a join or some other thing, you want to do all of the same
processing that's in create_scan_plan().

So I think what should happen is that create_plan_recurse() should handle
T_CustomScan the same way it handles T_SeqScan, T_IndexScan, et
al: by calling create_scan_plan(). The switch inside that function can
then call a function create_customscan_plan() if it sees T_CustomScan. And
that function will be simpler than the
create_custom_plan() that you have now, and it will be named correctly,
too.

Fixed, according to what you suggested. It seems to me create_customscan_plan()
became more simplified than before.
Probably, it will minimize the portion of special case handling if CustomScan
with scanrelid==0 replaces built-in join plan in the future version.

In ExplainNode(), I think sname should be set to "Custom Scan", not "Custom".
And further down, the custom_name should be printed as "Custom Plan
Provider" not just "Custom".

Fixed. I added an additional regression test to check EXPLAIN output
if not a text format.

setrefs.c has remaining handling for the scanrelid = 0 case; please remove
that.

Sorry, I removed it, and checked the patch again to ensure here is no similar
portions.

Thanks for your reviewing.

pgsql-v9.5-custom-scan.part-2.v11.patch

+GetSpecialCustomVar(CustomPlanState *node,
+                    Var *varnode,
+                    PlanState **child_ps);

This doesn't seem to strictly match the actual function:

+GetSpecialCustomVar(PlanState *ps, Var *varnode, PlanState **child_ps)

--
Thom

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-09-29 20:26 GMT+09:00 Thom Brown <thom@linux.com>:

On 29 September 2014 09:48, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

On Wed, Sep 17, 2014 at 7:40 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

At this moment, I revised the above portion of the patches.
create_custom_plan() was modified to call "PlanCustomPath" callback
next to the initialization of tlist and clauses.

It's probably same as what you suggested.

create_custom_plan() is mis-named. It's actually only applicable to the
custom-scan case, because it's triggered by create_plan_recurse() getting
a path node with a T_CustomScan pathtype. Now, we could change that;
although in general create_plan_recurse() dispatches on pathtype, we could
make CustomPath an exception; the top of that function could say if
(IsA(best_path, CustomPath)) { /* do custom stuff */ }. But the problem
with that idea is that, when the custom path is specifically a custom scan,
rather than a join or some other thing, you want to do all of the same
processing that's in create_scan_plan().

So I think what should happen is that create_plan_recurse() should handle
T_CustomScan the same way it handles T_SeqScan, T_IndexScan, et
al: by calling create_scan_plan(). The switch inside that function can
then call a function create_customscan_plan() if it sees T_CustomScan. And
that function will be simpler than the
create_custom_plan() that you have now, and it will be named correctly,
too.

Fixed, according to what you suggested. It seems to me create_customscan_plan()
became more simplified than before.
Probably, it will minimize the portion of special case handling if CustomScan
with scanrelid==0 replaces built-in join plan in the future version.

In ExplainNode(), I think sname should be set to "Custom Scan", not "Custom".
And further down, the custom_name should be printed as "Custom Plan
Provider" not just "Custom".

Fixed. I added an additional regression test to check EXPLAIN output
if not a text format.

setrefs.c has remaining handling for the scanrelid = 0 case; please remove
that.

Sorry, I removed it, and checked the patch again to ensure here is no similar
portions.

Thanks for your reviewing.

pgsql-v9.5-custom-scan.part-2.v11.patch

+GetSpecialCustomVar(CustomPlanState *node,
+                    Var *varnode,
+                    PlanState **child_ps);

This doesn't seem to strictly match the actual function:

+GetSpecialCustomVar(PlanState *ps, Var *varnode, PlanState **child_ps)

It's more convenient if the first argument is PlanState, because
GetSpecialCustomVar() is called towards all the suspicious special
var-node that might be managed by custom-plan provider.
If we have to ensure its first argument is CustomPlanState on the
caller side, it makes function's invocation more complicated.
Also, the callback portion is called only when PlanState is
CustomPlanState, so it is natural to take CustomPlanState for
argument of the callback interface.
Do we need to match the prototype of wrapper function with callback?

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

Do we need to match the prototype of wrapper function with callback?

Yes.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai <kaigai@kaigai.gr.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@ak.jp.nec.com>

On 30 September 2014 07:27, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:

On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai <kaigai@kaigai.gr.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@ak.jp.nec.com>

On Mon, Oct 27, 2014 at 2:35 AM, Kouhei Kaigai <kaigai@ak.jp.nec.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 27, 2014 at 2:35 AM, Kouhei Kaigai <kaigai@ak.jp.nec.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.

Thanks for your great help.

I and Hanada-san have discussed a further enhancement of that interface
that allows to replace a join by custom-scan; probably, can be utilized
with an extension that runs materialized-view instead of join on the fly.
We will submit a design proposal of this enhancement later.

Best regards,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Nov 8, 2014 at 4:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Oct 27, 2014 at 2:35 AM, Kouhei Kaigai <kaigai@ak.jp.nec.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

On Sat, Nov 8, 2014 at 4:16 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Oct 27, 2014 at 2:35 AM, Kouhei Kaigai <kaigai@ak.jp.nec.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@ak.jp.nec.com>

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.

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 10, 2014 at 6:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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

On Mon, Nov 10, 2014 at 6:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane <tgl@sss.pgh.pa.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.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@ak.jp.nec.com>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane <tgl@sss.pgh.pa.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.

So, the existing form shall be revised as follows?

* CustomScan shall not be a base type of custom data-type managed by
extension, instead of private data field.
* It also eliminates CopyCustomScan and TextOutCustomPath/Scan callback.
* Expression nodes that will not be processed by core backend, but
processed by extension shall be connected to special field, like
fdw_exprs in FDW.
* Translation between a pseudo varnode and original expression node
shall be informed to the core backend, instead of SetCustomScanRef
and GetSpecialCustomVar.

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.

For people's convenient, I'd like to set up a wikipage to write up a draft
of SGML documentation for easy updates by native English speakers.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kouhei Kaigai <kaigai@ak.jp.nec.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers