dblink: add polymorphic functions.

On Fri, Feb 20, 2015 at 7:06 AM, Corey Huinker <corey.huinker@gmail.com> wrote:

Proposed patch attached.

At quick glance, this patch lacks two things:
- regression test coverage
- documentation
(Note: do not forget to add your name in the field "Author" when
adding a new patch in the CF app).
--
Michael

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

Thanks - completely new to this process, so I'm going to need
walking-through of it. I promise to document what I learn and try to add
that to the commitfest wiki. Where can I go for guidance about
documentation format and regression tests?

Author field is presently being finicky, reported that to Magnus already.

On Thu, Feb 19, 2015 at 11:00 PM, Michael Paquier <michael.paquier@gmail.com

On Fri, Feb 20, 2015 at 2:50 PM, Corey Huinker <corey.huinker@gmail.com> wrote:

Thanks - completely new to this process, so I'm going to need
walking-through of it. I promise to document what I learn and try to add
that to the commitfest wiki. Where can I go for guidance about documentation
format and regression tests?

Here are some guidelines about how to submit a patch:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

You can have as well a look here to see how extensions like dblink are
structured:
http://www.postgresql.org/docs/devel/static/extend-pgxs.html
What you need to do in the case of your patch is to add necessary test
cases to in sql/dblink.sql for the new functions you have added and
then update the output in expected/. Be sure to not break any existing
things as well. After running the tests in your development
environment output results are available in results then pg_regress
generates a differential file in regressions.diffs.

For the documentation, updating dblink.sgml with the new function
prototypes would be sufficient. With perhaps an example(s) to show
that what you want to add is useful.
Regards,
--
Michael

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

I seem to be getting tripped up in the regression test. This line was found
in regression.diff

+ ERROR: could not stat file
"/home/ubuntu/src/postgres/contrib/dblink/tmp_check/install/usr/local/pgsql/share/extension/dblink--1.2.sql":
No such file or directory

The file dblink--1.2.sql does exist
in /home/ubuntu/src/postgres/contrib/dblink/

~/src/postgres/contrib/dblink$ ls -la dblink--1.?.sql
-rw-rw-r-- 1 ubuntu ubuntu 5.7K Feb 22 16:02 dblink--1.1.sql
-rw-rw-r-- 1 ubuntu ubuntu 6.8K Feb 22 16:50 dblink--1.2.sql

But it evidently isn't making it into the tmp_check dir.

What needs to happen for new files to be seen by the regression test
harness?

On Fri, Feb 20, 2015 at 1:14 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Mon, Feb 23, 2015 at 12:03 PM, Corey Huinker <corey.huinker@gmail.com> wrote:

+ ERROR: could not stat file
"/home/ubuntu/src/postgres/contrib/dblink/tmp_check/install/usr/local/pgsql/share/extension/dblink--1.2.sql":
No such file or directory

Didn't you forget to add dblink--1.2.sql to DATA in contrib/dblink
Makefile? That would explain why this file has not been included in
the temporary installation deployed by pg_regress.
--
Michael

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

Yes, that was it, I discovered it myself and should have posted a
"nevermind".

Now I'm slogging through figuring out where to find elog() messages from
the temporary server. It's slow, but it's progress.

On Sun, Feb 22, 2015 at 10:39 PM, Michael Paquier <michael.paquier@gmail.com

"nevermind". Found it.

On Sun, Feb 22, 2015 at 11:18 PM, Corey Huinker <corey.huinker@gmail.com>
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()

