public inbox for [email protected]  

help / color / mirror / Atom feed

From: Henson Choi <[email protected]>
To: Tatsuo Ishii <[email protected]>
To: jian he <[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, 1 Jun 2026 10:16:54 +0900
Message-ID: <CAAAe_zDDuJofafXyhggPPPLzUXeejH-19NfcLR7jNNQxZchtog@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAAAe_zBdwAUDNs_WFdLkFF=ewhkDkv-AqizVEVzhsfremGFb4w@mail.gmail.com>
	<[email protected]>
	<[email protected]>

Hi Tatsuo,

> I accidentaly noticed that v47-0002 changes findTargetlistEntrySQL99
> from static to extern.
> [...]
> I think this is not necessary anymore since findTargetlistEntrySQL99
> is not used outside parse_clause.c.

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.

Thanks,
Henson

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: <CAAAe_zDDuJofafXyhggPPPLzUXeejH-19NfcLR7jNNQxZchtog@mail.gmail.com>

* 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