Re: dblink: add polymorphic functions.

On Sun, Jul 5, 2015 at 9:00 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/05/2015 12:25 PM, Joe Conway wrote:
> > On 02/22/2015 10:26 PM, Corey Huinker wrote:
> >> Changes in this patch: - added polymorphic versions of
> >> dblink_fetch() - upped dblink version # to 1.2 because of new
> >> functions - migration 1.1 -> 1.2 - DocBook changes for dblink(),
> >> dblink_get_result(), dblink_fetch()
> >
> > The previous patch was missing dblink--1.1--1.2.sql and
> > dblink--1.2.sql. I have added them, so it should apply cleanly
> > against git master, but not done any actual review yet.
>
> Looking at the argument handling logic in dblink_fetch() and
> dblink_record_internal(), it has been messy for a while, but this
> patch serves to make it even worse.
>
> I wonder if it isn't better to just loop through all the args with
> get_fn_expr_argtype() every time and then test for the exact signature
> match? Another alternative might be to create a wrapper C function for
> each variant SQL function, but that seems like it could also get
> messy, especially with dblink_record_internal() since we would have to
> deal with every variant of all the external facing callers.
>
> Thoughts?
>

When I was digging into it, it seemed that the parameter sniffing was on
the verge of combinatorical chaos, which I assumed was due to parameter
checking being expensive.
And while there was no processing for the anyelement parameters, it did
throw off the parameter counts, thus the extra complexity.
If it's possible to suss out whether the last parameter is the polymorphic
type, then we could bump down the arg-count by one, and have the old messy
logic tree.

I also didn't delve into whether or not it was dangerous to ask
get_fn_expr_argtype() for parameters beyond PG_NARGS(). If it is, then we
would still have the complicated signature-testing tree, but at least we
could separate out the signature poking from the actual processing, and
that might add some clarity.

We could walk the arglist and handle each arg as you go, but that gets
complicated with the text params because their meaning is often determined
by whether the next param is also text. That has the potential to be every
bit as messy.

So from this, I see five options:

1. Leave it.
It's a mess, but we have test case coverage to show it works.

2. A bloody minded approach
if ((PG_NARGS() == 3) && (get_fn_expr_argtype(...,0) == TEXTOID) &&
(get_fn_expr_argtype(...,1) == TEXTOID) && (get_fn_expr_argtype(...,2) ==
BOOLOID))
{
}
else if ((PG_NARGS() == 3) && (get_fn_expr_argtype(...,0) == TEXTOID)
&& (get_fn_expr_argtype(...,1) == BOOLOID) && (get_fn_expr_argtype(...,2)
== WHATEVER_ANYELEMENTS_ARE_OID))
{
}
else if ...

Pro:
It's utterly clear which signature was found
Con:
A lot of param checks, though most will shortcircuit
The number of checks itself might become clutter.

3. Separate signature testing from using the parameters.
if (PG_NARGS() == 3)
{
if (get_fn_expr_argtype(fcinfo->flinfo, 2) == BOOLOID)
{
signature = TEXTTEXTBOOL
}
else if (get_fn_expr_argtype(fcinfo->flinfo, 1) == BOOLOID)
{
signature = TEXTBOOLANY
}
else
{
signature = TEXTTEXTANY
}
}

switch(signature)
{
case TEXTTEXTBOOL:
t1 = text_to_cstring(PG_GETARG_TEXT_PP(0);
t2 = text_to_cstring(PG_GETARG_TEXT_PP(1);
fail = PG_GETARG_BOOL(2);
break;

Pro:
Still getting to the signature with a minimum of comparisons.
Con:
A fair number of #defines for each signature combination (minus
anyelements, as we never have to process those).
Big(?) waterfall of options.
The mess has just been segregated, not eliminated.

4. Walk the arglist, assigning as you go, on the assumption that if the
current arg is text field it will be the last one encountered, and
re-assigning the pointers when we discover otherwise.

Pro:
A tight loop, probably the least amount of code.
I *think* this is what Joe was suggesting earlier.
Con:
We just made a mini-state engine, sorta like what you have to do
with event driven parsers (XML SAX, for example).

5. Walk the arglist like in #3, assigning bool/int parameters immediately,
but just count the text parameters, and assign them based on how many we
found.

Pro:
Same as #4, but my high school CS teacher won't come out of
retirement just to scold me for repurposing variables.
Con:
If at some point in the future we add more text parameters such
that simple ordering is ambiguous (i.e. were those text fields at positions
1,2,3 or 1,2,5?) this would break down.

I'm happy to try to code whichever of these people think is the most
promising.