public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Row pattern recognition
Date: Mon, 01 Jun 2026 11:11:19 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAAAe_zDDuJofafXyhggPPPLzUXeejH-19NfcLR7jNNQxZchtog@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<CAAAe_zDDuJofafXyhggPPPLzUXeejH-19NfcLR7jNNQxZchtog@mail.gmail.com>

Hi Henson,

> Good catch -- you're right, and I'll revert it to static in v48. I
> double-checked the current tree: the only callers left are inside
> parse_clause.c (the findTargetlistEntry wrapper, plus the GROUP BY and ORDER
> BY paths), so the extern prototype in parse_clause.h is now unused. The
> revert is just dropping that prototype and restoring the static qualifier
> with the in-file forward declaration it used to have.
> 
> It's worth saying why it went extern in the first place, since the reason is
> no longer visible in the tree:
> 
> The DEFINE clause needs its referenced columns present in the plan's
> targetlist to be evaluable at run time. The original implementation did
> that from the RPR side, in parse_rpr.c, by calling
> findTargetlistEntrySQL99()
> with resjunk = true to add the missing entry -- and since that function was
> static in parse_clause.c, reaching it across files is what required exposing
> it as extern.
> 
> That approach added the whole DEFINE expression to the targetlist, and that
> turned out to be the source of a SIGSEGV: when an RPR window and a plain
> window coexist, the non-RPR WindowAgg inherited targetlist entries carrying
> RPRNavExpr nodes it has no way to evaluate. The fix was to add only the Vars
> a DEFINE references (with a guard in allpaths.c to keep those columns from
> being pruned), and that is what removed the cross-file call. So the extern
> has simply outlived its caller -- exactly as you spotted.
> 
> The one loose end there is an optimization, not a correctness issue: the
> allpaths.c guard is deliberately coarse, so it also blocks removing a
> WindowAgg whose RPR WindowFuncs are all unused. Doing that precisely means
> restructuring remove_unused_subquery_outputs(), which runs for every
> subquery and not just RPR -- broad enough that I'm treating it as a
> longer-term item rather than part of this work. It doesn't bring the
> external call back -- the Var-only path stays -- so the revert to static is
> safe independently of it.
> 
> I'll fold the static revert into v48.

Thank for eplaining the history.

BTW, in v47-0002 patch,
there are some non ASCII characters ("§").

+	/*
+	 * Qualified column references in DEFINE are not supported.  This covers
+	 * both FROM-clause range variables (prohibited by §6.5) and pattern
+	 * variable qualified names (e.g. UP.price), which are valid per §4.16
+	 * but not yet implemented.
+	 */

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

view thread (109+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Row pattern recognition
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox