Re: why does plperl cache functions using just a bool for is_trigger

I see that plperl uses a triple of (function oid, is_trigger flag, user
id) as a hash key for caching compiled functions. OTOH pltcl and plpgsql
both use (oid, trigger relation oid, user id). Is there any reason why
just using a bool as plperl does would be wrong?

I'm trying to write a validator function for plpython and I'm not sure
if I should copy plperl's or plpgsql's logic.

Cheers,
Jan

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
> I see that plperl uses a triple of (function oid, is_trigger flag, user
> id) as a hash key for caching compiled functions. OTOH pltcl and plpgsql
> both use (oid, trigger relation oid, user id). Is there any reason why
> just using a bool as plperl does would be wrong?

plpgsql needs to consider the trigger relation OID because datatypes of
the trigger relation's columns will make their way into cached plans
for the function. The same function, if applied as a trigger on two
different rels, could therefore have different cached plans so it needs
two separate cache entries.

In PLs where this kind of dependency isn't possible, there's no need for
separate function cache entries.

I'm not certain that plperl is actually correct to do it that way,
but that's the basic idea.

regards, tom lane

On 10/24/2010 06:44 PM, Tom Lane wrote:
> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer(at)wulczer(dot)org> writes:
>> I see that plperl uses a triple of (function oid, is_trigger flag, user
>> id) as a hash key for caching compiled functions. OTOH pltcl and plpgsql
>> both use (oid, trigger relation oid, user id). Is there any reason why
>> just using a bool as plperl does would be wrong?
> plpgsql needs to consider the trigger relation OID because datatypes of
> the trigger relation's columns will make their way into cached plans
> for the function. The same function, if applied as a trigger on two
> different rels, could therefore have different cached plans so it needs
> two separate cache entries.
>
> In PLs where this kind of dependency isn't possible, there's no need for
> separate function cache entries.
>
> I'm not certain that plperl is actually correct to do it that way,
> but that's the basic idea.

Why do we need the is_trigger flag at all for the plperl hash key? At
first glance it strikes me as unnecessary.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 10/24/2010 06:44 PM, Tom Lane wrote:
>> I'm not certain that plperl is actually correct to do it that way,
>> but that's the basic idea.

> Why do we need the is_trigger flag at all for the plperl hash key? At
> first glance it strikes me as unnecessary.

We might not. Does the presence or absence of the $_TD hash reference
have any impact on what we cache, or what Perl might cache internally?

regards, tom lane

On 10/24/2010 07:50 PM, Tom Lane wrote:
> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>> On 10/24/2010 06:44 PM, Tom Lane wrote:
>>> I'm not certain that plperl is actually correct to do it that way,
>>> but that's the basic idea.
>> Why do we need the is_trigger flag at all for the plperl hash key? At
>> first glance it strikes me as unnecessary.
> We might not. Does the presence or absence of the $_TD hash reference
> have any impact on what we cache, or what Perl might cache internally?

For both trigger and non-trigger functions, we compile this ahead of the
user-set function code:

our $_TD; local $_TD=shift;

Non-trigger functions get passed "undef" to correspond to this invisible
argument, while trigger functions get passed the hashref that the
trigger calling code has set up.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 10/24/2010 07:50 PM, Tom Lane wrote:
>> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>>> Why do we need the is_trigger flag at all for the plperl hash key? At
>>> first glance it strikes me as unnecessary.

>> We might not. Does the presence or absence of the $_TD hash reference
>> have any impact on what we cache, or what Perl might cache internally?

> For both trigger and non-trigger functions, we compile this ahead of the
> user-set function code:
> our $_TD; local $_TD=shift;
> Non-trigger functions get passed "undef" to correspond to this invisible
> argument, while trigger functions get passed the hashref that the
> trigger calling code has set up.

Seems like we don't need it then. You going to get rid of it?

regards, tom lane