On Sun, Feb 22, 2015 at 11:38 PM, Corey Huinker <corey.huinker@gmail.com>
wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVmYTEAAoJEDfy90M199hl4tcQAJVIDiJn/ogFlxPSOxIQ1XRF
hYllqLTCALIyfDWsq5oizVrs3uFF5TpqMrFxpfpLhKbeGgGpnaOhP5rISw3DD1NA
P73MDVbP0/+Q2pwAk174+teXxqFBK3gQi4wgtaq0bC4aTC+LlphYImDbb6ExfrRR
CFlEV4MoC3vFsOKRjGalcv/iaM7HIZSn8lilmynCFx96BDwTgrmZu5vSk17a5MsO
oOc1s+1eIiZ7JpUGcYHwCmunC2Aed8OtcLjCFu3BTKTJEq1xhkbvHPFZvLBI6CtD
CI74SIHdtTg56Rm8lsJFnkg8SM9QW8kEKP/eJedS/ft5d3dFdOwYfORh+2qwmsCo
JOvtriUEVs835HGTatuh47dscwgCt0d6SA0a7rp4UxmoQTmohyt5fN8LW2fJpHd8
bj7du7SUv6n3GwxZT+PBSALkvD011shNl0/AxehOoqXHmL86G+c3qc0vHEF5ulId
jeq/ECCX509Ef+DmjFoCtnotI/oXB74JmgOqy3RtvuuZ4hgsRzzT9kyKWmcpCHQ8
ZsmwmRY5xrjClzf7dsvh8LjyM1nOAoRNQhHLlj9I8lWKupChaWaY1PH0qzAdYm9p
a1kKTV4sfuNDqV57OUk/u33lvHVx1HDyvdCNRPHz+7cyw8Zka8jAGXvzq8vZp+bV
Grl29j/8fGaErff1CNZl
=Bh5j
-----END PGP SIGNATURE-----

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

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVmdM8AAoJEDfy90M199hldowQAJ8F14qSJC9BIGdxKwRIDCS2
bzbWoynYhGiDmvffmD3e3MAUrFwh+zS3QIN5BcJVBrdLnzjZIgHFz83Z1iuH1HVw
39z0sSZVJ0C7DvPV6UiNoDpKnyAJQUu3+A5ebWKj5AYOA0AVunA7J1vSMfXAYThV
zQ6lYEpCOFq0owIyjUFpjvXUSZFs6AHEQC5wQ/UW+EXCZKl0OYQtSf8oxi8m4DFu
xRrjM+bO7LmOrBPa5fPvQOXHr5KJRjq9x7CfU+a8mJaJh4r1MmDRp7iLVxlMae0k
YVdmsLP9FOS+RhAdmmKHsTWiEJIFhffKWqcahBXGdOOWjzUVzih/LAL0BS2z44AU
ygVW/ORg5ua7Y4zxg4PUKbIkvhA+qRs0WUpuY+nYZoYAayP3VqsWP3VvMYb/e/Fb
nyws+/C1wC2aIQxgoF/+Whdfh4eTcFMSK9Qsc0pdMleDizz9O0qauHWzglo89x8X
t+s2zNnmZ0NtWd4PSTcMAcq59v288CoKDME+gjT7A6jthEbBnkx6SdEjh/NLhJba
dKKv99ZP0Rg5v8pFpQ/3TzdBB1UifUz3nd6ubeWNPjAdBbiW1FFD+I6booPmaxh0
EOYLBVkVgJFgaz0bWI5P6bDi55LDQlUQEfLRE3dxoNdffivNNfElOkXly4VShB4i
YlCc/0A1opGZb3iJwo4X
=8VAQ
-----END PGP SIGNATURE-----

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

On Sun, Jul 5, 2015 at 9:00 PM, Joe Conway <mail@joeconway.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.

On Mon, Jul 6, 2015 at 4:25 AM, Joe Conway <mail@joeconway.com> wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

This extension format is still incorrect, as there is no need for
dblink--1.1.sql as on the latest version of an extension needs to be
supported, and Makefile needs to be updated in consequence. Please
find attached an updated patch.

+-- fetch using anyelement, which will change the column names
 SELECT *
-FROM dblink_fetch('rmt_foo_cursor',4) AS t(a int, b text, c text[]);
- a | b |     c
----+---+------------
- 4 | e | {a4,b4,c4}
- 5 | f | {a5,b5,c5}
- 6 | g | {a6,b6,c6}
- 7 | h | {a7,b7,c7}
+FROM dblink_fetch('rmt_foo_cursor',4,null::foo) AS t;
+ f1 | f2 |     f3
+----+----+------------
+  4 | e  | {a4,b4,c4}
+  5 | f  | {a5,b5,c5}
+  6 | g  | {a6,b6,c6}
+  7 | h  | {a7,b7,c7}
 (4 rows)

