Re: SLOPE - Planner optimizations on monotonic expressions.

Thank you for your thorough review Zsolt

On Tue, Jul 21, 2026 at 11:27 PM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>
wrote:

> Hello
>
> > Removed prosupport for timestamptz <-> timestamp.
>
> This is not fixed, same testcase still fails. Maybe because casts are
> skipped unconditionally?
>
> + if ((fexpr->funcformat == COERCE_IMPLICIT_CAST ||
> + fexpr->funcformat ==
> COERCE_EXPLICIT_CAST) &&
> + list_length(fexpr->args) == 1)
> + {
> + expr = (Expr *) linitial(fexpr->args);
> + continue;
> + }
>
> And it seems like we have more issues with casts, there are some more
> cases: timestamptz -> date (I mentioned this in my previous email),
> timestamp -> time, int4 -> bool (-1 -> true, 0 -> false, 1 -> true)
>

Well explicit casts should not be there. I thought implicit casts would
be fine. But checking pg_cast with
SELECT
oid,
castsource::regtype, casttarget::regtype,
castfunc::regproc,
castcontext, castmethod
FROM pg_cast

I see character varying => regclass, I didn't see any other such conversion.
Making it explicit now would break a few, if not many, DBA scripts.

*timestamptz -> date
I was optimistic here but you are right again. I checked tzinfo (using
python
and tzdump). From 596 time zones there are 7 for which the date moved
backward at some point.

1 America/Goose_Bay 00:00:59 -> 22:01:00, 1988
23 America/Goose_Bay 00:00:59 -> 23:01:00, 1987-2010
1 Canada/Newfoundland 00:00:59 -> 22:01:00, 1988
23 Canada/Newfoundland 00:00:59 -> 23:01:00, 1987-2010
1 America/St_Johns 00:00:59 -> 22:01:00, 1988
23 America/St_Johns 00:00:59 -> 23:01:00, 1987-2010
14 America/Moncton 00:00:59 -> 23:01:00, 1993-2006
1 Antarctica/Casey 01:59:59 -> 23:00:00, 2010-2010
1 Pacific/Guam 00:00:59 -> 23:01:00, 1969
1 Pacific/Saipan 00:00:59 -> 23:01:00, 1969

I rechecked another old issue, and realized that this was dropped
> somewhere around v7, was that intentional?
> > v5 aims to prevent the elimination of the sort node if the index has a
> > custom sort operator family.
>

OK, that got lost in some refactoring, apparently I lost it squeezing
some commits on v7.2 and v7.3, the v7 and then the dropping the code
on v7.14 wasn't detected. What I submitted was v7.20, now I added that part
just after get_slope_wrt

I found one more problem with binary coercible types, the skip there
> is too generic, it should be more restrictive (btree opfamilies
> maybe?):
>
> + /* Skip RelabelType (no-op coercion) */
> + if (IsA(expr, RelabelType))
> + {
> + expr = (Expr *) ((RelabelType *) expr)->arg;
> + continue;
> + }

I think my assumptions (guesses) about RelabelType were completely
incorrect. From your feedback I thought of writing something like this

+ argtype = getBaseType(exprType((Node *) ((RelabelType *) expr)->arg));
+ restype = getBaseType(((RelabelType *) expr)->resulttype);
+ expr = (Expr *) ((RelabelType *) expr)->arg;
+ if (argtype == restype)
+ continue;
+
+ arg_opclass = GetDefaultOpClass(argtype, BTREE_AM_OID);
+ res_opclass = GetDefaultOpClass(restype, BTREE_AM_OID);
+ if (arg_opclass == res_opclass)
+ continue;
+
+ if(OidIsValid(arg_opclass))
+ arg_opfamily = get_opclass_family(arg_opclass);
+
+ if(OidIsValid(res_opclass))
+ res_opfamily = get_opclass_family(res_opclass);
+
+ if (arg_opfamily == res_opfamily)
+ continue;
+
+ return MONOTONICFUNC_NONE;

But then, checking the catalog to see what could match

SELECT opfname, array_agg(oc.opcintype::regtype)
FROM pg_opclass oc
JOIN pg_am am ON am.oid = oc.opcmethod
JOIN pg_opfamily of ON of.oid = oc.opcfamily
WHERE oc.opcdefault
AND am.amname = 'btree'
GROUP BY 1
HAVING count(distinct oc.opcintype) > 1

+------------+----------------------------+
|opfname |array_agg |
+------------+----------------------------+
|datetime_ops|{date,timestamp,timestamptz}|
|float_ops |{real,"double precision"} |
|integer_ops |{bigint,smallint,integer} |
|text_ops |{name,text} |
+------------+----------------------------+

Since we are scoping to default opfamilies I am stopping at
RelabelTypes, declaring it non-monotonic.

+ /*
> + * Case 2: f(x) after x —
> ascending chain. x is already
> + * in retval, so within each group
> of equal x values, f(x)
> + * is constant (for any
> deterministic f). The pathkey is
> + * redundant as a tiebreaker
> regardless of monotonicity.
> + */
> + if (!pathkey_is_redundant(qpk,
> retval))
> + retval = lappend(retval,
> qpk);
> + continue;
>
> "within each group of equal x values, f(x) is constant (for any
> deterministic f)" -- doesn't this also require equalimage?
>

Yes it is required, thank you for educating me about that, I wasn't aware
of this feature. x, f(x) pattern is very broad, and matches things beyond
intended.

There are also a few typos:

disables
>
Fixed.

> takes
>
Fixed

> inferred
>
Fixed

> x1 and x2
>
Fixed

> + * 'nslopes' points to a MonotonicFunction array (one per argument up
> to
> + * nslopes). Arguments beyond nslopes are treated as
> MONOTONICFUNC_NONE.
>
> slopes points to
>
Fixed

> +
> +typedef enum NUMERIC_SIGN
> +{
>
> Also nitpick, but numeric.c has a define with the same name. There's
> no conflict as it's in a different file and that's a macro, but could
> make searching more difficult.

Renamed to SLOPE_NUMERIC.

I also merged the 'redundancy checks' patch in the 'planner support' patch
and I changed that to use chasing pointers instead of nested loops.
doing (max(index columns, query pathkeys)) iterations instead
of ((index columns) * (query pathkeys).

I have applying and cleaning my work tree so many times that is better
to submit this before I lose something important :)

Regards,
Alexandre