On 10/24/2010 09:34 PM, Tom Lane wrote:
>
>> For both trigger and non-trigger functions, we compile this ahead of the
>> user-set function code:
>> our $_TD; local $_TD=shift;
>> Non-trigger functions get passed "undef" to correspond to this invisible
>> argument, while trigger functions get passed the hashref that the
>> trigger calling code has set up.
> Seems like we don't need it then. You going to get rid of it?

Ok, will do.

cheers

andrew

On 25/10/10 03:59, Andrew Dunstan wrote:
>
>
> On 10/24/2010 09:34 PM, Tom Lane wrote:
>>
>>> For both trigger and non-trigger functions, we compile this ahead of the
>>> user-set function code:
>>> our $_TD; local $_TD=shift;
>>> Non-trigger functions get passed "undef" to correspond to this invisible
>>> argument, while trigger functions get passed the hashref that the
>>> trigger calling code has set up.
>> Seems like we don't need it then. You going to get rid of it?
>
> Ok, will do.

Seems that this circumverts some output conversion error checking, since
adding the attached to the regression suite results in a segfault during
the plperl installcheck.

Reverting 2d01ec0708d571eef926f3f5795aa73759df5d9a fixes it. Noticed
while fooling around with plpython and hitting a similar error (since
plpython does have a regression test for trigger functions being called
directly).

Cheers,
Jan

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
> Seems that this circumverts some output conversion error checking, since
> adding the attached to the regression suite results in a segfault during
> the plperl installcheck.

> Reverting 2d01ec0708d571eef926f3f5795aa73759df5d9a fixes it.

Good catch, patch reverted (and regression test added).

regards, tom lane

On 10/31/2010 11:44 AM, Tom Lane wrote:
> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer(at)wulczer(dot)org> writes:
>> Seems that this circumverts some output conversion error checking, since
>> adding the attached to the regression suite results in a segfault during
>> the plperl installcheck.
>> Reverting 2d01ec0708d571eef926f3f5795aa73759df5d9a fixes it.
> Good catch, patch reverted (and regression test added).

Well, I guess that answers the question of why we needed it, which
nobody could answer before. I'm not sure I exactly understand what's
going on here, though - I guess I need to look at it closer. At least I
think we need a code comment on why the trigger flag is needed as part
of the hash key.

cheers

andrew

On Sun, Oct 31, 2010 at 12:00, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> On 10/31/2010 11:44 AM, Tom Lane wrote:
>> Good catch, patch reverted (and regression test added).
>
> Well, I guess that answers the question of why we needed it, which nobody
> could answer before. I'm not sure I exactly understand what's going on here,
> though - I guess I need to look at it closer. At least I think we need a
> code comment on why the trigger flag is needed as part of the hash key.

The stack trace is:
#0 0x0000000000000000 in ?? ()
#1 0x00000000006c18e9 in InputFunctionCall (flinfo=0x2a039a0,
str=0x0, typioparam=0, typmod=-1)
#2 0x00007ff6d2bdf950 in plperl_func_handler (fcinfo=0x7fff4743bec0)
at plperl.c:1729

which happens because prodesc->result_in_func.fn_addr (flinfo) is
NULL. That happens because when we are a trigger we don't setup
input/output conversion. And with the change we get the same
proc_desc for triggers and non triggers, so if the trigger function
gets called first, any call to the direct function will use the same
proc_desc with the wrong input/out conversion.

There is a check so that trigger functions can not be called as plain
functions, but it only gets called when we do not have an entry in
plperl_proc_hash. I think just moving that up, something the like the
attached should be enough. Thoughts?

