Re: plperl features

Patch provide support for array type and pseudo type
(anyelement, anyarray) for function parameters and result.
for example:

CREATE FUNCTION add_three_values(anyelement, anyelement, anyelement)
RETURNS anyelement AS '
return $_[0]+$_[1]+$_[2];
' LANGUAGE plperl;

CREATE FUNCTION make_array(anyelement, anyelement, anyelement) RETURNS
anyarray AS '
return [$_[0], $_[1], $_[2]];
' LANGUAGE plperl;

Comments?

--ggRay

On Wed, Sep 29, 2004 at 07:13:47PM +0300, Sergej Sergeev wrote:
> Patch provide support for array type and pseudo type
> (anyelement, anyarray) for function parameters and result.
> for example:
>
> CREATE FUNCTION add_three_values(anyelement, anyelement, anyelement)
> RETURNS anyelement AS '
> return $_[0]+$_[1]+$_[2];
> ' LANGUAGE plperl;
>
> CREATE FUNCTION make_array(anyelement, anyelement, anyelement) RETURNS
> anyarray AS '
> return [$_[0], $_[1], $_[2]];
> ' LANGUAGE plperl;
>
> Comments?

What happens if you feed other pseudotypes, like cstring or
language_handler? Shouldn't that be disallowed or something?

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Java is clearly an example of a money oriented programming" (A. Stepanov)

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> What happens if you feed other pseudotypes, like cstring or
> language_handler? Shouldn't that be disallowed or something?

Indeed. This patch breaks those defenses...

regards, tom lane

Alvaro Herrera wrote:

>On Wed, Sep 29, 2004 at 07:13:47PM +0300, Sergej Sergeev wrote:
>
>
>>Patch provide support for array type and pseudo type
>>(anyelement, anyarray) for function parameters and result.
>>for example:
>>
>>CREATE FUNCTION add_three_values(anyelement, anyelement, anyelement)
>>RETURNS anyelement AS '
>>return $_[0]+$_[1]+$_[2];
>>' LANGUAGE plperl;
>>
>>CREATE FUNCTION make_array(anyelement, anyelement, anyelement) RETURNS
>>anyarray AS '
>>return [$_[0], $_[1], $_[2]];
>>' LANGUAGE plperl;
>>
>>Comments?
>>
>>
>
>What happens if you feed other pseudotypes, like cstring or
>language_handler? Shouldn't that be disallowed or something?
>
>
>
Other pseudo-types are disallowed (no-change)

---
g.gRay: PL/perl, PL/PHP ;)

Sergej Sergeev <sergej(at)commandprompt(dot)com> writes:
>> What happens if you feed other pseudotypes, like cstring or
>> language_handler? Shouldn't that be disallowed or something?

> Other pseudo-types are disallowed (no-change)

No, because you diked out the check at lines 1452ff, rather than
upgrading it to something correct.

I find the "fn_retispseudo" and "arg_is_p" flags pretty bogus anyway
since they fail to indicate *which* pseudotype it is. You might as
well just test for the specific type OID.

regards, tom lane

>Sergej Sergeev <sergej(at)commandprompt(dot)com> writes:
>
>
>>>What happens if you feed other pseudotypes, like cstring or
>>>language_handler? Shouldn't that be disallowed or something?
>>>
>>>
>
>
>
>>Other pseudo-types are disallowed (no-change)
>>
>>
>
>No, because you diked out the check at lines 1452ff, rather than
>upgrading it to something correct.
>
>I find the "fn_retispseudo" and "arg_is_p" flags pretty bogus anyway
>since they fail to indicate *which* pseudotype it is. You might as
>well just test for the specific type OID.
>
> regards, tom lane
>
>
New patch. I have added the check pseudo-type argumetns.
Specific type is substituted in runtime, during function call.

--
g.gRay: PL/perl, PL/PHP ;)

any comments on my last patch?

--
g.gRay: PL/perl, PL/PHP ;)

Sergej Sergeev wrote:

> any comments on my last patch?
>

