MIME-Version: 1.0
References: <20260202.162000.2139438609347688347.ishii@postgresql.org>
 <20260203.090039.1140229154610300206.ishii@postgresql.org>
 <CAAAe_zDtstKUcKsH3FJuKeZ=ykXFWKO1Lz80JXEn9y8BeQeYNQ@mail.gmail.com> <20260204.090451.1095005750732720998.ishii@postgresql.org>
In-Reply-To: <20260204.090451.1095005750732720998.ishii@postgresql.org>
Reply-To: assam258@gmail.com
From: Henson Choi <assam258@gmail.com>
Date: Wed, 4 Feb 2026 17:54:26 +0900
Message-ID: <CAAAe_zC95wT9EdBu_nBn6mWVB7-xNNuJ4N8wPr0LrWJBdwmxiQ@mail.gmail.com>
Subject: Re: Row pattern recognition
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: jacob.champion@enterprisedb.com, david.g.johnston@gmail.com, 
	vik@postgresfriends.org, er@xs4all.nl, peter@eisentraut.org, 
	pgsql-hackers@postgresql.org
Content-Type: multipart/alternative; boundary="000000000000b20b300649fbb46f"
Archived-At: <https://www.postgresql.org/message-id/CAAAe_zC95wT9EdBu_nBn6mWVB7-xNNuJ4N8wPr0LrWJBdwmxiQ%40mail.gmail.com>
Precedence: bulk

--000000000000b20b300649fbb46f
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Tatsuo,

BTW, I have a question regarding NFA related objects initialization in
> nodeWindowAgg.c.
>
> In release_partition():
>         /* Reset NFA state for new partition */
>         winstate->nfaContext =3D NULL;
>         winstate->nfaContextTail =3D NULL;
>         winstate->nfaContextFree =3D NULL;
>         winstate->nfaStateFree =3D NULL;
>         winstate->nfaLastProcessedRow =3D -1;
>         winstate->nfaStatesActive =3D 0;
>         winstate->nfaContextsActive =3D 0;
>
> While in ExecInitWindowAgg():
>         /* Initialize NFA free lists for row pattern matching */
>         winstate->nfaContext =3D NULL;
>         winstate->nfaContextTail =3D NULL;
>         winstate->nfaContextFree =3D NULL;
>         winstate->nfaStateFree =3D NULL;
>         winstate->nfaLastProcessedRow =3D -1;
>
> So the latter does not initialize winstate->nfaStatesActive and
> winstate->nfaContextsActive. Can you please explain why?
>

You're absolutely right to notice this inconsistency. Let me explain:

## Why ExecInitWindowAgg() omits these two fields

In ExecInitWindowAgg(), the WindowAggState structure is created via
makeNode(WindowAggState), which internally uses palloc0(). This
zero-initializes all memory, so:

  - All pointers =E2=86=92 NULL (0)
  - nfaStatesActive =E2=86=92 0
  - nfaContextsActive =E2=86=92 0
  - All other statistics counters =E2=86=92 0

Technically, only nfaLastProcessedRow =3D -1 truly needs explicit
initialization since it requires a non-zero value. The nfaContext =3D NULL
assignments are redundant but follow PostgreSQL's convention of
explicitly setting pointers to NULL for code clarity and documentation.

The pattern in ExecInitWindowAgg() follows PostgreSQL's common practice:
  - Pointers: explicitly initialized to NULL (for readability)
  - Counters: implicitly zero-initialized via palloc0() (intentionally
omitted)

This same pattern appears throughout the function for other fields too.
For example, framehead_slot and frametail_slot are explicitly set to NULL,
while framehead_valid and frametail_valid are left zero-initialized.

## What nfaStatesActive and nfaContextsActive do

These two fields serve as internal counters for tracking active NFA
resources:

nfaStatesActive: Tracks the current number of active NFA states in memory.
  - Incremented in nfa_state_alloc() when allocating a new state
  - Decremented in nfa_state_free() when returning a state to the free list
  - Used to calculate nfaStatesMax (peak usage) for EXPLAIN ANALYZE