On 10/31/2010 04:40 PM, Alex Hunsaker wrote:
> On Sun, Oct 31, 2010 at 12:00, Andrew Dunstan<andrew(at)dunslane(dot)net> wrote:
>> On 10/31/2010 11:44 AM, Tom Lane wrote:
>>> Good catch, patch reverted (and regression test added).
>> Well, I guess that answers the question of why we needed it, which nobody
>> could answer before. I'm not sure I exactly understand what's going on here,
>> though - I guess I need to look at it closer. At least I think we need a
>> code comment on why the trigger flag is needed as part of the hash key.
> The stack trace is:
> #0 0x0000000000000000 in ?? ()
> #1 0x00000000006c18e9 in InputFunctionCall (flinfo=0x2a039a0,
> str=0x0, typioparam=0, typmod=-1)
> #2 0x00007ff6d2bdf950 in plperl_func_handler (fcinfo=0x7fff4743bec0)
> at plperl.c:1729
>
> which happens because prodesc->result_in_func.fn_addr (flinfo) is
> NULL. That happens because when we are a trigger we don't setup
> input/output conversion. And with the change we get the same
> proc_desc for triggers and non triggers, so if the trigger function
> gets called first, any call to the direct function will use the same
> proc_desc with the wrong input/out conversion.

How does that happen given that the function Oid is part of the hash key?

> There is a check so that trigger functions can not be called as plain
> functions, but it only gets called when we do not have an entry in
> plperl_proc_hash. I think just moving that up, something the like the
> attached should be enough.

This looks like the right fix, though.

cheers

andrew

On Sun, Oct 31, 2010 at 15:17, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> On 10/31/2010 04:40 PM, Alex Hunsaker wrote:
>> And with the change we get the same
>> proc_desc for triggers and non triggers, so if the trigger function
>> gets called first, any call to the direct function will use the same
>> proc_desc with the wrong input/out conversion.
>
>
> How does that happen given that the function Oid is part of the hash key?

They are the same function and so have the same Oid ?

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 10/31/2010 04:40 PM, Alex Hunsaker wrote:
>> which happens because prodesc->result_in_func.fn_addr (flinfo) is
>> NULL. That happens because when we are a trigger we don't setup
>> input/output conversion. And with the change we get the same
>> proc_desc for triggers and non triggers, so if the trigger function
>> gets called first, any call to the direct function will use the same
>> proc_desc with the wrong input/out conversion.

> How does that happen given that the function Oid is part of the hash key?

I think the crash is dependent on the fact that the function is created
and called in the same session. That means the validator gets called on
it first, and the validator not unreasonably assumes istrigger = true,
and then it calls compile_plperl_function which sets up a cache entry
on that basis. So then when the "regular" call comes along, it tries
to reuse the cache entry in the other style. Kaboom.

>> There is a check so that trigger functions can not be called as plain
>> functions, but it only gets called when we do not have an entry in
>> plperl_proc_hash. I think just moving that up, something the like the
>> attached should be enough.

> This looks like the right fix, though.

No, that is just moving a test that only needs to be done once into a
place where it has to be done every time. You might as well argue that
we shouldn't cache any of the setup but just redo it all every time.

The fundamental issue here is that the contents of plperl_proc_desc
structs are different between the trigger and non-trigger cases.
Unless you're prepared to make them the same, and guarantee that they
always will be the same in future, I think that including the istrigger
flag in the hash key is a good safety feature. It's also the same way
that the other three PLs do things, and I see no good excuse for plperl
to do things differently here.

IOW, it's not broke, let's not fix it.

regards, tom lane

On 11/01/2010 11:28 AM, Tom Lane wrote:
> The fundamental issue here is that the contents of plperl_proc_desc
> structs are different between the trigger and non-trigger cases.
> Unless you're prepared to make them the same, and guarantee that they
> always will be the same in future, I think that including the istrigger
> flag in the hash key is a good safety feature. It's also the same way
> that the other three PLs do things, and I see no good excuse for plperl
> to do things differently here.
>
> IOW, it's not broke, let's not fix it.

Ok, then let's make a note in the code to this effect. When the question
was asked before about why it was there nobody seemed to have any good
answer.

cheers

andrew

On Mon, Nov 1, 2010 at 09:28, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think the crash is dependent on the fact that the function is created
> and called in the same session.  That means the validator gets called on
> it first, and the validator not unreasonably assumes istrigger = true,
> and then it calls compile_plperl_function which sets up a cache entry
> on that basis.  So then when the "regular" call comes along, it tries
> to reuse the cache entry in the other style.  Kaboom.

The other Kaboom happens if the trigger gets called as a trigger first
and then directly.

