Re: [v9.5] Custom Plan API - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: [v9.5] Custom Plan API
Date
Msg-id CAA4eK1Jc3NEv8r1wJv7qJxbHVuGvrPwmfWuCairnmQSVPewQug@mail.gmail.com
Whole thread Raw
In response to Re: [v9.5] Custom Plan API  (Robert Haas <robertmhaas@gmail.com>)
List 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

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: Compiler warning in master branch
Next
From: Michael Paquier
Date:
Subject: Re: [REVIEW] Re: Compression of full-page-writes