Hi Henson and Jian,
> This is an increment on top of v53. It is ten patches, named
> nocfbot-XXXX-*.txt as before.
Thank you for the patches!
I looked through nocfbot-0001 to 0010. Since all the comments are
minor, I think I can build v54 from the ten patches as it is. If you
think differently, please let me know.
> 0001 Let a row pattern DEFINE clause take part in grouping
> [wrong result]
> v53 left DEFINE out of the GROUP BY substitution, so a grouping
> set that nulls a column made setrefs.c fail with "wrong
> varnullingrels". DEFINE now goes through the same steps as the
> target list. Also fixes PREV(k / 0) failing at plan time when
> k is a USING column of mixed types.
Looks good to me. Minor point.
+ /*
+ * A row pattern DEFINE clause carries GROUP Vars of its own;
expand
+ * them, or the deparsed text would name the grouping step
rather than
+ * the expression the user wrote, and the view would not
re-parse.
+ */
+ foreach(lc, query->windowClause)
Can we use foreach_node instead or foreach? In the same patch
foreach_node is used.
> 0002 Tighten parse analysis of DEFINE clauses and row pattern
> syntax [wrong result]
> Whole-row references, ROW(t.*), names qualified by a function
> name or block label, and the FILTER or ORDER BY of an aggregate
> got around the qualifier rule of ISO/IEC 19075-5 6.5. They
> are rejected now, and a few SQLSTATEs change.
Looks good to me.
> 0003 Fix DEFINE memory leak and empty-iteration looping in RPR
> executor [wrong result]
> An empty first iteration kept a derivation the standard
> excludes, so some patterns matched different rows than Perl;
> query results change for such patterns. Also fixes the DEFINE
> memory leak (94MB down to 18MB on 300k rows) and adds an
> interrupt check.
I briefly looked through this. No objection from me.
> 0004 Move DEFINE input tracking from parse analysis to the planner
> [wrong result]
> Parse analysis planted DEFINE's Vars in the target list, and
> planning could fail with "variable not found in subplan target
> list". DEFINE is now handled the way havingQual is. Also
> stops subquery pull-up from folding a navigation argument, and
> fixes a crash on a navigation offset.
I still need to learn more about planner, but the direction looks
right to me.
> 0005 Make a deparsed DEFINE clause re-parse as written
> [wrong result]
> A view holding a DEFINE clause could not be dumped and restored
> once another column carried the same name. The names DEFINE
> reads are now settled first and reserved. Also changes
> pg_get_viewdef() output for some queries without row pattern
> recognition; the commit message lists them.
+/*
+ * reserve_colname: keep any other RTE from being given this column name
+ */
+static void
+reserve_colname(deparse_namespace *dpns, char *colname)
+{
+ ListCell *lc;
+
+ foreach(lc, dpns->using_names)
+ {
+ if (strcmp((char *) lfirst(lc), colname) == 0)
+ return;
+ }
+ dpns->using_names = lappend(dpns->using_names, colname);
+}
colname should be duplicated using pstrdup() before passing to
lappend? As some of callers of this function might pass a string
literal.
+ if (rte->rtekind == RTE_RELATION)
+ reserve_colname(dpns, get_rte_attribute_name(rte,
attno));
>
> 0006 Tighten RPRPattern node I/O and reuse existing helpers in RPR
> code [latent]
> A malformed node string now fails with an internal error
> instead of being misread. Existing helpers replace
> hand-written code.
Overall looks good. Minor point:
@@ -728,23 +727,27 @@ _outRPRPattern(StringInfo str, const RPRPattern *node)
WRITE_INT_FIELD(maxDepth);
WRITE_INT_FIELD(numElements);
- /* Write varNames array as list of strings */
+ /*
+ * Write varNames array as list of strings. makeRPRPattern() guarantees
+ * the array, so the list has exactly one spelling and the read side has
+ * no second shape to interpret.
+ */
I think "makeRPRPattern() guarantees the array" should be
"makeRPRPattern() guarantees the array exists." to make what it means
cleaner.
> 0007 Record two RPR costs and correct comments the code no longer
> matches [comments, asserts]
> Comments, plus five checks the executor cannot reach, now
> Asserts: two elog(ERROR) calls and three early returns.
> Records two known costs as XXX: the state comparison in
> nfa_states_equal(), and nav_winobj's shared read pointer.
I had no time to read the details.
> 0008 Tidy and extend the row pattern recognition regression tests
> [tests]
> Tests only: rpr_ prefixes, fewer redundant ORDER BY, new tests,
> and fixes for tests that tested less than their comments said.
I had no time to read the details.
> 0009 Bring the row pattern recognition documentation up to date
> [docs]
> Documentation only: SGML and README.rpr checked against the
> code.
Looks good except:
--- a/src/backend/executor/README.rpr
+++ b/src/backend/executor/README.rpr
+ Related code, by the phase each file serves. A file appears once,
+ under the phase where its row pattern work belongs:
+
+ Parser
scan.l seems missed in the list.
In perform.sgml there are few lines using <literal> tag for ANALYZE
command. The tag should be <command>, rather than <literal>.
Under <literal>ANALYZE</literal>, a node that was executed shows the
bound resolved for its last scan instead of <literal>runtime</literal>:
a row count, <literal>retain all</literal> or
<literal>infinite</literal>.
</para>
<para>
Under <literal>ANALYZE</literal>, further lines report counters for the
> 0010 Reorder README.rpr by processing order [docs]
> Moves the chapters into the order a query goes through the
> code: parsing, DEFINE planning, PATTERN compilation, the
> executor, the worked example, deparse and EXPLAIN, and the
> summary last. Text moves; section numbers and cross references
> follow, and the Chapter IV title is reworded.
Looks good.
> Open questions
>
> Tatsuo, four items need your decision on the direction or on whether
> to go ahead:
>
> - State explosion. To keep the preference order from giving wrong
> answers, the cycle guard marks only a nullable END, since marking
> anything else loses legitimate matches. That bounds cycles but not
> cost: alternations whose branches are all nullable enumerate paths
> rather than states, as an XXX in nfa_advance_state() records. A
> runtime bound, on the work in one expansion or on the states held,
> is the first answer; rejecting quantified subpatterns that can
> match empty, as Oracle does with ORA-62513, is the alternative.
I need more time to answer to the question.
> - Names. You accepted the RPR* policy for the typedefs on 07-14 [1]
> and asked whether it reaches struct members too. I would say yes:
> rpSkipTo becomes rprSkipTo along with the types, in one mechanical
> patch after the defects are settled.
Agreed on this direction.
> - WindowAggState. On 05-31 [2] you agreed with Jian's idea [3] of
> putting the row pattern members behind one struct, for the size
> and for the move to R010, but not for v48. On 07-14 [1] you asked
> when to do it, saying it has to be done to make the code
> committable. I would group the members before the commit, since
> that only changes structure, and leave the R010 redesign for
> after. The struct needs a name other than RPRContext, since
> rprContext is already the ExprContext DEFINE runs in.
In the last this is necessary but it does not need to be hurry.
> - sql_features.txt still says R020 is NO although the patch
> implements it. It has to change before the commit. Whether it
> says YES with a note or NO, partially supported is your call.
When I implemented RPR alone, there were too many missing features. So
I thought the status should be "NO". Now many features in R020 are
implemneted in the current patch. However, we still miss a big
features: range variable qualified pattern variables and MEASURES
clause. So someone might say that the current patch is not eligible to
be marked as "R020 is supported". However I personally think even
without those two features, current patch is useful enough to enjoy
the split of R020. This is really the community's decision. Until the
decision is made, I would like to change it "YES with a note".
> None of these is urgent, and none is a structural problem that puts
> the quality of the patch at risk. Each is a matter of direction or
> timing that can wait until the code has settled, and I will follow
> whichever way you decide.
Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