>>> There is a check so that trigger functions can not be called as plain
>>> functions... I think just moving that up...

> No, that is just moving a test that only needs to be done once into a
> place where it has to be done every time.  You might as well argue that
> we shouldn't cache any of the setup but just redo it all every time.

Huh? I might try and argue that if the new test was more complex than
2 compares :P. In-fact the way it stands now we uselessly grab the
functions pg_proc entry in the common case. Would you object to
trying to clean that up across all pls? Im thinking add an is_trigger
or context to each proc_desc, then check that in the corresponding
handlers. While im at it get rid of at least one SysCache lookup.
Thoughts? We can still keep the is_trigger bool in the hash key, as
you said below it is a good safety feature. I just wish the logic was
spelled out a bit more. Maybe im alone here.

> It's also the same way
> that the other three PLs do things, and I see no good excuse for plperl
> to do things differently here.

Im all in favor of keeping things between the pls as close as possible.

Speaking of which, pltcl stores the trigger reloid instead of a flag
(it also uses tg_reloid in the internal proname). It seems a tad
excessive to have one function *per* trigger table. I looked through
the history to see if there was some reason, it goes all the way back
to the initial commit. I assume its this way because it copied
plpgsql, which needs it as the rowtype might be different per table.
pltcl should not have that issue. Find attached a patch to clean that
up and make it match the other pls (err um plperl). It passes its
regression tests and some additional limited testing. Thoughts?

On Mon, Nov 1, 2010 at 15:24, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
houldn't cache any of the setup but just redo it all every time.
>
> Huh?  I might try and argue that if the new test was more complex than
> 2 compares :P.  In-fact the way it stands now we uselessly grab the
> functions pg_proc entry in the common case.

This is bogus, I missed the fact that we need it to make sure the
function is uptodate for the OR REPLACE in CREATE OR REPLACE.

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> Speaking of which, pltcl stores the trigger reloid instead of a flag
> (it also uses tg_reloid in the internal proname). It seems a tad
> excessive to have one function *per* trigger table. I looked through
> the history to see if there was some reason, it goes all the way back
> to the initial commit. I assume its this way because it copied
> plpgsql, which needs it as the rowtype might be different per table.
> pltcl should not have that issue. Find attached a patch to clean that
> up and make it match the other pls (err um plperl). It passes its
> regression tests and some additional limited testing. Thoughts?

Surely, removing the internal name's dependency on the istrigger flag is
wrong. If you're going to maintain separate hash entries at the pltcl
level, why would you want to risk collisions underneath that?

regards, tom lane

On Mon, Nov 1, 2010 at 16:59, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alex Hunsaker <badalex(at)gmail(dot)com> writes:
>> Speaking of which, pltcl stores the trigger reloid instead of a flag
>> (it also uses tg_reloid in the internal proname).  It seems a tad
>> excessive to have one function *per* trigger table.
>
> Surely, removing the internal name's dependency on the istrigger flag is
> wrong.  If you're going to maintain separate hash entries at the pltcl
> level, why would you want to risk collisions underneath that?