nfaContextsActive: Tracks the current number of active NFA contexts.
  - Incremented in nfa_context_alloc() when creating a new context
  - Decremented in nfa_context_free() when recycling a context
  - Used to calculate nfaContextsMax (peak usage) for EXPLAIN ANALYZE

Both counters start at 0 and are updated during NFA execution to track
resource usage for performance monitoring.

## Why release_partition() must explicitly reset them

release_partition() is called when moving to a new partition and must
reset per-partition state. Unlike ExecInitWindowAgg() which creates a
fresh structure, release_partition() reuses an existing structure that
contains values from the previous partition.

The root cause of the reset requirement is that release_partition()
calls MemoryContextReset(winstate->partcontext), which frees all
partition-local memory. This includes:
  - All NFA context structures pointed to by nfaContext
  - All NFA state nodes in the free lists (nfaContextFree, nfaStateFree)
  - All other partition-specific data

As a consequence of this memory reset:
  1. The NFA pointers (nfaContext, nfaContextFree, etc.) must be set to
     NULL to avoid dangling pointers
  2. The active resource counters (nfaStatesActive, nfaContextsActive)
     must be reset to 0 since all the resources they were counting have
     been freed

Example scenario:
  Partition 1 processing:
    - nfaStatesActive increments one-by-one as states are allocated,
      decrements as they are freed, fluctuates during execution,
      ends at some non-zero value (e.g., 3)
    - nfaContextsActive similarly fluctuates, ends at some value (e.g., 2)

  Partition 2 starts =E2=86=92 release_partition() called:
    - MemoryContextReset(partcontext) frees all NFA structures
    - nfaContext, nfaContextFree, etc. =E2=86=92 must set to NULL
    - nfaStatesActive: 3 =E2=86=92 must reset to 0
    - nfaContextsActive: 2 =E2=86=92 must reset to 0

These counters track instantaneous active resources within a partition,
so they must be reset to 0 when starting a new partition. In contrast,
cumulative statistics like nfaStatesMax and nfaStatesTotalCreated are
NOT reset because they track overall query-wide statistics across all
partitions.

## Why only these two counters?

You might wonder why I don't propose initializing all the other NFA
statistics fields as well. ExecInitWindowAgg() currently omits explicit
initialization for many statistics fields:

  Per-partition internal counters (reset at partition boundaries):
    - nfaStatesActive =3D 0;        (omitted)
    - nfaContextsActive =3D 0;      (omitted)

  Query-wide cumulative statistics (never reset):
    - nfaStatesMax =3D 0;                 (omitted)
    - nfaStatesTotalCreated =3D 0;        (omitted)
    - nfaStatesMerged =3D 0;              (omitted)
    - nfaContextsMax =3D 0;               (omitted)
    - nfaContextsTotalCreated =3D 0;      (omitted)
    - nfaContextsAbsorbed =3D 0;          (omitted)
    - nfaContextsSkipped =3D 0;           (omitted)
    - nfaContextsPruned =3D 0;            (omitted)
    - nfaMatchesSucceeded =3D 0;          (omitted)
    - nfaMatchesFailed =3D 0;             (omitted)
    - nfaMatchLen =3D {0, 0, 0};          (omitted)
    - nfaFailLen =3D {0, 0, 0};           (omitted)
    - nfaAbsorbedLen =3D {0, 0, 0};       (omitted)
    - nfaSkippedLen =3D {0, 0, 0};        (omitted)

The key distinction is that nfaStatesActive and nfaContextsActive are
the ONLY statistics that get reset in release_partition(). All the other
statistics are cumulative across the entire query and should never be
reset between partitions.

For example, nfaStatesMax tracks the peak number of active states across
ALL partitions, not just within one partition. Similarly,
nfaStatesTotalCreated accumulates the total count across all partitions.
These cumulative statistics start at 0 (via palloc0) and only increase,
never getting reset.

