Hi Tatsuo,
Thank you for going through all ten 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.
Then please go ahead and build v54 from the ten patches as they are.
It is not that I found nothing that needs fixing, but what we need now
is a patch that can serve as the basis for discussion. From here on,
I think putting consensus first, even if it is slower, is a faster way
to a commit than moving quickly. I will include the five fixes below
in the next increment.
Meanwhile, I am running a quality audit of the series as I expect v54
to be. Once v54 is posted, I would like to audit it again and share
the results with you.
>> 0001 Let a row pattern DEFINE clause take part in grouping
>> [wrong result]
>
> 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.
Agreed; that loop will use foreach_node like the others in the patch.
>> 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.
>
> I still need to learn more about planner, but the direction looks
> right to me.
Thank you. I also think the planner is an area that takes a lot of
study, and it would be better to widen my knowledge a little more
before taking it further. The quality audit is turning up some
planner-related points as well; once v54 is out, I will put them
together and send them along with the results. I think they will give
us leads for future improvements.
>> 0005 Make a deparsed DEFINE clause re-parse as written
>> [wrong result]
>
> +static void
> +reserve_colname(deparse_namespace *dpns, char *colname)
> +{
> ...
> + 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.
Agreed. No current caller passes a literal, but the callers may
change, so copying with pstrdup() as you suggest is the safer way.
While we are on 0005: as I wrote on 10-08, I would like to pull
the generic ruleutils.c fixes inside it (TABLEFUNC column alias
lists, columns added to a function RTE after the view was created,
and relation RTEs that do not appear in FROM) out into a separate
prerequisite patch with its own thread and CF entry, and to rebase the
row pattern series on top of it. v54 can keep them as they are; I plan
to do the split in the next increment.
>> 0006 Tighten RPRPattern node I/O and reuse existing helpers in RPR
>> code [latent]
>
> + /*
> + * 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.
Agreed.
>> 0007 Record two RPR costs and correct comments the code no longer
>> matches [comments, asserts]
>
> I had no time to read the details.
>> 0008 Tidy and extend the row pattern recognition regression tests
>> [tests]
>
> I had no time to read the details.
No problem. I am reviewing these further on my side, and I will send
what I find along with the audit results.
>> 0009 Bring the row pattern recognition documentation up to date
>> [docs]
>
> 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.
Agreed. I will add scan.l, and mention psqlscan.l and pgc.l, which
have to follow it, on the same line.
> 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
Agreed on the direction. Throughout the docs, though,
<command>ANALYZE</command> alone always means the statistics command,
so I think it is better to follow one of the two existing precedents:
<command>EXPLAIN ANALYZE</command> for the command, or "the
<literal>ANALYZE</literal> option" for the option.
>> - 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.
Please take your time. On my side, I have found a few leads on how to
deal with the state explosion. Some are simple, and one is involved
enough that it takes understanding a paper first. I will come back to
you once they are in better shape.
>> - 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.
Thank you. I will put together a draft of the RPR* naming, struct
members included, so that we can agree on the 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.
>
> In the last this is necessary but it does not need to be hurry.
Understood. I will wait for the right time.
>> - 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".
YES with a note sounds right to me. I think this change is yours to
make, so I will leave it out of my patches.
Best regards,
Henson