Good catch. I was basing it off plperl which uses the same proname
for both (sprintf(subname, %s__%u", prodesc->proname, fn_oid)). Its
OK for plperl because when we compile we save a reference to it and
use that directly (more or less). The name does not really matter.

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> On Mon, Nov 1, 2010 at 16:59, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Surely, removing the internal name's dependency on the istrigger flag is
>> wrong. If you're going to maintain separate hash entries at the pltcl
>> level, why would you want to risk collisions underneath that?

> Good catch. I was basing it off plperl which uses the same proname
> for both (sprintf(subname, %s__%u", prodesc->proname, fn_oid)). Its
> OK for plperl because when we compile we save a reference to it and
> use that directly (more or less). The name does not really matter.

OK, applied.

I notice that plpython is also using the trigger relation's OID, but I
don't know that language well enough to tell whether it really needs to.

regards, tom lane

On Wed, Nov 3, 2010 at 10:28, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> OK, applied.

Thanks!

> I notice that plpython is also using the trigger relation's OID, but I
> don't know that language well enough to tell whether it really needs to.

This thread was started by someone working a plpython a validator, I
figured the two areas overlap somewhat and did not want to step on any
toes. Anyhow the patch is tiny. So toes should remain intact. Find
it attached.

Given that plpython can only return None/OK", "MODIFY" or "SKIP" and
looking around the code for a bit, I don't see any reason it needs it.
I only tried plpython3, but it passed an installcheck and some
additional testing (two tables with the same column name and different
types using the same trigger).

[ Aside ]
I almost thought using tgreloid was required as PLy_modify_tuple has:

plpython.c:748
modvalues[i] =
InputFunctionCall(&proc->result.out.r.atts[atti].typfunc,
^^^^^
NULL,
proc->result.out.r.atts[atti].typioparam
^^^^^

But Ply_procedure_get has this bit which gets run every time the
function is called:
plpython.c: 1336
/*
* Input/output conversion for trigger tuples. Use the result
* TypeInfo variable to store the tuple conversion info. We do this
* over again on each call to cover the possibility that the
* relation's tupdesc changed since the trigger was last called.
* PLy_input_tuple_funcs and PLy_output_tuple_funcs are responsible
* for not doing repetitive work.
*/
....
PLy_input_tuple_funcs(&(proc->result), tdata->tg_relation->rd_att);
PLy_output_tuple_funcs(&(proc->result), tdata->tg_relation->rd_att);
....

I double checked the other pls just for my sanity. They get it right,
that is look it up instead of using anything cached in proc_desc.

On 03/11/10 20:57, Alex Hunsaker wrote:
> On Wed, Nov 3, 2010 at 10:28, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> OK, applied.
>
> Thanks!
>
>> I notice that plpython is also using the trigger relation's OID, but I
>> don't know that language well enough to tell whether it really needs to.
>
> This thread was started by someone working a plpython a validator, I
> figured the two areas overlap somewhat and did not want to step on any
> toes. Anyhow the patch is tiny. So toes should remain intact. Find
> it attached.

Yeah, it just needs a flag to say trigger/not (but it does need a flag,
for the same reason plperl needs it).

By the way, I'm leaning in the direction of not using a Python
dictionary for the cache, but a standard Postgres HTAB instead. It's
more like other pls this way, and you can get rid of PyCObjects (which
are deprecated BTW) and messing around with reference counting the
cached procedures.

I was even thinking about having *two* hash tables, for trigger and
nontrigger procedures. This way you can make the function OID (which is
useful to hav anyway) be the first field of the structure being cached
and make both hash tables keyed by OIDs. Saves you the trouble of
defining a structure for the key... Not sure if it'll turn out for the
better, but I'm definitely for not using a Python dictionary for the cache.

The validator is ready, once I'm done with the hash tables I'll try to
fix up the error checking (get rid of the global error state) and
finally do what started it all, that is make plpythonu use
subtransactions for SPI and be able to do:

try:
plpy.execute("insert into foo values(1)")
except plpy.UniqueViolation, e:
plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)

Cheers,
Jan

On Wed, Nov 3, 2010 at 14:43, Jan Urbański <wulczer(at)wulczer(dot)org> wrote:
> By the way, I'm leaning in the direction of not using a Python
> dictionary for the cache, but a standard Postgres HTAB instead. It's
> more like other pls this way, and you can get rid of PyCObjects (which
> are deprecated BTW) and messing around with reference counting the
> cached procedures.

Well if they are deprecated and there is an arguably cleaner way to do
it... might as well.

> I was even thinking about having *two* hash tables, for trigger and
> nontrigger procedures...<snip>... Saves you the trouble of
> defining a structure for the key... Not sure if it'll turn out for the
> better, but I'm definitely for not using a Python dictionary for the cache.

*shrug*

> make plpythonu use
> subtransactions for SPI and be able to do:
>
> try:
>    plpy.execute("insert into foo values(1)")
> except plpy.UniqueViolation, e:
>    plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)

Ouuu <googly eyes>.

[ now that eval { }, thanks to Tim Bunce, works with plperl it should
be possible to do something similar there as well. Just noting the
possibility... not volunteering :) ]

On Nov 3, 2010, at 2:06 PM, Alex Hunsaker wrote:

>> try:
>> plpy.execute("insert into foo values(1)")
>> except plpy.UniqueViolation, e:
>> plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
>
> Ouuu <googly eyes>.
>
> [ now that eval { }, thanks to Tim Bunce, works with plperl it should
> be possible to do something similar there as well. Just noting the
> possibility... not volunteering :) ]

/me wants a global $dbh that mimics the DBI interface but just uses SPI under the hood. Not volunteering, either…

David

On Wed, 2010-11-03 at 21:43 +0100, Jan Urbański wrote:
> The validator is ready, once I'm done with the hash tables I'll try to
> fix up the error checking (get rid of the global error state) and
> finally do what started it all, that is make plpythonu use
> subtransactions for SPI and be able to do:
>
> try:
> plpy.execute("insert into foo values(1)")
> except plpy.UniqueViolation, e:
> plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)

