Date: Mon, 19 Oct 2026 09:52:59 +0900 (JST)
Message-Id: <20261019.095259.1988929700676866808.ishii@postgresql.org>
To: assam258@gmail.com, jian.universality@gmail.com
Cc: zsolt.parragi@percona.com, sjjang112233@gmail.com,
 vik@postgresfriends.org, er@xs4all.nl, jacob.champion@enterprisedb.com,
 david.g.johnston@gmail.com, peter@eisentraut.org, li.evan.chao@gmail.com,
 pgsql-hackers@postgresql.org
Subject: Re: Row pattern recognition
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: 
 <CAAAe_zDsYugq506ou49PU+Ok+4Umn5n59Qs5wYofvKyfEpvZJQ@mail.gmail.com>
References: 
 <CACJufxEGf2=W-Sx8cCcKV2jr8XCYeteBEjdBAW_HUJOwXkT-yQ@mail.gmail.com>
	<CAAAe_zDuRWBPHWainjJC_WS_uH270N2iJzU1CcE1B=fyuLnYsg@mail.gmail.com>
	<CAAAe_zDsYugq506ou49PU+Ok+4Umn5n59Qs5wYofvKyfEpvZJQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20261019.095259.1988929700676866808.ishii%40postgresql.org>
Precedence: bulk

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