Обсуждение: 9.1 plperlu bug with null rows in trigger hash

I've not been able to duplicate this in a standalone script yet,=20
but in the guts of Bucardo is a trigger function called validate_goat()=20
that is giving this error on 9.1 HEAD, but not on previous versions:

"Failed to add table "public.pgbench_tellers": DBD::Pg::st execute=20
failed: ERROR:  Modification of non-creatable hash value attempted,=20
subscript "pkey" at line 4."

I was able to simplify the function to just this and still produce=20
the error:

CREATE OR REPLACE FUNCTION bucardo.validate_goat()
RETURNS TRIGGER
LANGUAGE plperlu
AS
$bc$

my $new =3D $_TD->{new};
$new->{pkey} =3D 'foobar';

return 'MODIFY';

$bc$;

It's used like this:

CREATE TRIGGER validate_goat
  BEFORE INSERT OR UPDATE ON bucardo.goat
  FOR EACH ROW EXECUTE PROCEDURE bucardo.validate_goat();

The goat table has many text fields, of which one is=20
pkey. Setting it to any of those other columns will cause the error.=20
However, setting it to a text field that is NOT NULL DEFAULT will=20
*not* produce the error, so obviously something is setting=20
$_TD->{new}{somecol} to undef in the wrong way. I'm baffled as=20
to why I cannot reproduce it standalone, but wanted to get the=20
bug out there so I don't forget about it and in case anyone=20
wants to take a swing at it. Some Googling suggests it might=20
be because we are using &PL_sv_undef instead of a proper=20
newSV(0).

--=20
Greg Sabino Mullane greg@endpoint.com
End Point Corporation
PGP Key: 0x14964AC8

On Mon, May 23, 2011 at 14:59, Greg Sabino Mullane <greg@endpoint.com> wrote:
> I've not been able to duplicate this in a standalone script yet,
> but in the guts of Bucardo is a trigger function called validate_goat()
> that is giving this error on 9.1 HEAD, but not on previous versions:
>
> "Failed to add table "public.pgbench_tellers": DBD::Pg::st execute
> failed: ERROR:  Modification of non-creatable hash value attempted,
> subscript "pkey" at line 4."

> ...
> Some Googling suggests it might
> be because we are using &PL_sv_undef instead of a proper
> newSV(0).

Yep. Per http://search.cpan.org/~jesse/perl-5.14.0/pod/perlguts.pod#AVs,_HVs_and_undefined_values

|...For example, intuition tells you that this XS code:
|
|    AV *av = newAV();
|    av_store( av, 0, &PL_sv_undef );
|
| is equivalent to this Perl code:
|
|    my @av;
|    $av[0] = undef;
| Unfortunately, this isn't true. AVs use &PL_sv_undef as a marker for
indicating that an array element has not yet been initialized.