Are you sure that having each try/except use a subtransaction is the
right way to do it ?

I'd like to make it more explicit and use

with plpy.subtransaction():
do your stuff

adding subtransactions to try/except would also act differently on
postgresql and python data, that is things in postgresql tables would
get rolled back but those made to python would not

or at least make the "rollback to savepoint x" optional

try:
plpy.savepoint('sp1')
for i in range(-5,5)
plpy.execute("insert into foo values(%s)", [abs(10/i)])
except plpy.UniqueViolation, e:
plpy.rollback('sp1')
plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
except ZeroDivisionError:
plpy.notice("Only some values were inserted")

-------
Hannu Krosing
PostgreSQL Infinite Scalability and Preformance Consultant
PG Admin Book: http://www.2ndQuadrant.com/books/

On Thu, 2010-11-04 at 11:46 +0200, Hannu Krosing wrote:
> On Wed, 2010-11-03 at 21:43 +0100, Jan Urbański wrote:
> > The validator is ready, once I'm done with the hash tables I'll try to
> > fix up the error checking (get rid of the global error state) and
> > finally do what started it all, that is make plpythonu use
> > subtransactions for SPI and be able to do:
> >
> > try:
> > plpy.execute("insert into foo values(1)")
> > except plpy.UniqueViolation, e:
> > plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
>
> Are you sure that having each try/except use a subtransaction is the
> right way to do it ?

Another objection

> I'd like to make it more explicit and use
>
> with plpy.subtransaction():
> do your stuff

Possibly better syntax would be

with plpy.subtransaction() as subtrx:
try:
plpy.execute("insert into foo values(1)")
except plpy.UniqueViolation, e:
subtrx.rollback()
plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)

-------
Hannu Krosing
PostgreSQL Infinite Scalability and Preformance Consultant
PG Admin Book: http://www.2ndQuadrant.com/books/

On ons, 2010-11-03 at 14:15 -0700, David E. Wheeler wrote:
> /me wants a global $dbh that mimics the DBI interface but just uses
> SPI under the hood. Not volunteering, either…

Already exists: DBD::PgSPI. Probably needs lots of updating through.

Hannu Krosing <hannu(at)2ndQuadrant(dot)com> writes:
> Are you sure that having each try/except use a subtransaction is the
> right way to do it ?

Actually it is not: what you have to do is use a subtransaction in the
plpy.execute() operation, so that if the called SQL operation fails, you
can clean it up and then report the error to Python as if it were any
other Python error. Messing with the host language's exception handling
is a sure route to misery. plperl and pltcl both contain examples of
doing this properly.

regards, tom lane

On Thu, Nov 4, 2010 at 03:54, Hannu Krosing <hannu(at)2ndquadrant(dot)com> wrote:
>> > try:
>> >     plpy.execute("insert into foo values(1)")
>> > except plpy.UniqueViolation, e:
>> >     plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
>>
>> Are you sure that having each try/except use a subtransaction is the
>> right way to do it ?

