Re: SLOPE - Planner optimizations on monotonic expressions.

Sorry for the basic bug in v13.

The slope logic was incorrectly breaking the outer loop
in build_index_pathkeys. v14 fixes that and renders the if block
by removing the
+ if (slope_match)
+ {
+ if (query_pk_cell == NULL)
+ break;

query_pk_cell is initialised with null making the inner loop
stop condition sufficient.

When an incompatible pathkey is detected skipping the inner loop
+ /*
+ * Index can't satisfy query pathkeys any further
+ */
+ query_pk_cell = NULL;
+ break;

Excluding the inner loop
we get

- if (cpathkey)
+ if(cpathkey && !pathkey_emitted)

with pathkey_emitted = false

- else
+ if(!column_pinned)

with column_pinned set to true inside the if clause block.

On Mon, Jul 27, 2026 at 8:00 AM Alexandre Felipe <
o(dot)alexandre(dot)felipe(at)gmail(dot)com> wrote:

>
> 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
>