Re: SLOPE - Planner optimizations on monotonic expressions.

Fixing warnings from v15

On Sun, Aug 23, 2026 at 10:14 PM Alexandre Felipe <
o(dot)alexandre(dot)felipe(at)gmail(dot)com> wrote:

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