Speaking for myself, I am not really thinking much about new plperl
stuff until we get closer to branching the code, which as I understand
the current processes would be at the time we release an RC - that seems
a little way off yet.

cheers

andrew

I assume this is an 8.0 fix.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Sergej Sergeev wrote:
>
> >Sergej Sergeev <sergej(at)commandprompt(dot)com> writes:
> >
> >
> >>>What happens if you feed other pseudotypes, like cstring or
> >>>language_handler? Shouldn't that be disallowed or something?
> >>>
> >>>
> >
> >
> >
> >>Other pseudo-types are disallowed (no-change)
> >>
> >>
> >
> >No, because you diked out the check at lines 1452ff, rather than
> >upgrading it to something correct.
> >
> >I find the "fn_retispseudo" and "arg_is_p" flags pretty bogus anyway
> >since they fail to indicate *which* pseudotype it is. You might as
> >well just test for the specific type OID.
> >
> > regards, tom lane
> >
> >
> New patch. I have added the check pseudo-type argumetns.
> Specific type is substituted in runtime, during function call.
>
> --
> g.gRay: PL/perl, PL/PHP ;)
>

> Index: plperl.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/pl/plperl/plperl.c,v
> retrieving revision 1.51
> diff -c -w -r1.51 plperl.c
> *** plperl.c 13 Sep 2004 20:08:59 -0000 1.51
> --- plperl.c 30 Sep 2004 16:07:25 -0000
> ***************
> *** 82,87 ****
> --- 82,89 ----
> bool lanpltrusted;
> bool fn_retistuple; /* true, if function returns tuple */
> bool fn_retisset; /* true, if function returns set */
> + bool fn_retisarray; /* true, if function returns "true" array*/
> + bool fn_retispseudo; /* true, if function returns pseudo type*/
> Oid ret_oid; /* Oid of returning type */
> FmgrInfo result_in_func;
> Oid result_typioparam;
> ***************
> *** 89,94 ****
> --- 91,97 ----
> FmgrInfo arg_out_func[FUNC_MAX_ARGS];
> Oid arg_typioparam[FUNC_MAX_ARGS];
> bool arg_is_rowtype[FUNC_MAX_ARGS];
> + bool arg_is_p[FUNC_MAX_ARGS];
> SV *reference;
> } plperl_proc_desc;
>
> ***************
> *** 277,282 ****
> --- 280,319 ----
> }
>
> /**********************************************************************
> + * convert perl array to the string representation
> + **********************************************************************/
> + static SV*
> + plperl_convert_to_pg_array(SV *src)
> + {
> + SV* rv;
> + SV** val;;
> + AV* internal;
> + int len,
> + i;
> +
> + internal=(AV*)SvRV(src);
> + len = av_len(internal)+1;
> +
> + rv = newSVpv("{ ",0);
> + for(i=0; i<len; i++)
> + {
> + val = av_fetch(internal, i, FALSE);
> + if (SvTYPE(*val)==SVt_RV)
> + if (SvTYPE(SvRV(*val))==SVt_PVAV)
> + sv_catpvf(rv, "%s", SvPV(plperl_convert_to_pg_array(*val),PL_na));
> + else
> + elog(ERROR, "plperl: check array structure");
> + else
> + sv_catpvf(rv, "%s", SvPV(*val,PL_na));
> + if (i != len-1) sv_catpvf(rv, ",");
> + }
> +
> + sv_catpvf(rv, "}");
> +
> + return rv;
> + }
> +
> + /**********************************************************************
> * turn a tuple into a hash expression and add it to a list
> **********************************************************************/
> static void
> ***************
> *** 752,757 ****
> --- 789,807 ----
> XPUSHs(sv_2mortal(newSVpv("undef", 0)));
> for (i = 0; i < desc->nargs; i++)
> {
> + if (desc->arg_is_p[i]){
> + HeapTuple typeTup;
> + Form_pg_type typeStruct;
> +
> + typeTup = SearchSysCache(TYPEOID,
> + ObjectIdGetDatum(get_fn_expr_argtype(fcinfo->flinfo, i)),
> + 0, 0, 0);
> + typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
> + perm_fmgr_info(typeStruct->typoutput, &(desc->arg_out_func[i]));
> + desc->arg_typioparam[i] = getTypeIOParam(typeTup);
> + ReleaseSysCache(typeTup);
> + }
> +
> if (desc->arg_is_rowtype[i])
> {
> if (fcinfo->argnull[i])
> ***************
> *** 909,914 ****
> --- 959,977 ----
> perlret = srf_perlret;
> }
>
> + if (prodesc->fn_retispseudo){
> + HeapTuple retTypeTup;
> + Form_pg_type retTypeStruct;
> +
> + retTypeTup = SearchSysCache(TYPEOID,
> + ObjectIdGetDatum(get_fn_expr_rettype(fcinfo->flinfo)),
> + 0, 0, 0);
> + retTypeStruct = (Form_pg_type) GETSTRUCT(retTypeTup);
> + perm_fmgr_info(retTypeStruct->typinput, &(prodesc->result_in_func));
> + prodesc->result_typioparam = getTypeIOParam(retTypeTup);
> + ReleaseSysCache(retTypeTup);
> + }
> +
> if (prodesc->fn_retisset && SRF_IS_FIRSTCALL())
> {
> if (prodesc->fn_retistuple)
> ***************
> *** 1149,1161 ****
>
> }
> else
> /* perl string to Datum */
>
> retval = FunctionCall3(&prodesc->result_in_func,
> PointerGetDatum(SvPV(perlret, PL_na)),
> ObjectIdGetDatum(prodesc->result_typioparam),
> Int32GetDatum(-1));
> !
> }
>
> SvREFCNT_dec(perlret);
> --- 1212,1234 ----
>
> }
> else
> + {
> + SV* ret;
> +
> + if (prodesc->fn_retisarray)
> + {
> + if(SvTYPE(SvRV(perlret))!=SVt_PVAV) elog(ERROR, "plperl: this function must return reference to array");
> + ret = plperl_convert_to_pg_array(perlret);
> + SvREFCNT_dec(perlret);
> + perlret = ret;
> + }
> /* perl string to Datum */
>
> retval = FunctionCall3(&prodesc->result_in_func,
> PointerGetDatum(SvPV(perlret, PL_na)),
> ObjectIdGetDatum(prodesc->result_typioparam),
> Int32GetDatum(-1));
> ! }
> }
>
> SvREFCNT_dec(perlret);
> ***************
> *** 1384,1395 ****
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype result, except VOID or RECORD */
> if (typeStruct->typtype == 'p')
> {
> if (procStruct->prorettype == VOIDOID ||
> ! procStruct->prorettype == RECORDOID)
> ! /* okay */ ;
> else if (procStruct->prorettype == TRIGGEROID)
> {
> free(prodesc->proname);
> --- 1457,1471 ----
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype result, except VOID, RECORD, ANYELEMENT or ANYARRAY */
> if (typeStruct->typtype == 'p')
> {
> if (procStruct->prorettype == VOIDOID ||
> ! procStruct->prorettype == RECORDOID ||
> ! procStruct->prorettype == ANYARRAYOID ||
> ! procStruct->prorettype == ANYELEMENTOID)
> ! /* okay */
> ! prodesc->fn_retispseudo=true;
> else if (procStruct->prorettype == TRIGGEROID)
> {
> free(prodesc->proname);
> ***************
> *** 1421,1426 ****
> --- 1497,1509 ----
> procStruct->prorettype;
> }
>
> + if (procStruct->prorettype != ANYARRAYOID)
> + if (typeStruct->typlen == -1 && typeStruct->typelem) /*true, if function returns "true" array*/
> + prodesc->fn_retisarray = true;
> + else
> + prodesc->fn_retisarray = false;
> + else prodesc->fn_retisarray = true;
> +
> perm_fmgr_info(typeStruct->typinput, &(prodesc->result_in_func));
> prodesc->result_typioparam = getTypeIOParam(typeTup);
>
> ***************
> *** 1448,1455 ****
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype argument */
> if (typeStruct->typtype == 'p')
> {
> free(prodesc->proname);
> free(prodesc);
> --- 1531,1543 ----
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype argument, except ANYELEMENT or ANYARRAY */
> if (typeStruct->typtype == 'p')
> + if (procStruct->proargtypes[i] == ANYARRAYOID ||
> + procStruct->proargtypes[i] == ANYELEMENTOID)
> + /* okay */
> + prodesc->arg_is_p[i] = true;
> + else
> {
> free(prodesc->proname);
> free(prodesc);
> ***************
> *** 1458,1463 ****
> --- 1546,1553 ----
> errmsg("plperl functions cannot take type %s",
> format_type_be(procStruct->proargtypes[i]))));
> }
> + else
> + prodesc->arg_is_p[i] = false;
>
> if (typeStruct->typtype == 'c')
> prodesc->arg_is_rowtype[i] = true;

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I assume this is an 8.0 fix.

It looks more like a new feature to me ...

regards, tom lane

Tom Lane wrote:

>Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>
>
>>I assume this is an 8.0 fix.
>>
>>
>
>It looks more like a new feature to me ...
>
>
They were requested features that we did not get done before freeze. It
would be great if we could
get them applied. We are continuing to develop plPerl and at this point,
what is going to ship with 8.0
won't even be close to what is available via the plPerl website.

We expected this to a degree of course, but if we can get some of them
in, it would be nice for the community
who wants to use plPerl.

Sincerely,

Joshua D. Drake

> regards, tom lane
>
>---------------------------(end of broadcast)---------------------------
>TIP 7: don't forget to increase your free space map settings
>
>

--
Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC
Postgresql support, programming shared hosting and dedicated hosting.
+1-503-667-4564 - jd(at)commandprompt(dot)com - http://www.commandprompt.com
PostgreSQL Replicator -- production quality replication for PostgreSQL

Joshua D. Drake wrote:
> We expected this to a degree of course, but if we can get some of
> them in, it would be nice for the community
> who wants to use plPerl.

On the other hand, it wouldn't be that nice for the community that
respects a good freeze. As you know, there will always be one more
feature.

Considering that we're already a long time into the beta phase, and
we're still working out portability issues especially in the various
plug-ins, we really ought to be strict about the freeze in that area if
we ever want to get finished.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Joshua D. Drake wrote:
> > We expected this to a degree of course, but if we can get some of
> > them in, it would be nice for the community
> > who wants to use plPerl.
>
> On the other hand, it wouldn't be that nice for the community that
> respects a good freeze. As you know, there will always be one more
> feature.
>
> Considering that we're already a long time into the beta phase, and
> we're still working out portability issues especially in the various
> plug-ins, we really ought to be strict about the freeze in that area if
> we ever want to get finished.

Agreed. This is the downside of being bundled with the server. Sorry.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

This has been saved for the 8.1 release:

http:/momjian.postgresql.org/cgi-bin/pgpatches2

---------------------------------------------------------------------------

Sergej Sergeev wrote:
>
> >Sergej Sergeev <sergej(at)commandprompt(dot)com> writes:
> >
> >
> >>>What happens if you feed other pseudotypes, like cstring or
> >>>language_handler? Shouldn't that be disallowed or something?
> >>>
> >>>
> >
> >
> >
> >>Other pseudo-types are disallowed (no-change)
> >>
> >>
> >
> >No, because you diked out the check at lines 1452ff, rather than
> >upgrading it to something correct.
> >
> >I find the "fn_retispseudo" and "arg_is_p" flags pretty bogus anyway
> >since they fail to indicate *which* pseudotype it is. You might as
> >well just test for the specific type OID.
> >
> > regards, tom lane
> >
> >
> New patch. I have added the check pseudo-type argumetns.
> Specific type is substituted in runtime, during function call.
>
> --
> g.gRay: PL/perl, PL/PHP ;)
>

> Index: plperl.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/pl/plperl/plperl.c,v
> retrieving revision 1.51
> diff -c -w -r1.51 plperl.c
> *** plperl.c 13 Sep 2004 20:08:59 -0000 1.51
> --- plperl.c 30 Sep 2004 16:07:25 -0000
> ***************
> *** 82,87 ****
> --- 82,89 ----
> bool lanpltrusted;
> bool fn_retistuple; /* true, if function returns tuple */
> bool fn_retisset; /* true, if function returns set */
> + bool fn_retisarray; /* true, if function returns "true" array*/
> + bool fn_retispseudo; /* true, if function returns pseudo type*/
> Oid ret_oid; /* Oid of returning type */
> FmgrInfo result_in_func;
> Oid result_typioparam;
> ***************
> *** 89,94 ****
> --- 91,97 ----
> FmgrInfo arg_out_func[FUNC_MAX_ARGS];
> Oid arg_typioparam[FUNC_MAX_ARGS];
> bool arg_is_rowtype[FUNC_MAX_ARGS];
> + bool arg_is_p[FUNC_MAX_ARGS];
> SV *reference;
> } plperl_proc_desc;
>
> ***************
> *** 277,282 ****
> --- 280,319 ----
> }
>
> /**********************************************************************
> + * convert perl array to the string representation
> + **********************************************************************/
> + static SV*
> + plperl_convert_to_pg_array(SV *src)
> + {
> + SV* rv;
> + SV** val;;
> + AV* internal;
> + int len,
> + i;
> +
> + internal=(AV*)SvRV(src);
> + len = av_len(internal)+1;
> +
> + rv = newSVpv("{ ",0);
> + for(i=0; i<len; i++)
> + {
> + val = av_fetch(internal, i, FALSE);
> + if (SvTYPE(*val)==SVt_RV)
> + if (SvTYPE(SvRV(*val))==SVt_PVAV)
> + sv_catpvf(rv, "%s", SvPV(plperl_convert_to_pg_array(*val),PL_na));
> + else
> + elog(ERROR, "plperl: check array structure");
> + else
> + sv_catpvf(rv, "%s", SvPV(*val,PL_na));
> + if (i != len-1) sv_catpvf(rv, ",");
> + }
> +
> + sv_catpvf(rv, "}");
> +
> + return rv;
> + }
> +
> + /**********************************************************************
> * turn a tuple into a hash expression and add it to a list
> **********************************************************************/
> static void
> ***************
> *** 752,757 ****
> --- 789,807 ----
> XPUSHs(sv_2mortal(newSVpv("undef", 0)));
> for (i = 0; i < desc->nargs; i++)
> {
> + if (desc->arg_is_p[i]){
> + HeapTuple typeTup;
> + Form_pg_type typeStruct;
> +
> + typeTup = SearchSysCache(TYPEOID,
> + ObjectIdGetDatum(get_fn_expr_argtype(fcinfo->flinfo, i)),
> + 0, 0, 0);
> + typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
> + perm_fmgr_info(typeStruct->typoutput, &(desc->arg_out_func[i]));
> + desc->arg_typioparam[i] = getTypeIOParam(typeTup);
> + ReleaseSysCache(typeTup);
> + }
> +
> if (desc->arg_is_rowtype[i])
> {
> if (fcinfo->argnull[i])
> ***************
> *** 909,914 ****
> --- 959,977 ----
> perlret = srf_perlret;
> }
>
> + if (prodesc->fn_retispseudo){
> + HeapTuple retTypeTup;
> + Form_pg_type retTypeStruct;
> +
> + retTypeTup = SearchSysCache(TYPEOID,
> + ObjectIdGetDatum(get_fn_expr_rettype(fcinfo->flinfo)),
> + 0, 0, 0);
> + retTypeStruct = (Form_pg_type) GETSTRUCT(retTypeTup);
> + perm_fmgr_info(retTypeStruct->typinput, &(prodesc->result_in_func));
> + prodesc->result_typioparam = getTypeIOParam(retTypeTup);
> + ReleaseSysCache(retTypeTup);
> + }
> +
> if (prodesc->fn_retisset && SRF_IS_FIRSTCALL())
> {
> if (prodesc->fn_retistuple)
> ***************
> *** 1149,1161 ****
>
> }
> else
> /* perl string to Datum */
>
> retval = FunctionCall3(&prodesc->result_in_func,
> PointerGetDatum(SvPV(perlret, PL_na)),
> ObjectIdGetDatum(prodesc->result_typioparam),
> Int32GetDatum(-1));
> !
> }
>
> SvREFCNT_dec(perlret);
> --- 1212,1234 ----
>
> }
> else
> + {
> + SV* ret;
> +
> + if (prodesc->fn_retisarray)
> + {
> + if(SvTYPE(SvRV(perlret))!=SVt_PVAV) elog(ERROR, "plperl: this function must return reference to array");
> + ret = plperl_convert_to_pg_array(perlret);
> + SvREFCNT_dec(perlret);
> + perlret = ret;
> + }
> /* perl string to Datum */
>
> retval = FunctionCall3(&prodesc->result_in_func,
> PointerGetDatum(SvPV(perlret, PL_na)),
> ObjectIdGetDatum(prodesc->result_typioparam),
> Int32GetDatum(-1));
> ! }
> }
>
> SvREFCNT_dec(perlret);
> ***************
> *** 1384,1395 ****
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype result, except VOID or RECORD */
> if (typeStruct->typtype == 'p')
> {
> if (procStruct->prorettype == VOIDOID ||
> ! procStruct->prorettype == RECORDOID)
> ! /* okay */ ;
> else if (procStruct->prorettype == TRIGGEROID)
> {
> free(prodesc->proname);
> --- 1457,1471 ----
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype result, except VOID, RECORD, ANYELEMENT or ANYARRAY */
> if (typeStruct->typtype == 'p')
> {
> if (procStruct->prorettype == VOIDOID ||
> ! procStruct->prorettype == RECORDOID ||
> ! procStruct->prorettype == ANYARRAYOID ||
> ! procStruct->prorettype == ANYELEMENTOID)
> ! /* okay */
> ! prodesc->fn_retispseudo=true;
> else if (procStruct->prorettype == TRIGGEROID)
> {
> free(prodesc->proname);
> ***************
> *** 1421,1426 ****
> --- 1497,1509 ----
> procStruct->prorettype;
> }
>
> + if (procStruct->prorettype != ANYARRAYOID)
> + if (typeStruct->typlen == -1 && typeStruct->typelem) /*true, if function returns "true" array*/
> + prodesc->fn_retisarray = true;
> + else
> + prodesc->fn_retisarray = false;
> + else prodesc->fn_retisarray = true;
> +
> perm_fmgr_info(typeStruct->typinput, &(prodesc->result_in_func));
> prodesc->result_typioparam = getTypeIOParam(typeTup);
>
> ***************
> *** 1448,1455 ****
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype argument */
> if (typeStruct->typtype == 'p')
> {
> free(prodesc->proname);
> free(prodesc);
> --- 1531,1543 ----
> }
> typeStruct = (Form_pg_type) GETSTRUCT(typeTup);
>
> ! /* Disallow pseudotype argument, except ANYELEMENT or ANYARRAY */
> if (typeStruct->typtype == 'p')
> + if (procStruct->proargtypes[i] == ANYARRAYOID ||
> + procStruct->proargtypes[i] == ANYELEMENTOID)
> + /* okay */
> + prodesc->arg_is_p[i] = true;
> + else
> {
> free(prodesc->proname);
> free(prodesc);
> ***************
> *** 1458,1463 ****
> --- 1546,1553 ----
> errmsg("plperl functions cannot take type %s",
> format_type_be(procStruct->proargtypes[i]))));
> }
> + else
> + prodesc->arg_is_p[i] = false;
>
> if (typeStruct->typtype == 'c')
> prodesc->arg_is_rowtype[i] = true;

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>Peter Eisentraut wrote:
>
>
>>Joshua D. Drake wrote:
>>
>>
>>>We expected this to a degree of course, but if we can get some of
>>>them in, it would be nice for the community
>>>who wants to use plPerl.
>>>
>>>
>>On the other hand, it wouldn't be that nice for the community that
>>respects a good freeze. As you know, there will always be one more
>>feature.
>>
>>Considering that we're already a long time into the beta phase, and
>>we're still working out portability issues especially in the various
>>plug-ins, we really ought to be strict about the freeze in that area if
>>we ever want to get finished.
>>
>>
>
>Agreed. This is the downside of being bundled with the server. Sorry.
>
>
>

Entirely expected (at least by me). I certainly respect a good freeze
(what a nice phrase).

However, there are outstanding patches from Abhijit Menon-Sen that are
genuine bug fixes that need to be queued, reviewed and applied.

cheers

andrew

>>
>>
>
> Entirely expected (at least by me). I certainly respect a good freeze
> (what a nice phrase).
>
> However, there are outstanding patches from Abhijit Menon-Sen that are
> genuine bug fixes that need to be queued, reviewed and applied.
>
It was worth a shot ;)