Why isn't this test case not left alone? It looks like a regression to
me to not test it.
Regards,
--
Michael

On Mon, Jul 6, 2015 at 10:00 AM, Joe Conway <mail@joeconway.com> wrote:

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?

Yeah, particularly the use of first_optarg makes things harder to
follow in the code with this patch. A C wrapper has the disadvantage
to decentralize the argument checks to many places making the flow
harder to follow hence using get_fn_expr_argtype() with PG_NARGS would
be the way to go, at least to me. This way, you can easily find how
many arguments there are, and which value is assigned to which
variable before moving on to the real processing.
--
Michael

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

On Mon, Jul 6, 2015 at 4:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Jul 6, 2015 at 10:00 AM, Joe Conway <mail@joeconway.com> wrote:

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?

Yeah, particularly the use of first_optarg makes things harder to
follow in the code with this patch. A C wrapper has the disadvantage
to decentralize the argument checks to many places making the flow
harder to follow hence using get_fn_expr_argtype() with PG_NARGS would
be the way to go, at least to me. This way, you can easily find how
many arguments there are, and which value is assigned to which
variable before moving on to the real processing.

Just to be clear I mean that:
if (PG_NARGS() == 5)
{
if (get_fn_expr_argtype(fcinfo->flinfo, 1) == TYPEOID)
var = PG_GETARG_BOOL(1)
[...]
}
else if (PG_NARGS() == 4)
{
[...]
}
else
[...]
--
Michael

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

On Thu, Feb 19, 2015 at 4:06 PM, Corey Huinker <corey.huinker@gmail.com> wrote:

In the course of writing a small side project which hopefully will make its
way onto pgxn soon, I was writing functions that had a polymorphic result
set.

create function foo( p_row_type anyelement, p_param1 ...) returns setof
anyelement

Inside that function would be multiple calls to dblink() in both synchronous
and async modes. It is a requirement of the function that each query return
a result set conforming to the structure passed into p_row_type, but there
was no way for me to express that.

Unfortunately, there's no way to say

select * from dblink_get_result('connname') as <polymorphic record type>;

Instead, I had to generate the query as a string like this.