I assumed the try was purely so you could 'catch' things. And did not
mean run it in a subtransaction (without the try block it still runs
in one).

Personally, I was looking more at:

>> > except plpy.UniqueViolation, e:
>> > plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)

Which to me says if SPI has an error we get a nice error object back,
that also lets you do the normal exception catching dance (if thats
what you are in to...) and translates IMO better to how plpgsql works
("exception when unique_violation").

> Another objection
>
>> I'd like to make it more explicit and use
>>
>> with plpy.subtransaction():
>>     do your stuff

Sounds more like a savepoint?

On Thu, 2010-11-04 at 11:07 -0600, Alex Hunsaker wrote:
> On Thu, Nov 4, 2010 at 03:54, Hannu Krosing <hannu(at)2ndquadrant(dot)com> wrote:
> >> > try:
> >> > plpy.execute("insert into foo values(1)")
> >> > except plpy.UniqueViolation, e:
> >> > plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
> >>
> >> Are you sure that having each try/except use a subtransaction is the
> >> right way to do it ?
>
> I assumed the try was purely so you could 'catch' things. And did not
> mean run it in a subtransaction (without the try block it still runs
> in one).

So your plan was to have some savepoint before each execute ?

How would one rollback the latest transaction ?

Or is it something else you mean by "subtransaction" ?

> Personally, I was looking more at:
>
> >> > except plpy.UniqueViolation, e:
> >> > plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
>
> Which to me says if SPI has an error we get a nice error object back,
> that also lets you do the normal exception catching dance (if thats
> what you are in to...) and translates IMO better to how plpgsql works
> ("exception when unique_violation").

I see. "exception when unique violation" in plpgsql does automatic
rollback to block start (matching BEGIN) so I assumed that your
try/except sample is designed to do something similar

> > Another objection
> >
> >> I'd like to make it more explicit and use
> >>
> >> with plpy.subtransaction():
> >> do your stuff
>
> Sounds more like a savepoint?

Yeah. SAVEPOINT command is how you start a "subtransaction" in plain
SQL.

--
-------
Hannu Krosing
PostgreSQL Infinite Scalability and Preformance Consultant
PG Admin Book: http://www.2ndQuadrant.com/books/

On Nov 4, 2010, at 4:20 AM, Peter Eisentraut wrote:

> On ons, 2010-11-03 at 14:15 -0700, David E. Wheeler wrote:
>> /me wants a global $dbh that mimics the DBI interface but just uses
>> SPI under the hood. Not volunteering, either…
>
> Already exists: DBD::PgSPI. Probably needs lots of updating through.

Funny I never noticed that before. I couldn't get it to build, of course. And it looks a bit heavy, relying on DBD::Pg. Seems like it'd be easier to write something that just uses SPI and emulates the DBI interface, IMHO.

Best,

David

On Thu, Nov 4, 2010 at 13:43, Hannu Krosing <hannu(at)2ndquadrant(dot)com> wrote:
> So your plan was to have some savepoint before each execute ?
>
> How would one rollback the latest transaction ?

It is always rolled back. Its how plperl works today:
create or replace function foo() returns int as $$
eval {
spi_exec_query('create table uniq (num int primary key');
spi_exec_query('insert into uniq (num) values (1), (1);', 1);
};

if($@) {
# do something ... $@ == "duplicate key value violates unique
constraint "uniq_pkey" at line 2."
warn $@;
}

# oh well do something else
# note the transaction is _not_ aborted
spi_exec_query('select 1;', 1);
return 1;
$$ language plperl;

=# begin;
=# select foo();
=# select 1;
=# commit;

It does not matter if you use eval or not, its always in a sub transaction.

> I see. "exception when unique violation" in plpgsql  does automatic
> rollback to block start (matching BEGIN) so I assumed that your
> try/except sample is designed to do something similar

Basically, minus the rollback to start. Its per query.

