Re: SLOPE - Planner optimizations on monotonic expressions.

Hello,

> I also realized one more timestamp issue: timestamptz -> timestamp /
> date is not monotonic at a DST change:

I had previously dropped the support to the timezone dependent functions.
This patchset has one additional patch where that is fixed.

I moved the prosupport definitions in 0004 from utils/adt/misc.c to
utils/fmgr/slopesupport.c, I hope it is for the better, as pg_proc ends up
in fmgr. This revealed implicit dependencies in nodes/plannodes.h
and nodes/pathnodes.h.

The new patch 0006 touches a few files that are out of the backend.
It was tempting to include pgtz, but if it was not done before I assume
there is a reason. I ended up using pgtime.h for shared declarations
and it worked, but let me know if you think that is not the right one.

I wrote the tests doing with some processing, on a nearly self-checking
test that shows the results like this

+ tz_name | monotonic date_trunc units | timezone
+--------------------+----------------------------------------+----------
+ Africa/Ouagadougou | year, month, day, hour, minute, second | t
+ Europe/London | year, month, day, hour | f
+ Antarctica/Troll | year, month, day | f
+ Pacific/Guam | year, month | f
+ America/Goose_Bay | year | f

Or

+SET timezone = 'Europe/London';
+EXECUTE query;
+ date | monotonic date_turnc units
+------+----------------------------
+ t | year month day hour

When writing the patch header in preparation to send I noticed that I
didn't test
timestamptz_at_local_slope_support but it is late here and will leave
it for the next version :)

Regards,
Alexandre

On Wed, Aug 5, 2026 at 7:09 PM Alexandre Felipe <
o(dot)alexandre(dot)felipe(at)gmail(dot)com> wrote:

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