Therefore, I believe it only makes sense to add explicit initialization
for nfaStatesActive and nfaContextsActive, since these are the only two
that release_partition() also explicitly resets. Adding explicit
initialization for all the cumulative statistics would be misleading,
as it might suggest they get reset somewhere (which they don't).

## Proposal for consistency

For consistency with release_partition(), I can add just the two
per-partition counter initializations to ExecInitWindowAgg():

  /* Initialize NFA free lists for row pattern matching */
  winstate->nfaContext =3D NULL;
  winstate->nfaContextTail =3D NULL;
  winstate->nfaContextFree =3D NULL;
  winstate->nfaStateFree =3D NULL;
  winstate->nfaLastProcessedRow =3D -1;
  winstate->nfaStatesActive =3D 0;        // Add this
  winstate->nfaContextsActive =3D 0;      // Add this

This would make both ExecInitWindowAgg() and release_partition()
initialize the same set of per-partition NFA fields, making the code
more uniform and easier to review. The cumulative statistics fields
remain omitted in both functions, which correctly reflects that they
are never reset.

Functionally, this change makes no difference since palloc0() already
zeros these fields, but it improves code consistency and makes the
per-partition vs. query-wide distinction clearer.

Would you like me to include this change in the next patch?

Best regards,
Henson

--000000000000b20b300649fbb46f
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div><div dir=3D"ltr"><div dir=3D"ltr">Hi Tatsuo,</div><di=
v dir=3D"ltr"><br></div><div class=3D"gmail_quote"></div></div></div><div><=
blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l=
eft-width:1px;border-left-style:solid;padding-left:1ex;border-left-color:rg=
b(204,204,204)">
BTW, I have a question regarding NFA related objects initialization in<br>
nodeWindowAgg.c.<br>
<br>
In release_partition():<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Reset NFA state for new partition */<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContext =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContextTail =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContextFree =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaStateFree =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaLastProcessedRow =3D -1;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaStatesActive =3D 0;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContextsActive =3D 0;<br>
<br>
While in ExecInitWindowAgg():<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Initialize NFA free lists for row pattern ma=
tching */<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContext =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContextTail =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaContextFree =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaStateFree =3D NULL;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 winstate-&gt;nfaLastProcessedRow =3D -1;<br>
<br>
So the latter does not initialize winstate-&gt;nfaStatesActive and<br>
winstate-&gt;nfaContextsActive. Can you please explain why?<br></blockquote=
><div><br></div></div><div>You&#39;re absolutely right to notice this incon=
sistency. Let me explain:<br><br>## Why ExecInitWindowAgg() omits these two=
 fields<br><br>In ExecInitWindowAgg(), the WindowAggState structure is crea=
ted via<br>makeNode(WindowAggState), which internally uses palloc0(). This<=
br>zero-initializes all memory, so:<br><br>=C2=A0 - All pointers =E2=86=92 =
NULL (0)<br>=C2=A0 - nfaStatesActive =E2=86=92 0<br>=C2=A0 - nfaContextsAct=
ive =E2=86=92 0<br>=C2=A0 - All other statistics counters =E2=86=92 0<br><b=
r>Technically, only nfaLastProcessedRow =3D -1 truly needs explicit<br>init=
ialization since it requires a non-zero value. The nfaContext =3D NULL<br>a=
ssignments are redundant but follow PostgreSQL&#39;s convention of<br>expli=
citly setting pointers to NULL for code clarity and documentation.<br><br>T=
he pattern in ExecInitWindowAgg() follows PostgreSQL&#39;s common practice:=
<br>=C2=A0 - Pointers: explicitly initialized to NULL (for readability)<br>=
=C2=A0 - Counters: implicitly zero-initialized via palloc0() (intentionally=
 omitted)<br><br>This same pattern appears throughout the function for othe=
r fields too.<br>For example, framehead_slot and frametail_slot are explici=
tly set to NULL,<br>while framehead_valid and frametail_valid are left zero=
-initialized.<br><br>## What nfaStatesActive and nfaContextsActive do<br><b=
r>These two fields serve as internal counters for tracking active NFA<br>re=
sources:<br><br>nfaStatesActive: Tracks the current number of active NFA st=
ates in memory.<br>=C2=A0 - Incremented in nfa_state_alloc() when allocatin=
g a new state<br>=C2=A0 - Decremented in nfa_state_free() when returning a =
state to the free list<br>=C2=A0 - Used to calculate nfaStatesMax (peak usa=
ge) for EXPLAIN ANALYZE<br><br>nfaContextsActive: Tracks the current number=
 of active NFA contexts.<br>=C2=A0 - Incremented in nfa_context_alloc() whe=
n creating a new context<br>=C2=A0 - Decremented in nfa_context_free() when=
 recycling a context<br>=C2=A0 - Used to calculate nfaContextsMax (peak usa=
ge) for EXPLAIN ANALYZE<br><br>Both counters start at 0 and are updated dur=
ing NFA execution to track<br>resource usage for performance monitoring.<br=
><br>## Why release_partition() must explicitly reset them<br><br>release_p=
artition() is called when moving to a new partition and must<br>reset per-p=
artition state. Unlike ExecInitWindowAgg() which creates a<br>fresh structu=
re, release_partition() reuses an existing structure that<br>contains value=
s from the previous partition.<br><br>The root cause of the reset requireme=
nt is that release_partition()<br>calls MemoryContextReset(winstate-&gt;par=
tcontext), which frees all<br>partition-local memory. This includes:<br>=C2=
=A0 - All NFA context structures pointed to by nfaContext<br>=C2=A0 - All N=
FA state nodes in the free lists (nfaContextFree, nfaStateFree)<br>=C2=A0 -=
 All other partition-specific data<br><br>As a consequence of this memory r=
eset:<br>=C2=A0 1. The NFA pointers (nfaContext, nfaContextFree, etc.) must=
 be set to<br>=C2=A0 =C2=A0 =C2=A0NULL to avoid dangling pointers<br>=C2=A0=
 2. The active resource counters (nfaStatesActive, nfaContextsActive)<br>=
=C2=A0 =C2=A0 =C2=A0must be reset to 0 since all the resources they were co=
unting have<br>=C2=A0 =C2=A0 =C2=A0been freed<br><br>Example scenario:<br>=
=C2=A0 Partition 1 processing:<br>=C2=A0 =C2=A0 - nfaStatesActive increment=
s one-by-one as states are allocated,<br>=C2=A0 =C2=A0 =C2=A0 decrements as=
 they are freed, fluctuates during execution,<br>=C2=A0 =C2=A0 =C2=A0 ends =
at some non-zero value (e.g., 3)<br>=C2=A0 =C2=A0 - nfaContextsActive simil=
arly fluctuates, ends at some value (e.g., 2)<br><br>=C2=A0 Partition 2 sta=
rts =E2=86=92 release_partition() called:<br>=C2=A0 =C2=A0 - MemoryContextR=
eset(partcontext) frees all NFA structures<br>=C2=A0 =C2=A0 - nfaContext, n=
faContextFree, etc. =E2=86=92 must set to NULL<br>=C2=A0 =C2=A0 - nfaStates=
Active: 3 =E2=86=92 must reset to 0<br>=C2=A0 =C2=A0 - nfaContextsActive: 2=
 =E2=86=92 must reset to 0<br><br>These counters track instantaneous active=
 resources within a partition,<br>so they must be reset to 0 when starting =
a new partition. In contrast,<br>cumulative statistics like nfaStatesMax an=
d nfaStatesTotalCreated are<br>NOT reset because they track overall query-w=
ide statistics across all<br>partitions.<br><br>## Why only these two count=
ers?<br><br>You might wonder why I don&#39;t propose initializing all the o=
ther NFA<br>statistics fields as well. ExecInitWindowAgg() currently omits =
explicit<br>initialization for many statistics fields:<br><br>=C2=A0 Per-pa=
rtition internal counters (reset at partition boundaries):<br>=C2=A0 =C2=A0=
 - nfaStatesActive =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0(omitted)<br>=C2=A0 =
=C2=A0 - nfaContextsActive =3D 0; =C2=A0 =C2=A0 =C2=A0(omitted)<br><br>=C2=
=A0 Query-wide cumulative statistics (never reset):<br>=C2=A0 =C2=A0 - nfaS=
tatesMax =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (om=
itted)<br>=C2=A0 =C2=A0 - nfaStatesTotalCreated =3D 0; =C2=A0 =C2=A0 =C2=A0=
 =C2=A0(omitted)<br>=C2=A0 =C2=A0 - nfaStatesMerged =3D 0; =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(omitted)<br>=C2=A0 =C2=A0 - nfaContextsM=
ax =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (omitted)<br>=C2=
=A0 =C2=A0 - nfaContextsTotalCreated =3D 0; =C2=A0 =C2=A0 =C2=A0(omitted)<b=
r>=C2=A0 =C2=A0 - nfaContextsAbsorbed =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0(omitted)<br>=C2=A0 =C2=A0 - nfaContextsSkipped =3D 0; =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 (omitted)<br>=C2=A0 =C2=A0 - nfaContextsPruned =3D 0; =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(omitted)<br>=C2=A0 =C2=A0 - nfaMa=
tchesSucceeded =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(omitted)<br>=C2=A0=
 =C2=A0 - nfaMatchesFailed =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 (omitted)<br>=C2=A0 =C2=A0 - nfaMatchLen =3D {0, 0, 0}; =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0(omitted)<br>=C2=A0 =C2=A0 - nfaFailLen =3D {0, 0, 0}; =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (omitted)<br>=C2=A0 =C2=A0 - nfaAbsorbedLen=
 =3D {0, 0, 0}; =C2=A0 =C2=A0 =C2=A0 (omitted)<br>=C2=A0 =C2=A0 - nfaSkippe=
dLen =3D {0, 0, 0}; =C2=A0 =C2=A0 =C2=A0 =C2=A0(omitted)<br><br>The key dis=
tinction is that nfaStatesActive and nfaContextsActive are<br>the ONLY stat=
istics that get reset in release_partition(). All the other<br>statistics a=
re cumulative across the entire query and should never be<br>reset between =
partitions.<br><br>For example, nfaStatesMax tracks the peak number of acti=
ve states across<br>ALL partitions, not just within one partition. Similarl=
y,<br>nfaStatesTotalCreated accumulates the total count across all partitio=
ns.<br>These cumulative statistics start at 0 (via palloc0) and only increa=
se,<br>never getting reset.<br><br>Therefore, I believe it only makes sense=
 to add explicit initialization<br>for nfaStatesActive and nfaContextsActiv=
e, since these are the only two<br>that release_partition() also explicitly=
 resets. Adding explicit<br>initialization for all the cumulative statistic=
s would be misleading,<br>as it might suggest they get reset somewhere (whi=
ch they don&#39;t).<br><br>## Proposal for consistency<br><br>For consisten=
cy with release_partition(), I can add just the two<br>per-partition counte=
r initializations to ExecInitWindowAgg():<br><br>=C2=A0 /* Initialize NFA f=
ree lists for row pattern matching */<br>=C2=A0 winstate-&gt;nfaContext =3D=
 NULL;<br>=C2=A0 winstate-&gt;nfaContextTail =3D NULL;<br>=C2=A0 winstate-&=
gt;nfaContextFree =3D NULL;<br>=C2=A0 winstate-&gt;nfaStateFree =3D NULL;<b=
r>=C2=A0 winstate-&gt;nfaLastProcessedRow =3D -1;<br>=C2=A0 winstate-&gt;nf=
aStatesActive =3D 0; =C2=A0 =C2=A0 =C2=A0 =C2=A0// Add this<br>=C2=A0 winst=
ate-&gt;nfaContextsActive =3D 0; =C2=A0 =C2=A0 =C2=A0// Add this<br><br>Thi=
s would make both ExecInitWindowAgg() and release_partition()<br>initialize=
 the same set of per-partition NFA fields, making the code<br>more uniform =
and easier to review. The cumulative statistics fields<br>remain omitted in=
 both functions, which correctly reflects that they<br>are never reset.<br>=
<br>Functionally, this change makes no difference since palloc0() already<b=
r>zeros these fields, but it improves code consistency and makes the<br>per=
-partition vs. query-wide distinction clearer.<br><br>Would you like me to =
include this change in the next patch?<br><br>Best regards,<br>Henson<br></=
div><div>
</div>
</div>

--000000000000b20b300649fbb46f--