On Thu, Nov 4, 2010 at 14:29, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> On Thu, Nov 4, 2010 at 13:43, Hannu Krosing <hannu(at)2ndquadrant(dot)com> wrote:
>> So your plan was to have some savepoint before each execute ?
>>
>> How would one rollback the latest transaction ?
>
> It is always rolled back.  Its how plperl works today:
> create or replace function foo() returns int as $$
> eval {
>    spi_exec_query('create table uniq (num int primary key');
>    spi_exec_query('insert into uniq (num) values (1), (1);', 1);
> };

To be clear, there is no reason to have both in an eval {}. There is
no magic savepoint there. if 'insert into' fails, the table will
still be created (assuming the transaction is not aborted later of
course).

On 04/11/10 20:43, Hannu Krosing wrote:
> On Thu, 2010-11-04 at 11:07 -0600, Alex Hunsaker wrote:
>> On Thu, Nov 4, 2010 at 03:54, Hannu Krosing <hannu(at)2ndquadrant(dot)com> wrote:
>>>>> try:
>>>>> plpy.execute("insert into foo values(1)")
>>>>> except plpy.UniqueViolation, e:
>>>>> plpy.notice("Ooops, you got yourself a SQLSTATE %d", e.sqlstate)
>>>>
>>>> Are you sure that having each try/except use a subtransaction is the
>>>> right way to do it ?
>>
>> I assumed the try was purely so you could 'catch' things. And did not
>> mean run it in a subtransaction (without the try block it still runs
>> in one).

Nice, lots of input before I was able to read my email :o)

I'm planning to make plpython work just like plperl with regards to
trapping errors from SPI. As Tom noticed, messing with the error
handling mechanism of Python is not a good way of implementing this.

So, basically each plpy.execute() would be internally executed inside a
subtransaction and if SPI would return an error, it would be transformed
into a Python exception and returned to the Python runtime, which will
then handle it as it would handle and exception coming form a C extension.

It would be interesting to provide an explicit way to start
subtransactions, like Hannu proposed:

with plpy.subtransaction():
plpy.execute("select foo()")
plpy.execute("select bar()")
plpy.execute("select baz()")

(of course that would only work for Python 2.6+, where with blocks were
introduced, we'd have to also provide the primitive functions of
plpy.enter_subxact() and plpy.commit_subxact() (names took at random))

It would set a flag somewhere and start an explicit subtransaction -
after that plpy.execute() would just go ahead and call SPI. This way you
would be sure that you executed a bunch of statements atomically.
Implementing that iss not very high on my priority list, though, as you
can always just wrap foo() bar() and baz() in a separate stored
procedure and call that, thus achieving atomicity (or am I wrong here?).

Cheers,
Jan

PS: I'm wondering if there's any noticable slowdown from always starting
a subxact before doing SPI. Plperl users seemed not to notice, so I
guess I shouldn't worry.

J

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
> PS: I'm wondering if there's any noticable slowdown from always starting
> a subxact before doing SPI. Plperl users seemed not to notice, so I
> guess I shouldn't worry.

It's not cheap :-( ... but it's *necessary*. There's no other way to
get sane behavior.

If the cost annoys you, you should put some effort into making subxact
start/stop cheaper overall, rather than trying to avoid having one here.

regards, tom lane

Excerpts from Jan Urbański's message of vie nov 05 04:19:07 -0300 2010:

> PS: I'm wondering if there's any noticable slowdown from always starting
> a subxact before doing SPI. Plperl users seemed not to notice, so I
> guess I shouldn't worry.

I think it's more "plperl users have to put up with it" rather than "not
notice".

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Nov 5, 2010 at 2:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> It's not cheap :-( ... but it's *necessary*.  There's no other way to
> get sane behavior.
>
> If the cost annoys you, you should put some effort into making subxact
> start/stop cheaper overall, rather than trying to avoid having one here.

I would be pretty happy even if only the *first* subxact was cheap.
That would take care of 99% of use implicit use cases leaving mostly
only cases where users have explicitly asked for a subxact with a
catch/throw block.

In particular it would cover the psql case of wanting to have a
subxact around every interactive command so the user can hit C-c
without undoing their whole transaction.

--
greg