Joshua D. Drake

> cheers
>
> andrew
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC
Postgresql support, programming shared hosting and dedicated hosting.
+1-503-667-4564 - jd(at)commandprompt(dot)com - http://www.commandprompt.com
PostgreSQL Replicator -- production quality replication for PostgreSQL

Andrew Dunstan wrote:
> >>Considering that we're already a long time into the beta phase, and
> >>we're still working out portability issues especially in the various
> >>plug-ins, we really ought to be strict about the freeze in that area if
> >>we ever want to get finished.
> >>
> >>
> >
> >Agreed. This is the downside of being bundled with the server. Sorry.
> >
> >
> >
>
> Entirely expected (at least by me). I certainly respect a good freeze
> (what a nice phrase).
>
> However, there are outstanding patches from Abhijit Menon-Sen that are
> genuine bug fixes that need to be queued, reviewed and applied.

Right, I have not gotten to them yet.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Sergej Sergeev wrote:
>
> >Sergej Sergeev <sergej(at)commandprompt(dot)com> writes:
> >
> >
> >>>What happens if you feed other pseudotypes, like cstring or
> >>>language_handler? Shouldn't that be disallowed or something?
> >>>
> >>>
> >
> >
> >
> >>Other pseudo-types are disallowed (no-change)
> >>
> >>
> >
> >No, because you diked out the check at lines 1452ff, rather than
> >upgrading it to something correct.
> >
> >I find the "fn_retispseudo" and "arg_is_p" flags pretty bogus anyway
> >since they fail to indicate *which* pseudotype it is. You might as
> >well just test for the specific type OID.
> >
> > regards, tom lane
> >
> >
> New patch. I have added the check pseudo-type argumetns.
> Specific type is substituted in runtime, during function call.

