Re: [HACKERS] user-defined numeric data types triggering ERROR:unsupported type

On 09/21/2017 04:24 PM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> [ scalarineqsel may fall over when used by extension operators ]
> 
> I concur with your thought that we could have
> ineq_histogram_selectivity fall back to a "mid bucket" default if
> it's working with a datatype it is unable to convert_to_scalar. But I
> think if we're going to touch this at all, we ought to have higher
> ambition than that, and try to provide a mechanism whereby an
> extension that's willing to work a bit harder could get that
> additional increment of estimation accuracy. I don't care for this
> way to do that:
> 

The question is - do we need a solution that is back-patchable? This
means we can't really use FIXEDDECIMAL without writing effectively
copying a lot of the selfuncs.c stuff, only to make some minor fixes.

What about using two-pronged approach:

1) fall back to mid bucket in back branches (9.3 - 10)
2) do something smarter (along the lines you outlined) in PG11

I'm willing to spend some time on both, but (2) alone is not a
particularly attractive for us as it only helps PG11 and we'll have to
do the copy-paste evil anyway to get the data type working on back branches.

>> * Make convert_numeric_to_scalar smarter, so that it checks if
>> there is an implicit cast to numeric, and fail only if it does not
>> find one.
> 
> because it's expensive, and it only works for numeric-category cases,
> and it will fail outright for numbers outside the range of "double".
> (Notice that convert_numeric_to_scalar is *not* calling the regular
> cast function for numeric-to-double.) Moreover, any operator ought to
> know what types it can accept; we shouldn't have to do more catalog
> lookups to figure out what to do.
> 

Ah, I see. I haven't realized it's not using the regular cast functions,
but now that you point that out it's clear relying on casts would fail.

> Now that scalarltsel and friends are just trivial wrappers for a
> common function, we could imagine exposing scalarineqsel_wrapper as a
> non-static function, with more arguments (and perhaps a better-chosen
> name ;-)). The idea would be for extensions that want to go this
> extra mile to provide their own selectivity estimation functions,
> which again would just be trivial wrappers for the core function, but
> would provide additional knowledge through additional arguments.
> 
> The additional arguments I'm envisioning are a couple of C function 
> pointers, one function that knows how to convert the operator's 
> left-hand input type to scalar, and one function that knows how to
> convert the right-hand type to scalar. (Identical APIs of course.) 
> Passing a NULL would imply that the core code must fall back on its 
> own devices for that input.
> 
> Now the thing about convert_to_scalar is that there are several
> different conversion conventions already (numeric, string, timestamp,
> ...), and there probably could be more once extension types are
> coming to the party. So I'm imagining that the API for these
> conversion functions could be something like
> 
>     bool convert(Datum value, Oid valuetypeid,
>                  int *conversion_convention, double *converted_value);
> 
> The conversion_convention output would make use of some agreed-on 
> constants, say 1 for numeric, 2 for string, etc etc. In the core 
> code, if either converter fails (returns false) or if they don't 
> return the same conversion_convention code, we give up and use the 
> mid-bucket default. If they both produce results with the same 
> conversion_convention, then we can treat the converted_values as 
> commensurable.
> 

OK, so instead of re-implementing the whole function, we'd essentially
do just something like this:

#typedef bool (*convert_callback) (Datum value, Oid valuetypeid, \                                  int
*conversion_convention,\                                  double *converted_value);
 

double
scalarineqsel(PlannerInfo *root, Oid operator,                bool isgt, bool iseq, VariableStatData *vardata,
     Datum constval, Oid consttype,                convert_calback convert);
 

and then, from the extension

double
scalarineqsel_wrapper(PlannerInfo *root, Oid operator,                bool isgt, bool iseq, VariableStatData *vardata,
             Datum constval, Oid consttype)
 
{   return scalarineqsel(root, operator, isgt, iseq, vardata,                constval, consttype, my_convert_func);
}

Sounds reasonable to me, I guess - I can't really think about anything
simpler giving us the same flexibility.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers