[BUGS] 9.1 plperlu bug with null rows in trigger hash

[Greg Sabino Mullane]

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."

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

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

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

[Alex Hunsaker]

Post by Greg Sabino Mullane
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()
"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.

[Greg Sabino Mullane]

On Mon, May 23, 2011 at 05:04:40PM -0600, Alex Hunsaker wrote:
...

Post by Alex Hunsaker
Greg, can you confirm the attached fixes it for you?

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

[Alex Hunsaker]

Post by Greg Sabino Mullane
...

Post by Alex Hunsaker
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 <***@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.

[Alexey Klyukin]

Post by Alex Hunsaker

Post by Greg Sabino Mullane
...

Post by Alex Hunsaker
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 ]
commit 87bb2ade2ce646083f39d5ab3e3307490211ad04
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.

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!

Post by Alex Hunsaker
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.

+1.

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

[Alvaro Herrera]

Post by Alex Hunsaker

Post by Greg Sabino Mullane
...

Post by Alex Hunsaker
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 :-)

[Alvaro Herrera]

Post by Alvaro Herrera

Post by Alex Hunsaker

Post by Greg Sabino Mullane
...

Post by Alex Hunsaker
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.

[Alex Hunsaker]

On Mon, May 30, 2011 at 11:02, Alvaro Herrera

Post by Alvaro Herrera

Post by Alvaro Herrera

Post by Alex Hunsaker

Post by Greg Sabino Mullane
...

Post by Alex Hunsaker
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!