suspicious pointer/integer coersion

Andrew Dunstan <andrew@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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@dunslane.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

This seems to have gone AWOL in the email ether, so I am resending.

-------- Original Message --------

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.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 ...

Further investigation yields this from the perlguts man page:

What is an "IV"?

Perl uses a special typedef IV which is a simple signed integer type
that is guaranteed to be large enough to hold a pointer (as well as
an integer).

So it looks like my original concern was unfounded. Sorry for the noise.
We now return you to your previous program ...

cheers

andrew