Hi
2016-11-17 19:22 GMT+01:00 Alvaro Herrera <alvhe...@2ndquadrant.com>:
> I've been going over this patch. I think it'd be better to restructure
> the <sect2> before adding the docs for this new function; I already
> split it out, so don't do anything about this.
>
> Next, looking at struct TableExprBuilder I noticed that the comments are
> already obsolete, as they talk about function params that do not exist
> (missing_columns) and they fail to mention the ones that do exist.
> Also, function member SetContent is not documented at all. Overall,
> these comments do not convey a lot -- apparently, whoever reads them is
> already supposed to know how it works: "xyz sets a row generating
> filter" doesn't tell me anything. Since this is API documentation, it
> needs to be much clearer.
>
> ExecEvalTableExpr and ExecEvalTableExprProtected have no comments
> whatsoever. Needs fixed.
>
I am sending the patch with more comments - but it needs a care someone
with good English skills.
>
> I wonder if it'd be a good idea to install TableExpr first without the
> implementing XMLTABLE, so that it's clearer what is API and what is
> implementation.
>
I am not sure about this step - the API is clean from name. In this moment,
for this API is not any other tests than XMLTABLE implementation.
>
> The number of new keywords in this patch is depressing. I suppose
> there's no way around that -- as I understand, this is caused by the SQL
> standard's definition of the syntax for this feature.
>
> Have to go now for a bit -- will continue looking afterwards. Please
> submit delta patches on top of your latest v12 to fix the comments I
> mentioned.
>
>
Regards
Pavel
> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index c386be1..76c2ec8 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -4505,8 +4505,18 @@ ExecEvalCurrentOfExpr(ExprState *exprstate, ExprContext *econtext,
}
/* ----------------------------------------------------------------
- * ExecEvalTableExpr
+ * ExecEvalTableExprProtected and ExecEvalTableExpr
*
+ * Evaluate a TableExpr node. ExecEvalTableExprProtected is called
+ * from ExecEvalTableExpr from PG_TRY(), PG_CATCH() block be ensured
+ * all allocated memory be released.
+ *
+ * It creates TableExprBuilder object, does all necessary settings and
+ * reads all tuples from this object. Is possible to go over this
+ * cycle more times per query - when LATERAL JOIN is used. On the end
+ * of cycle, the TableExprBuilder object is destroyed, state pointer
+ * to this object is cleaned, and related memory context is resetted.
+ * New call starts new cycle.
* ----------------------------------------------------------------
*/
static Datum
@@ -4682,6 +4692,12 @@ ExecEvalTableExprProtected(TableExprState * tstate,
return result;
}
+/*
+ * ExecEvalTableExpr - this is envelop of ExecEvalTableExprProtected() function.
+ *
+ * This function ensures releasing all TableBuilder context and related
+ * memory context, when ExecEvalTableExprProtected fails on exception.
+ */
static Datum
ExecEvalTableExpr(TableExprState * tstate,
ExprContext *econtext,
diff --git a/src/include/executor/tableexpr.h b/src/include/executor/tableexpr.h
index ad5d8e2..3056317 100644
--- a/src/include/executor/tableexpr.h
+++ b/src/include/executor/tableexpr.h
@@ -20,27 +20,37 @@
* for generating content of table-expression functions like
* XMLTABLE.
*
- * CreateContext is called before execution and it does query level
- * initialization. Returns pointer to private TableExprBuilder data
- * (context). A query context should be active in call time. A param
- * missing_columns is true, when a user doesn't specify returned
- * columns.
+ * The TableBuilder is initialized by calling CreateContext function
+ * at evaluation time. First parameter - tuple descriptor describes
+ * produced (expected) table. in_functions is a array of FmgrInfo input
+ * functions for types of columns of produced table. The typioparams
+ * is a array of typio Oids for types of columns of produced table.
+ * The created context is living in special memory context passed
+ * as last parameter.
*
- * AddNs add namespace info when namespaces are is supported.
- * then passed namespace is default namespace. Namespaces should be
- * passed before Row/Column Paths setting. When name is NULL, then
- * related uri is default namespace.
+ * The SetContent function is used for passing input document to
+ * table builder. The table builder handler knows expected format
+ * and it can do some additional transformations that are not propagated
+ * out from table builder.
*
- * SetRowPath sets a row generating filter.
+ * The AddNs add namespace info when namespaces are supported.
+ * Namespaces should be passed before Row/Column Paths setting.
+ * When name is NULL, then related uri is default namespace.
*
- * SetColumnPath sets a column generating filter.
+ * The SetRowPath sets a row generating filter. This filter is used
+ * for separation of rows from document. Passed as cstring.
*
- * FetchRow ensure loading row raleted data. Returns false, when
- * document doesn't containt any next data.
+ * The SetColumnPath sets a column generating filter. This filter is
+ * used for separating nth value from row. Passed as cstring.
*
- * GetValue returns a value related to colnum column.
+ * The FetchRow ensure loading row raleted data. Returns false, when
+ * document doesn't containt any next row.
*
- * DestroyContext - release all memory
+ * The GetValue returns a value related to colnum column.
+ *
+ * The DestroyContext - should to release all sources related to
+ * processing the document. Called when all rows are fetched or
+ * when a error is catched.
*/
typedef struct TableExprBuilder
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
