Bound the group estimate when the planner had nothing to estimate from (#369)

[jdatcmd]

Closes #369. @ChronicallyJD for review, and this one has a long history of me being wrong
about it, so please be as hard on it as you were on #441.

The measurement, same fixture and box, both arms

	main	with the bound
G1 one aggregate, 12h window	serial, est 19,996,000, 2,017 ms	PARALLEL, est 722, 497 ms	4.1x
G2 ten aggregates	serial, est 19,996,000, 4,687 ms	PARALLEL, est 722, 1,146 ms	4.1x
G3 plain column key	PARALLEL, est 3998, 409 ms	PARALLEL, est 3998, 401 ms	unchanged

722 is floor(12h / 1min) + 2. Actual is 720.

G3's estimate is bit-identical, which is what I said I would assert rather than "still
fast". It is the detector for the gate leaking into the informed case, and it did not move
by a digit.

Why this and not the obvious fix

Raising the serial node's price cannot work, and the reason is worth having on the record
because it was my own first proposal. At a faithful cost_agg rate that node lands at
~578,578, which ties core's HashAggregate inside STD_FUZZ_FACTOR, and add_path's
tie-break then prefers the parallel-safe path. Ours sets parallel_safe = false. So
charging the serial node honestly does not flip G1 to our arm, it deletes the feature.

The inflated number is charged twice on the parallel arm, by the Gather as
parallel_tuple_cost * rows and again by the Finalize, and not at all on the serial
node, which is priced per input row. Attacking the estimate fixes both terms at once.

The gate is core's own test, not one I invented

examine_variable reporting a statsTuple or a unique index. That is exactly what
clausesel.c uses to decide whether an expression estimate is informed. It means:

• a plain Var is informed, so G3 never reaches the substitution
• an expression index is informed
• a user's CREATE STATISTICS ON (date_trunc(...)) is informed and outranks us

and only when nothing is informed do we substitute, only ever downward via Min.

The bound is deliberately narrow

One shape: a truncating time function over a single Var. Its distinct outputs cannot
exceed the buckets its input range spans, plus one partial at each end. The range comes
only from Const btree comparisons on that same Var in baserestrictinfo, which
describe the rows this scan will actually read.

month, quarter and year use their shortest possible length, so the count is
over-estimated rather than under. That direction matters: an under-estimate would
under-price the arm and under-size the Finalize's hash table into avoidable spill.

Coverage

native_groupagg.sh, 71 checks. The five new ones assert the bound is accurate rather
than merely smaller (12 for an 11-bucket window), that it really is an upper bound, that an
informed estimate is not replaced, and that both a key with no range bound and a non-time
expression key get nothing from us.

ungrouped_vector_agg, parallel_vector_agg, native_agg, parallel and
pushdown_report unchanged. Builds clean on 15, 16, 17, 18 and 19.

What I would attack if I were reviewing

• The USECS_PER_* arithmetic and the shortest-length choice for month/quarter/year.
  If any of those is wrong in the under-estimating direction, the bound stops being a
  bound.
• Whether examine_variable can report informed for something I have not thought of, which
  would silently disable the fix rather than break it. That fails safe, but quietly.
• Whether the Const extraction handles a timestamp against a timestamptz bound
  correctly. I only test matching types.

[ChronicallyJD]

The bound is applied to functions that are not date_trunc, and then it is not a bound

I went at this the way you asked. The design holds up: the gate is core's own test, the
substitution is only ever downward, hi <= lo is guarded so contradictory predicates cannot
produce a negative bound, both sides are required so an open-ended query gets nothing from
you, and the month/quarter/year widths are minimums (28/89/365 days) so those over-count
buckets, which is the safe direction. I could not break any of that.

I did break the premise the whole argument rests on.

f->funcid is never checked. pgcolumnar_truncating_time_bound matches on shape alone:
a FuncExpr of two or three args, a text Const first, a timestamp Var second. date_trunc
has that shape. So does any user function with the signature (text, timestamp).

Measured on your branch (8b02bd3), pg18a assert build, 500k rows across a 12h window:

group key	actual groups	estimate, feature on	estimate, feature off
date_trunc('minute',ts)	720	722	499,900
hi_card('minute',ts)	499,999	722	499,900

hi_card is deliberately not a truncating function, and PL/pgSQL because an SQL function
would be inlined and lose the shape:

CREATE FUNCTION hi_card(t text, s timestamp) RETURNS text AS
$$ BEGIN RETURN t || md5(s::text); END $$ LANGUAGE plpgsql IMMUTABLE;

Custom Scan (PgColumnarScan)  (cost=138742.76..138742.76 rows=722 width=40) (actual rows=499999.00 loops=1)

722 against 499,999 actual. A 692x underestimate, and core had it very nearly right at
499,900 before the substitution replaced it.

What it costs, interleaved so the first arm is not just the cold one

round	feature on	feature off
1	1328.2 ms	1139.9 ms
2	1309.0 ms	1133.5 ms
3	1350.9 ms	1130.9 ms

About 16% slower, consistently. Read that as "the plan this estimate wins" against "the plan
it displaces", not as a pure A/B of the bound, because the off arm disables the vectorized
aggregate entirely. Core's HashAggregate even spills here (9 batches, 31 MB disk) and is
still ahead.

I also went looking for something worse than a bad price, since a node costed for 722 groups
that receives 499,999 is the shape that blows up a pre-sized hash table. I did not find it.
Our node returned the right 499,999 rows and did not fail. So this is a mispricing, not a
correctness or memory bug.

Why I am not calling it urgent

The first argument has to be one of your unit names for the width lookup to succeed, so
myfunc('x', ts) gets no bound and only myfunc('minute', ts) does. That narrows real
exposure a long way. My first version of this probe used 'x', found nothing, and would have
told you the code was clean. Worth knowing if you reproduce it.

It still needs fixing, because the function claims an upper bound it has not established, and
the fix is one line against the date_trunc OIDs. You already include utils/fmgroids.h.

Smaller, and reasoned rather than measured

c->consttype may be TIMESTAMPOID or TIMESTAMPTZOID regardless of tsvar->vartype, and
the raw datum is read with DatumGetTimestamp either way. When the two differ, PostgreSQL
converts using the session TimeZone at runtime, so the two int64s are not on one scale and
hi - lo can come out short by up to the UTC offset. Short range means short bound. I did
not measure this, and the harness pins timezone='UTC', which would hide it. Requiring
c->consttype == tsvar->vartype costs nothing.

One thing I did not re-verify

I did not reproduce G3. Your bit-identical claim for the plain-column key is the right
detector and I have no reason to doubt it, but it is not something I checked, so do not read
this review as confirming it.

[jdatcmd]

You broke the premise the whole argument rests on, and you are right.

f->funcid was never checked. The match was on shape alone, which date_trunc has and so
does any user function declared (text, timestamp). I wrote "a truncating time function
over a single Var" in the comment and then implemented "a function that looks like one".

Reproduced your finding and the fix on the same fixture:

group key	actual	estimate before	estimate after
date_trunc('minute',ts)	720	722	722
hi_card('minute',ts)	499,999	722	499,900

722 against 499,999. And the part that makes it worse than a missed optimisation: core had
it very nearly right at 499,900 and my substitution replaced a good estimate with a bad
one, in the under-estimating direction, which is the one I wrote three paragraphs about
avoiding.

Now matched on F_DATE_TRUNC_TEXT_TIMESTAMP, F_DATE_TRUNC_TEXT_TIMESTAMPTZ and the
three-argument timezone form. Identical oids on 15 through 19, checked.

The interval form is excluded deliberately, and I want that on the record rather than
looking like an oversight: date_trunc(text, interval) takes a duration, not a point in
time, so a range over it does not describe a scanned window.

Your lookalike is now a permanent guard in native_groupagg.sh, kept in PL/pgSQL for the
reason you gave: an SQL function would be inlined and lose the shape that made this
reachable. 72 checks.

On the 16 percent

Noted, and I agree with your reading that it is "the plan this estimate wins" against "the
plan it displaces" rather than a clean A/B, since the off arm disables the vectorized
aggregate entirely. With the funcid check in place that comparison no longer arises for
hi_card, because we no longer touch its estimate at all.

That leaves the measured G1/G2 case as the only one the bound changes, which is what I
should have scoped it to from the start.

[ChronicallyJD]

The funcid fix is right, and putting the lookalike in native_groupagg.sh in PL/pgSQL is
better than what I asked for. Excluding the interval form is correct and I am glad it is on the
record rather than inferred.

The second one was not fixed, and I said it was unmeasured, so here it is measured

c->consttype may still be TIMESTAMPOID or TIMESTAMPTZOID regardless of tsvar->vartype,
and DatumGetTimestamp reads it either way. Both are int64 microseconds, on different scales:
a naive timestamp is wall clock, a timestamptz is UTC. PostgreSQL has cross-type operators
for these, so a mixed predicate keeps a bare Const and reaches your loop instead of being
wrapped in a cast.

On your fixed branch 67278cb, pg18a, timezone='Asia/Kolkata' (+05:30), one row per minute:

predicate	actual groups	estimate
ts >= timestamp '...00:00' AND ts < timestamptz '...12:00+00'	1050	722	under by 328
both bounds timestamptz (control)	1050	1050	sound

The planner kept it as a bare Const, which is the step the concern depends on:

Filter: ((ev.ts >= '2024-01-01 00:00:00'::timestamp without time zone)
     AND (ev.ts <  '2024-01-01 17:30:00+05:30'::timestamp with time zone))

The gap is not approximately right, it is exactly right. +05:30 is 330 minutes, your slack is
+2, and the shortfall is 328. That is the fingerprint of the mechanism rather than a
correlation: the raw timestamp datum reads as its wall-clock value, so hi - lo comes out
330 minutes short of the window the scan actually reads, and the bound lands 328 under a value
it is documented to be an upper bound on.

Same direction as the funcid bug, and same reason it matters more than a missed optimisation:
it replaces a correct estimate with a low one.

One line, as before:

if (c->constisnull || c->consttype != tsvar->vartype)
    continue;

With that, the mixed predicate contributes no bound, havelo stays false, and the estimate
falls back to core. Which is the right answer for a window this function cannot measure.

I am leaving the review at changes-requested for that one line only. Everything else here I
would merge.

[jdatcmd]

Fixed. And my first attempt to verify it could not have told the difference.

You measured the case I listed as untested in the PR description, which is the right thing
to do with a list like that.

c->consttype == tsvar->vartype now, so a mixed predicate gets no bound. Converting
between the scales is possible and not worth it: no bound is always safe, and this is an
optimisation.

The part worth reporting is my instrument, not the fix

My first verification used the 720-row fixture from the issue. Result:

MIX    actual=389  estimate=395   upper bound OK

Which looks like a pass. It is not evidence of anything: at 720 rows core's own estimate is
already near the bound, so "no bound applied" and "bound applied correctly" produce the
same number. I could not have distinguished a working fix from a broken one.

With a fixture where core's estimate is six figures:

MIX    actual=390  estimate=274949   (core's estimate, no bound)
SAME   actual=720  estimate=722      (bound applied)

Now the two outcomes cannot be confused. Both guards are permanent in
native_groupagg.sh with the discriminating fixture, not the small one. 74 checks.

That is the third time today I have caught myself with a check that would have passed
either way, and the first two were caught by you. It is the same failure as check "" "":
the check ran, and it was not the check I thought.

[ChronicallyJD]

Verified on 4020b45, same fixture and box as the finding, with a premise check that the guard
is actually in the tree I built:

predicate	actual	estimate before	estimate now
mixed timestamp / timestamptz bounds	1050	722	1050
both timestamptz (control)	1050	1050	1050

The underestimate is gone. Approving.

One caveat on my instrument, by your own argument. My fixture has 1050 rows with 1050
distinct minutes, so core's estimate and a correctly-applied bound both land near 1050. It
proves the 722 is gone; it does not discriminate "no bound applied" from "bound applied
correctly". That is the same weakness you just described in your 720-row attempt, and I did not
notice it in mine until I read your comment. Your discriminating fixture in native_groupagg.sh
is what actually covers this, not my probe.

Agreed on not converting between the scales. No bound is always safe and this is an
optimisation, so declining to guess at a timezone is the right call.

On the tally

Three today, and the pattern is the same in both directions: the defects were in claims that had
been written down and not run, and the near-misses were checks that would have passed either
way. Mine included. PGC_SKIP_TIMING was documented in a comment and wired to nothing, and I
shipped it.

What seems to actually work is not being more careful. It is that the person who did not write
the claim is the one who tests it.