with x as (
select a.attname || ' ' || pg_catalog.format_type(a.atttypid,
a.atttypmod) as sql_text
from pg_catalog.pg_attribute a
where a.attrelid = pg_typeof(p_row_type)::text::regclass
and a.attisdropped is false
and a.attnum > 0
order by a.attnum )
select format('select * from dblink_get_result($1) as
t(%s)',string_agg(x.sql_text,','))
from x;

Moreover, I'm now executing this string dynamically, incurring reparsing and
replanning each time (and if all goes well, this would be executed many
times). Granted, I could avoid that by rewriting the stored procedure in C
and using prepared statements (not available in PL/PGSQL), but it seemed a
shame that dblink couldn't itself handle this polymorphism.

So with a little help, we were able to come up with polymorphic set
returning dblink functions.

Below is the results of the patch applied to a stock 9.4 installation.

[local]:ubuntu@dblink_test# create extension dblink;
CREATE EXTENSION
Time: 12.778 ms
[local]:ubuntu@dblink_test# \df dblink
List of functions
Schema | Name | Result data type | Argument data types |
Type
--------+--------+------------------+---------------------------------+--------
public | dblink | SETOF record | text |
normal
public | dblink | SETOF anyelement | text, anyelement |
normal
public | dblink | SETOF record | text, boolean |
normal
public | dblink | SETOF anyelement | text, boolean, anyelement |
normal
public | dblink | SETOF record | text, text |
normal
public | dblink | SETOF anyelement | text, text, anyelement |
normal
public | dblink | SETOF record | text, text, boolean |
normal
public | dblink | SETOF anyelement | text, text, boolean, anyelement |
normal
(8 rows)

sorry for the late reply. I'm a little concerned about the state of
overloading here. If I'm not mistaken, you may have introduced a
pretty serious backwards compatibility issue. Having the two
signatures (text, anyelement) and (text, boolean) will now fail
anytime (unknown, unknown) is passed, and that's a pretty common
invocation. If I'm right, quickly scanning the function list, I don't
think there's an easy solution to this issue other than adding an
alternately named call.

merlin

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/06/2015 12:39 AM, Michael Paquier wrote:

Yeah, particularly the use of first_optarg makes things harder
to follow in the code with this patch. A C wrapper has the
disadvantage to decentralize the argument checks to many places
making the flow harder to follow hence using
get_fn_expr_argtype() with PG_NARGS would be the way to go, at
least to me. This way, you can easily find how many arguments
there are, and which value is assigned to which variable before
moving on to the real processing.

Just to be clear I mean that: if (PG_NARGS() == 5) { if
(get_fn_expr_argtype(fcinfo->flinfo, 1) == TYPEOID) var =
PG_GETARG_BOOL(1) [...]

Actually, I had in mind something like:
8<---------------------
int i;
int numargs;
int *argtypes;

numargs = PG_NARGS();
argtypes = palloc(numargs * sizeof(int));
for (i = 0; i < numargs; i++)
argtypes[i] = get_fn_expr_argtype(fcinfo->flinfo, i);

if ((numargs == 4 || numargs == 5) &&
argtypes[0] == TEXTOID &&
argtypes[1] == TEXTOID &&
argtypes[2] == INT4OID &&
argtypes[3] == BOOLOID)
{
[...]
}
else if ((numargs == 3 || numargs == 4) &&
argtypes[0] == TEXTOID &&
argtypes[1] == INT4OID &&
argtypes[2] == BOOLOID)
{
[...]
8<---------------------
etc.

If the highest number of arguments is always checked first, the
pattern is not ambiguous even with the extra anyelement.

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVmovZAAoJEDfy90M199hlzDsP/j/v76Ecv3VtZ+02Lo9mUVp6
mf+ZTLgw2OE4sNI6PL2MeVzu5ayZAkHxPRAah9rR1A7nDNEyZovi3dymFRHeiJ6Z
rowjkdZiRX/xlV5s6RHrdmX6DkVkBeGcKMuIKB/Tud2uPCiuBULkcUuD1OvlxsCs
W0E+hsuYmpGtsH8Vth+ciKiBUDX/BVWCOnqZXISRf6BZ5BjzITOEuCyn4EChx2o4
9gOGPTf3P/4I3buuDuV+DEmO1N4L07VvSWb9e92NrdS3VI1ae8YJu3u248WVmdPY
+qHg0J7jLGYBFRZ+isC7p8OX7PANCm88GvMCqklZdPo+/76n4J6x5MJfjinQEfXN
rbScwRh3O1DCimw404WqYSGKGEcX7MtUt98h+//nMft3aEz1gRnsuopnM/eRmQcS
wYlbBYon5YEdamx2o5AP6NX5zFU+6HBcKcznCFQtcsaJeh03yz9zILuCWxg4dWNj
T5aVCYu58PN4ELKP3yF5wN2UUSE4/OZJHgvIrHF8LiDOSdygvfADdUAmfD0sry48
tjbL9GC7JwHxqQ8qYktDMogxZo+s3TBsmw3NsnYXrNYwObbGCP9J0K0zV76ukMEc
V1qiMXD4/gddkKXJz0cVfcActrDqEltKDPTCdhLpoc4Gb59mrohgk+j75f2af14V
+n/AvaymgwdKjQpBhaTb
=tESF
-----END PGP SIGNATURE-----

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

Joe Conway <mail@joeconway.com> writes:

Actually, I had in mind something like:
8<---------------------
int i;
int numargs;
int *argtypes;

numargs = PG_NARGS();
argtypes = palloc(numargs * sizeof(int));
for (i = 0; i < numargs; i++)
argtypes[i] = get_fn_expr_argtype(fcinfo->flinfo, i);

if ((numargs == 4 || numargs == 5) &&
argtypes[0] == TEXTOID &&
argtypes[1] == TEXTOID &&
argtypes[2] == INT4OID &&
argtypes[3] == BOOLOID)
{
[...]
}
else if ((numargs == 3 || numargs == 4) &&
argtypes[0] == TEXTOID &&
argtypes[1] == INT4OID &&
argtypes[2] == BOOLOID)
{
[...]
8<---------------------
etc.

If the set of allowed argument-type combinations is so easily enumerable,
I don't understand why this is being done at all. Create a separate SQL
function for each combination. You can still let the called C functions
call a common implementation routine if that's helpful.

However, this might all be moot in view of Merlin's objection. It is
definitely completely uncool to have both of these:

public | dblink | SETOF anyelement | text, anyelement | normal
public | dblink | SETOF record | text, boolean | normal

It's quite unclear which one will get called for cases like, say, second
argument is a domain over boolean. And even if the second arg is just a
boolean, maybe the user wanted the first case --- how will he get that
behavior, if so? These need to have different names, and that might well
help resolve the implementation-level issue...

regards, tom lane

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

On Mon, Jul 6, 2015 at 9:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Joe Conway <mail@joeconway.com> writes:

Actually, I had in mind something like:
8<---------------------
int i;
int numargs;
int *argtypes;

numargs = PG_NARGS();
argtypes = palloc(numargs * sizeof(int));
for (i = 0; i < numargs; i++)
argtypes[i] = get_fn_expr_argtype(fcinfo->flinfo, i);

if ((numargs == 4 || numargs == 5) &&
argtypes[0] == TEXTOID &&
argtypes[1] == TEXTOID &&
argtypes[2] == INT4OID &&
argtypes[3] == BOOLOID)
{
[...]
}
else if ((numargs == 3 || numargs == 4) &&
argtypes[0] == TEXTOID &&
argtypes[1] == INT4OID &&
argtypes[2] == BOOLOID)
{
[...]
8<---------------------
etc.

If the set of allowed argument-type combinations is so easily enumerable,
I don't understand why this is being done at all. Create a separate SQL
function for each combination. You can still let the called C functions
call a common implementation routine if that's helpful.

However, this might all be moot in view of Merlin's objection. It is
definitely completely uncool to have both of these:

public | dblink | SETOF anyelement | text, anyelement | normal
public | dblink | SETOF record | text, boolean | normal

It's quite unclear which one will get called for cases like, say, second
argument is a domain over boolean. And even if the second arg is just a
boolean, maybe the user wanted the first case --- how will he get that
behavior, if so? These need to have different names, and that might well
help resolve the implementation-level issue...

yup, and at least one case now fails where previously it ran through:
postgres=# select * from dblink('a', 'b', 'c');
ERROR: function dblink(unknown, unknown, unknown) is not unique

merlin

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

On Mon, Jul 6, 2015 at 10:08 AM, Joe Conway <mail@joeconway.com> wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/06/2015 12:39 AM, Michael Paquier wrote:

Yeah, particularly the use of first_optarg makes things harder
to follow in the code with this patch. A C wrapper has the
disadvantage to decentralize the argument checks to many places
making the flow harder to follow hence using
get_fn_expr_argtype() with PG_NARGS would be the way to go, at
least to me. This way, you can easily find how many arguments
there are, and which value is assigned to which variable before
moving on to the real processing.

Just to be clear I mean that: if (PG_NARGS() == 5) { if
(get_fn_expr_argtype(fcinfo->flinfo, 1) == TYPEOID) var =
PG_GETARG_BOOL(1) [...]

Actually, I had in mind something like:
8<---------------------
int i;
int numargs;
int *argtypes;

numargs = PG_NARGS();
argtypes = palloc(numargs * sizeof(int));
for (i = 0; i < numargs; i++)
argtypes[i] = get_fn_expr_argtype(fcinfo->flinfo, i);

if ((numargs == 4 || numargs == 5) &&
argtypes[0] == TEXTOID &&
argtypes[1] == TEXTOID &&
argtypes[2] == INT4OID &&
argtypes[3] == BOOLOID)
{
[...]
}
else if ((numargs == 3 || numargs == 4) &&
argtypes[0] == TEXTOID &&
argtypes[1] == INT4OID &&
argtypes[2] == BOOLOID)
{
[...]
8<---------------------
etc.

If the highest number of arguments is always checked first, the
pattern is not ambiguous even with the extra anyelement.

Utterly reasonable and do-able. I'll get to work on that soonish, pending
resolution of other's concerns.