I can't apply this patch because the code has changed too much. Would
you regenerate a patch against current CVS?

Also, this indenting seems wrong:

> ! /* Disallow pseudotype argument, except ANYELEMENT or ANYARRAY */
> if (typeStruct->typtype == 'p')
> + if (procStruct->proargtypes[i] == ANYARRAYOID ||
> + procStruct->proargtypes[i] == ANYELEMENTOID)
> + /* okay */
> + prodesc->arg_is_p[i] = true;
> + else
> {
> free(prodesc->proname);
> free(prodesc);

Putting an 'if' after an 'if' is just too strange. Please make a more
complete fix that has proper block indenting.

Also, I don't think the arg_is_p variable is really the proper fix for
this, but I am unsure what to recomment. Others?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Also, I don't think the arg_is_p variable is really the proper fix for
> this, but I am unsure what to recomment. Others?

The thing I didn't like about that was that it assumes there is only
one pseudotype behavior that is or ever will be interesting for plperl.

I think it'd probably make more sense to store an array of the parameter
type OIDs and then check for ANYELEMENT or ANYARRAY as such in the
places where the patch uses arg_is_p.

regards, tom lane

Sergej, are you going to repost this patch?

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Also, I don't think the arg_is_p variable is really the proper fix for
> > this, but I am unsure what to recomment. Others?
>
> The thing I didn't like about that was that it assumes there is only
> one pseudotype behavior that is or ever will be interesting for plperl.
>
> I think it'd probably make more sense to store an array of the parameter
> type OIDs and then check for ANYELEMENT or ANYARRAY as such in the
> places where the patch uses arg_is_p.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

This was the patch that I took the array processing piece from and
attempted to fix, since it was badly broken. However, I'm not happy
about any of the ways of doing it, and suspect I won't get it done for
8.1. I think we need that piece done before we look at ANYELEMENT/ANYARRAY.

cheers

andrew

Bruce Momjian wrote:

>Sergej, are you going to repost this patch?
>
>---------------------------------------------------------------------------
>
>Tom Lane wrote:
>
>
>>Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>>
>>
>>>Also, I don't think the arg_is_p variable is really the proper fix for
>>>this, but I am unsure what to recomment. Others?
>>>
>>>
>>The thing I didn't like about that was that it assumes there is only
>>one pseudotype behavior that is or ever will be interesting for plperl.
>>
>>I think it'd probably make more sense to store an array of the parameter
>>type OIDs and then check for ANYELEMENT or ANYARRAY as such in the
>>places where the patch uses arg_is_p.
>>
>> regards, tom lane
>>
>>---------------------------(end of broadcast)---------------------------
>>TIP 7: don't forget to increase your free space map settings
>>
>>
>>
>
>
>

Do we need a TODO item?

---------------------------------------------------------------------------

Andrew Dunstan wrote:
>
> This was the patch that I took the array processing piece from and
> attempted to fix, since it was badly broken. However, I'm not happy
> about any of the ways of doing it, and suspect I won't get it done for
> 8.1. I think we need that piece done before we look at ANYELEMENT/ANYARRAY.
>
> cheers
>
> andrew
>
> Bruce Momjian wrote:
>
> >Sergej, are you going to repost this patch?
> >
> >---------------------------------------------------------------------------
> >
> >Tom Lane wrote:
> >
> >
> >>Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> >>
> >>
> >>>Also, I don't think the arg_is_p variable is really the proper fix for
> >>>this, but I am unsure what to recomment. Others?
> >>>
> >>>
> >>The thing I didn't like about that was that it assumes there is only
> >>one pseudotype behavior that is or ever will be interesting for plperl.
> >>
> >>I think it'd probably make more sense to store an array of the parameter
> >>type OIDs and then check for ANYELEMENT or ANYARRAY as such in the
> >>places where the patch uses arg_is_p.
> >>
> >> regards, tom lane
> >>
> >>---------------------------(end of broadcast)---------------------------
> >>TIP 7: don't forget to increase your free space map settings
> >>
> >>
> >>
> >
> >
> >
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>Do we need a TODO item?
>
>

Sure, Maybe two:

. pass arrays natively instead of as text between plperl and postgres
. add support for polymorphic arguments and return types to plperl

cheers

andrew

Andrew Dunstan wrote:
>
>
> Bruce Momjian wrote:
>
> >Do we need a TODO item?
> >
> >
>
> Sure, Maybe two:
>
> . pass arrays natively instead of as text between plperl and postgres
> . add support for polymorphic arguments and return types to plperl

Added to TODO:

o Pass arrays natively instead of as text between plperl and postgres
o Add support for polymorphic arguments and return types to plperl

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>Sergej, are you going to repost this patch?
>
>
Sorry for delaying.
Yes, I working on it, but I wait for decision about Andrew and Abhijit
patches.

Sergej Sergeev said:
> Bruce Momjian wrote:
>
>>Sergej, are you going to repost this patch?
>>
>>
> Sorry for delaying.
> Yes, I working on it, but I wait for decision about Andrew and Abhijit
> patches.
>

This is the polymorphic types plus perl to pg array patch, right?

I am not working on this right now (or anything else for 8.1) - the original
plperl array to pg array implementation was broken and I was not happy about
the fix I first came up with, and ran out of time to work on an acceptable
solution.

Neither is Abhijit, to the best of my knowledge - he has today submitted a
patch for SPI fetching via cursor, which should not affect this stuff.

We recently put the two items on the TODO list, as I understood from Joshua
that you guys weren't working on plperl at all in the 8.1 feature freeze
time frame.

cheers

andrew