Re: suspicious pointer/integer coersion

I have just noticed this code in plperl.c:

hv_store(plperl_proc_hash, internal_proname, proname_len,
newSViv((IV) prodesc), 0);

basically, here prodesc is a pointer to a struct, and we are storing it
as an integer in a perl hash for easy lookup by stringified oid.

How safe is this conversion on 64 bit platforms?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I have just noticed this code in plperl.c:

> hv_store(plperl_proc_hash, internal_proname, proname_len,
> newSViv((IV) prodesc), 0);

> basically, here prodesc is a pointer to a struct, and we are storing it
> as an integer in a perl hash for easy lookup by stringified oid.

> How safe is this conversion on 64 bit platforms?

Not at all. I'd vote for converting the whole thing to a dynahash
hashtable ...

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>I have just noticed this code in plperl.c:
>>
>>
>
>
>
>> hv_store(plperl_proc_hash, internal_proname, proname_len,
>> newSViv((IV) prodesc), 0);
>>
>>
>
>
>
>>basically, here prodesc is a pointer to a struct, and we are storing it
>>as an integer in a perl hash for easy lookup by stringified oid.
>>
>>
>
>
>
>>How safe is this conversion on 64 bit platforms?
>>
>>
>
>Not at all. I'd vote for converting the whole thing to a dynahash
>hashtable ...
>
>
>
>

Works for me. There are some other things about the procdesc stuff I'm
trying to sort out (especially if we should be storing per-call info
inside it).

Cleaning this up is definitely a TODO.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Works for me. There are some other things about the procdesc stuff I'm
> trying to sort out (especially if we should be storing per-call info
> inside it).

Hmm, probably not ... check to see if a recursive plperl function
behaves sanely. (This might not have been much of an issue before
we had SPI support in plperl, since there was no way to recurse;
but it is an issue now.)

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>Works for me. There are some other things about the procdesc stuff I'm
>>trying to sort out (especially if we should be storing per-call info
>>inside it).
>>
>>
>
>Hmm, probably not ... check to see if a recursive plperl function
>behaves sanely. (This might not have been much of an issue before
>we had SPI support in plperl, since there was no way to recurse;
>but it is an issue now.)
>
>

Behaviour is not good (see below for proof).

ISTM we'll need some sort of implicit of explicit stack of per-call
data. The trick will be getting it to behave right under error recovery.

cheers

andrew

[andrew inst]$ bin/psql -e -f recurse.sql
create or replace function recurse(i int) returns setof text language plperl
as $$

my $i = shift;
elog(NOTICE,"i = $i");
foreach my $x (1..$i)
{
return_next "hello $x";
}
if ($i > 2)
{
my $z = $i-1;
my $cursor = spi_query("select * from recurse($z)");
while (defined(my $row = spi_fetchrow($cursor)))
{
return_next "recurse $i: $row";
}
}
return undef;

$$;
CREATE FUNCTION
select * from recurse(2);
psql:recurse.sql:24: NOTICE: i = 2
recurse
---------
hello 1
hello 2
(2 rows)

select * from recurse(3);
psql:recurse.sql:25: NOTICE: i = 3
psql:recurse.sql:25: NOTICE: i = 2
psql:recurse.sql:25: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
psql:recurse.sql:25: connection to server was lost
[andrew inst]$

Andrew Dunstan wrote:

> Tom Lane wrote:
>
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>
>>> Works for me. There are some other things about the procdesc stuff
>>> I'm trying to sort out (especially if we should be storing per-call
>>> info inside it).
>>>
>>
>>
>> Hmm, probably not ... check to see if a recursive plperl function
>> behaves sanely. (This might not have been much of an issue before
>> we had SPI support in plperl, since there was no way to recurse;
>> but it is an issue now.)
>
>
> Behaviour is not good (see below for proof).
>
> ISTM we'll need some sort of implicit of explicit stack of per-call
> data. The trick will be getting it to behave right under error recovery.

Looking further ... we already do this implicitly for prodesc in the
call handler - we would just need to do the same thing for per-call
structures and divorce them from prodesc, which can be repeated on the
implicit stack.

I'll work on that - changes should be quite small.

cheers

andrew

> Looking further ... we already do this implicitly for prodesc in the
> call handler - we would just need to do the same thing for per-call
> structures and divorce them from prodesc, which can be repeated on the
> implicit stack.
>
> I'll work on that - changes should be quite small.

Sounds like recursive calls are somethign that should be tested for PLs
in the regressions...

Chris

Andrew Dunstan wrote:

>
>
>
>
> Looking further ... we already do this implicitly for prodesc in the
> call handler - we would just need to do the same thing for per-call
> structures and divorce them from prodesc, which can be repeated on the
> implicit stack.
>
> I'll work on that - changes should be quite small.
>

Attached is a patch that fixes both a recently introduced problem with
recursion and a problem with array returns that became evident as a
result of not throwing away non-fatal warnings (thanks to David Fetter
for noticing this). Regression test updates to include both cases are
included in the patch.

I will start looking at putting the procedure descriptors in a dynahash.

cheers

andrew

Andrew Dunstan wrote:

>
>
> Andrew Dunstan wrote:
>
>>
>>
>>
>>
>> Looking further ... we already do this implicitly for prodesc in the
>> call handler - we would just need to do the same thing for per-call
>> structures and divorce them from prodesc, which can be repeated on
>> the implicit stack.
>>
>> I'll work on that - changes should be quite small.
>>
>
> Attached is a patch that fixes (I hope) both a recently introduced
> problem with recursion and a problem with array returns that became
> evident as a result of not throwing away non-fatal warnings (thanks to
> David Fetter for noticing this). Regression test updates to include
> both cases are included in the patch.
>
> I will start looking at putting the procedure descriptors in a dynahash.
>
>

and here's the patch this time.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> Attached is a patch that fixes (I hope) both a recently introduced
>> problem with recursion and a problem with array returns that became
>> evident as a result of not throwing away non-fatal warnings (thanks to
>> David Fetter for noticing this). Regression test updates to include
>> both cases are included in the patch.

Applied, thanks.

regards, tom lane