Обсуждение: 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!