We have a few places that have that pattern :-(.

I was able to reproduce it fairly easily(1) by passing in NULL values
explicitly. Fixed in the attached.

I looked at 9.0 and below and they did this correctly. This code path
was heavily re-factored in 9.1 for better array and composite type
support . As noted in perlguts using &PL_sv_undef follows your
intuition, but its wrong :-(. Classic perl xs I suppose.

Greg, can you confirm the attached fixes it for you?

--
[1]
=> create or replace function td() returns trigger language plperlu as
$bc$
$_TD->{new}{a} = 1;
return 'MODIFY';
$bc$;
CREATE FUNCTION

=>  create table trig_test(a int);
CREATE TABLE

=> create trigger test_trig before insert on trig_test for each row
execute procedure td();
CREATE TRIGGER

=> insert into trig_test values (NULL);
CONTEXT:  PL/Perl function "td"
ERROR:  Modification of non-creatable hash value attempted, subscript
"a" at line 2.

On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
...
> Greg, can you confirm the attached fixes it for you?

Yes, seems to have done the job, thank you.


--=20
Greg Sabino Mullane greg@endpoint.com
End Point Corporation
PGP Key: 0x14964AC8

On Mon, May 23, 2011 at 20:08, Greg Sabino Mullane <greg@endpoint.com> wrote:
> On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
> ...
>> Greg, can you confirm the attached fixes it for you?
>
> Yes, seems to have done the job, thank you.

Thanks for testing!

[ Does a little dance to try and attract a -commiter ]

This was broken as part of:
commit 87bb2ade2ce646083f39d5ab3e3307490211ad04
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date:   Thu Feb 17 22:11:50 2011 -0300

    Convert Postgres arrays to Perl arrays on PL/perl input arguments

    More generally, arrays are turned in Perl array references, and row and
    composite types are turned into Perl hash references.  This is done
    recursively, in a way that's natural to every Perl programmer.

    To avoid a backwards compatibility hit, the string representation of
    each structure is also available if the function requests it.

    Authors: Alexey Klyukin and Alex Hunsaker.
    Some code cleanups by me.

Which also means it only breaks HEAD/9.1 (I did test and review 9.0 and down.)

Per http://search.cpan.org/~jesse/perl-5.14.0/pod/perlguts.pod#AVs,_HVs_and_undefined_values,
we do not want to use &PL_sv_undef for undef values in hashes and
arrays. I (inadvertently) broke this in the above commit. As perldoc
mentions &PL_sv_undef seems like the intuitive thing to use. But its
wrong in certain cases.

We have 6 other uses of &PL_sv_undef, 2 &PL_sv_no and 1 &PL_sv_yes.
These are all ok as none of them use the HV/AV store interface.

I elected _not_ to add any regression tests. (If we forget about this
in the future, it will likely be in other code paths). Instead I added
comments to the places that used &PL_sv_undef noting that we
explicitly avoid it on purpose.

On May 27, 2011, at 7:14 PM, Alex Hunsaker wrote:

> On Mon, May 23, 2011 at 20:08, Greg Sabino Mullane <greg@endpoint.com> wr=
ote:
>> On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
>> ...
>>> Greg, can you confirm the attached fixes it for you?
>>=20
>> Yes, seems to have done the job, thank you.
>=20
> Thanks for testing!
>=20
> [ Does a little dance to try and attract a -commiter ]
>=20
> This was broken as part of:
> commit 87bb2ade2ce646083f39d5ab3e3307490211ad04
> Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Date:   Thu Feb 17 22:11:50 2011 -0300
>=20
>    Convert Postgres arrays to Perl arrays on PL/perl input arguments
>=20
>    More generally, arrays are turned in Perl array references, and row and
>    composite types are turned into Perl hash references.  This is done
>    recursively, in a way that's natural to every Perl programmer.
>=20
>    To avoid a backwards compatibility hit, the string representation of
>    each structure is also available if the function requests it.
>=20
>    Authors: Alexey Klyukin and Alex Hunsaker.
>    Some code cleanups by me.
>=20
> Which also means it only breaks HEAD/9.1 (I did test and review 9.0 and d=
own.)
>=20
> Per http://search.cpan.org/~jesse/perl-5.14.0/pod/perlguts.pod#AVs,_HVs_a=
nd_undefined_values,
> we do not want to use &PL_sv_undef for undef values in hashes and
> arrays. I (inadvertently) broke this in the above commit. As perldoc
> mentions &PL_sv_undef seems like the intuitive thing to use. But its
> wrong in certain cases.

Yeah, per the link above the problem is evident and after a little testing
I think your patch fixed the problem. Thank you for tracking down this!

>=20
> We have 6 other uses of &PL_sv_undef, 2 &PL_sv_no and 1 &PL_sv_yes.
> These are all ok as none of them use the HV/AV store interface.
>=20
> I elected _not_ to add any regression tests. (If we forget about this
> in the future, it will likely be in other code paths). Instead I added
> comments to the places that used &PL_sv_undef noting that we
> explicitly avoid it on purpose.

+1.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

Excerpts from Alex Hunsaker's message of vie may 27 12:14:25 -0400 2011:
> On Mon, May 23, 2011 at 20:08, Greg Sabino Mullane <greg@endpoint.com> wrote:
> > On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
> > ...
> >> Greg, can you confirm the attached fixes it for you?
> >
> > Yes, seems to have done the job, thank you.
>
> Thanks for testing!
>
> [ Does a little dance to try and attract a -commiter ]

Okay, I'll handle it :-)

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

Excerpts from Alvaro Herrera's message of sáb may 28 01:06:42 -0400 2011:
> Excerpts from Alex Hunsaker's message of vie may 27 12:14:25 -0400 2011:
> > On Mon, May 23, 2011 at 20:08, Greg Sabino Mullane <greg@endpoint.com> wrote:
> > > On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
> > > ...
> > >> Greg, can you confirm the attached fixes it for you?
> > >
> > > Yes, seems to have done the job, thank you.
> >
> > Thanks for testing!
> >
> > [ Does a little dance to try and attract a -commiter ]
>
> Okay, I'll handle it :-)

Pushed, thanks.

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

On Mon, May 30, 2011 at 11:02, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Excerpts from Alvaro Herrera's message of s=C3=A1b may 28 01:06:42 -0400 =
2011:
>> Excerpts from Alex Hunsaker's message of vie may 27 12:14:25 -0400 2011:
>> > On Mon, May 23, 2011 at 20:08, Greg Sabino Mullane <greg@endpoint.com>=
 wrote:
>> > > On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
>> > > ...
>> > >> Greg, can you confirm the attached fixes it for you?
>> > >
>> > > Yes, seems to have done the job, thank you.
>> >
>> > Thanks for testing!
>> >
>> > [ Does a little dance to try and attract a -commiter ]
>>
>> Okay, I'll handle it :-)
>
> Pushed, thanks.

Great thanks!