codlin_month is up and complain - PL/Python crash

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:

I revived codlin_month and it falls during PL/Python test:
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=codlin_moth&amp;dt=2010-02-16%2015:09:05

All of the MemoryContextSwitchTo calls in plpython seem to be in
patterns like this:

MemoryContext oldcontext;

oldcontext = CurrentMemoryContext;
PG_TRY();
{
... do something ...
}
PG_CATCH();
{
MemoryContextSwitchTo(oldcontext);

Since oldcontext is only set in the one place, it really shouldn't
require "volatile" decoration, but maybe it does. Can you do some
testing to see if that would fix it?

(Of course, really plpython's bogus approach to error handling ought
to get thrown out and rewritten from scratch, but that's not happening
right now.)

regards, tom lane

On ons, 2010-02-17 at 11:05 -0500, Tom Lane wrote:

All of the MemoryContextSwitchTo calls in plpython seem to be in
patterns like this:

MemoryContext oldcontext;

oldcontext = CurrentMemoryContext;
PG_TRY();
{
... do something ...
}
PG_CATCH();
{
MemoryContextSwitchTo(oldcontext);

Since oldcontext is only set in the one place, it really shouldn't
require "volatile" decoration, but maybe it does.

It is my understanding that local automatic variables may be clobbered
by [sig]longjmp unless they are marked volatile. The PG_CATCH branch is
reached by means of a [sig]longjmp. So that would mean that any
variable that you want to use both before the TRY and inside the CATCH
has to be volatile.

Peter Eisentraut <peter_e@gmx.net> writes:

On ons, 2010-02-17 at 11:05 -0500, Tom Lane wrote:

Since oldcontext is only set in the one place, it really shouldn't
require "volatile" decoration, but maybe it does.

It is my understanding that local automatic variables may be clobbered
by [sig]longjmp unless they are marked volatile. The PG_CATCH branch is
reached by means of a [sig]longjmp. So that would mean that any
variable that you want to use both before the TRY and inside the CATCH
has to be volatile.

If the rule were quite that strict then we'd need many more "volatile"
markers than we have. I believe the actual implementation issue is that
longjmp restores the register contents to what they were at the time of
the setjmp call, and thus a variable allocated in a register would get
restored to the value it had at entry to PG_TRY whereas a variable
allocated on the stack would still have an up-to-date value. Now the
picture isn't quite that simple since a sufficiently smart compiler
might move the variable's value around within the routine. But the
behavior gcc appears to exhibit is that it won't warn about variables
that are only assigned once before the PG_TRY is entered, and that seems
reasonable to me since such a variable ought to have the correct value
either way.

It might be interesting to modify these bits of code so that the
oldcontext variables are assigned only at declaration:

MemoryContext oldcontext = CurrentMemoryContext;

...
PG_TRY();

and see if that makes the issue go away.

regards, tom lane

On ons, 2010-02-17 at 11:26 -0500, Tom Lane wrote:

But the behavior gcc appears to exhibit is that it won't warn about
variables that are only assigned once before the PG_TRY is entered,
and that seems reasonable to me since such a variable ought to have
the correct value either way.

FWIW, this is a Sun Studio build that is complaining here.

Dne 17.02.10 18:39, Peter Eisentraut napsal(a):

On ons, 2010-02-17 at 11:26 -0500, Tom Lane wrote:

But the behavior gcc appears to exhibit is that it won't warn about
variables that are only assigned once before the PG_TRY is entered,
and that seems reasonable to me since such a variable ought to have
the correct value either way.

FWIW, this is a Sun Studio build that is complaining here.

Yes It is SS12. I add volatile keyword and problem disappears. The code
difference is following:

< PLy_spi_execute+0x742: 83 ec 0c subl $0xc,%esp
< PLy_spi_execute+0x745: ff b5 b8 f9 ff ff pushl 0xfffff9b8(%ebp)
< PLy_spi_execute+0x74b: e8 fc ff ff ff call MemoryContextSwitch

PLy_spi_execute+0x742: 8b 85 cc f9 ff ff movl

0xfffff9cc(%ebp),%eax

PLy_spi_execute+0x748: 83 ec 0c subl $0xc,%esp
PLy_spi_execute+0x74b: 50 pushl %eax
PLy_spi_execute+0x74c: e8 fc ff ff ff call MemoryContextSwitch

Good to mention that SS inline PLy_spi_execute_query inside
PLy_spi_execute(), because it is only one caller.

Zdenek

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:

Dne 17.02.10 18:39, Peter Eisentraut napsal(a):

FWIW, this is a Sun Studio build that is complaining here.

Yes It is SS12. I add volatile keyword and problem disappears.

OK, I've applied that change in CVS. Please change codlin_moth back to
the higher optimization setting.

regards, tom lane