Re: SLOPE - Planner optimizations on monotonic expressions.

Fix uncommitted slope_catalog.out in 0006

On Mon, Aug 24, 2026 at 9:19 AM Alexandre Felipe <
o(dot)alexandre(dot)felipe(at)gmail(dot)com> wrote:

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