Skip Menu |

This queue is for tickets about the DBD-Pg CPAN distribution.

Report information
The Basics
Id: 68893
Status: resolved
Priority: 0/
Queue: DBD-Pg

People
Owner: greg [...] turnstep.com
Requestors: DDICK [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: 2.18.1
Fixed in: 3.0.0



Subject: PATCH: Allow pg_server_prepare with AutoInactiveDestroy
The attached patch allows for DBD::Pg to specify pg_server_prepare and AutoInactiveDestroy. Without this patch, DBD::Pg dies with a message indicating that "ERROR: prepared statement "$statement_id" does not exist" after any fork if AutoInactiveDestroy is set. Test coverage proving patch works is included.
Subject: dbdpg_auto_inactive_destroy.patch
diff -Naur old/dbdimp.c new/dbdimp.c --- old/dbdimp.c 2011-05-10 01:42:15.000000000 +1000 +++ new/dbdimp.c 2011-06-17 20:32:56.000000000 +1000 @@ -3664,6 +3664,16 @@ if (NULL == imp_sth->seg) /* Already been destroyed! */ croak("dbd_st_destroy called twice!"); + /* If the AutoInactiveDestroy flag has been set, we go no further */ + if (DBIc_AIADESTROY(imp_dbh)) { + if (TRACE4) { + TRC(DBILOGFP, "%sskipping sth destroy due to AutoInactiveDestroy\n", THEADER); + } + DBIc_IMPSET_off(imp_sth); /* let DBI know we've done it */ + if (TEND) TRC(DBILOGFP, "%sEnd dbd_st_destroy (AutoInactiveDestroy set)\n", THEADER); + return; + } + /* If the InactiveDestroy flag has been set, we go no further */ if (DBIc_IADESTROY(imp_dbh)) { if (TRACE4) { diff -Naur old/Makefile.PL new/Makefile.PL --- old/Makefile.PL 2011-05-10 01:53:37.000000000 +1000 +++ new/Makefile.PL 2011-06-17 20:32:56.000000000 +1000 @@ -203,7 +203,7 @@ ABSTRACT => 'PostgreSQL database driver for the DBI module', PREREQ_PM => { 'ExtUtils::MakeMaker' => '6.11', - 'DBI' => '1.52', + 'DBI' => '1.614', 'Test::More' => '0.61', 'version' => '0', }, diff -Naur old/Pg.pm new/Pg.pm --- old/Pg.pm 2011-05-10 01:53:37.000000000 +1000 +++ new/Pg.pm 2011-06-17 20:32:56.000000000 +1000 @@ -2196,6 +2196,13 @@ rewrite your application not to use use forking. See the section on L</Asynchronous Queries> for a way to have your script continue to work while the database is processing a request. +=head3 B<AutoInactiveDestroy> (boolean) + +The InactiveDestroy attribute, described above, needs to be explicitly set in the child +process after a fork. If the code that performs the fork is in a third party module such +as Sys::Syslog, this can present a problem. Use AutoInactiveDestroy to get around this +problem. + =head3 B<RaiseError> (boolean, inherited) Forces errors to always raise an exception. Although it defaults to off, it is recommended that this diff -Naur old/t/02attribs.t new/t/02attribs.t --- old/t/02attribs.t 2011-02-25 12:12:14.000000000 +1100 +++ new/t/02attribs.t 2011-06-17 20:33:39.000000000 +1000 @@ -18,7 +18,7 @@ if (! $dbh) { plan skip_all => 'Connection to database failed, cannot continue testing'; } -plan tests => 249; +plan tests => 260; isnt ($dbh, undef, 'Connect to database for handle attributes testing'); @@ -101,6 +101,7 @@ a Profile (not tested) a ReadOnly +d AutoInactiveDestroy (must be the last one tested) d InactiveDestroy (must be the last one tested) }; @@ -1539,6 +1540,79 @@ is ($attrib, '', $t); SKIP: { + skip ('Cannot test database handle "AutoInactiveDestroy" on a non-forking system', 8) + if $^O =~ /Win/; + + require Test::Simple; + + skip ('Test::Simple version 0.47 or better required for testing of attribute "AutoInactiveDestroy"', 8) + if $Test::Simple::VERSION < 0.47; + + # Test of forking. Hang on to your hats + + my $answer = 42; + $SQL = "SELECT $answer FROM dbd_pg_test WHERE id > ? LIMIT 1"; + + for my $destroy (0,1) { + + $dbh = connect_database({nosetup => 1, AutoCommit => 1 }); + $dbh->{'AutoInactiveDestroy'} = $destroy; + $dbh->{'pg_server_prepare'} = 1; + $sth = $dbh->prepare($SQL); + $sth->execute(1); + $sth->finish(); + + # Desired flow: parent test, child test, child kill, parent test + + if (fork) { + $t=qq{Parent in fork test is working properly ("AutoInactiveDestroy" = $destroy)}; + $sth->execute(1); + $val = $sth->fetchall_arrayref()->[0][0]; + is ($val, $answer, $t); + # Let the child exit first + select(undef,undef,undef,0.3); + } + else { # Child + select(undef,undef,undef,0.1); # Age before beauty + exit; ## Calls disconnect via DESTROY unless AutoInactiveDestroy set + } + + if ($destroy) { + $t=qq{Ping works after the child has exited ("AutoInactiveDestroy" = $destroy)}; + ok ($dbh->ping(), $t); + + $t='Successful ping returns a SQLSTATE code of 00000 (empty string)'; + my $state = $dbh->state(); + is ($state, '', $t); + + $t='Statement handle works after forking'; + $sth->execute(1); + $val = $sth->fetchall_arrayref()->[0][0]; + is ($val, $answer, $t); + } + else { + $t=qq{Ping fails after the child has exited ("AutoInactiveDestroy" = $destroy)}; + is ( $dbh->ping(), 0, $t); + + $t='Failed ping returns a SQLSTATE code of 08000'; + my $state = $dbh->state(); + is ($state, '08000', $t); + + $t=qq{pg_ping gives an error code of -2 after the child has exited ("AutoInactiveDestroy" = $destroy)}; + is ( $dbh->pg_ping(), -2,$t); + ok ($dbh->disconnect(), 'Disconnect from database'); + } + } +} + +# Disconnect in preparation for the fork tests +ok ($dbh->disconnect(), 'Disconnect from database'); + +$t='Database handle attribute "Active" is false after disconnect'; +$attrib = $dbh->{Active}; +is ($attrib, '', $t); + +SKIP: { skip ('Cannot test database handle "InactiveDestroy" on a non-forking system', 8) if $^O =~ /Win/;
On Fri Jun 17 06:39:51 2011, DDICK wrote: Show quoted text
> The attached patch allows for DBD::Pg to specify pg_server_prepare and > AutoInactiveDestroy. Without this patch, DBD::Pg dies with a message > indicating that "ERROR: prepared statement "$statement_id" does not > exist" after any fork if AutoInactiveDestroy is set. Test coverage > proving patch works is included.
Smaller patch provided
Subject: dbdpg_auto_inactive_destroy2.patch
diff -Naur old/dbdimp.c new/dbdimp.c --- old/dbdimp.c 2011-05-10 01:42:15.000000000 +1000 +++ new/dbdimp.c 2011-06-18 09:01:16.000000000 +1000 @@ -3664,8 +3664,8 @@ if (NULL == imp_sth->seg) /* Already been destroyed! */ croak("dbd_st_destroy called twice!"); - /* If the InactiveDestroy flag has been set, we go no further */ - if (DBIc_IADESTROY(imp_dbh)) { + /* If the InactiveDestroy or AutoInactiveDestroy flag has been set, we go no further */ + if (DBIc_IADESTROY(imp_dbh) || DBIc_AIADESTROY(imp_dbh)) { if (TRACE4) { TRC(DBILOGFP, "%sskipping sth destroy due to InactiveDestroy\n", THEADER); } diff -Naur old/Makefile.PL new/Makefile.PL --- old/Makefile.PL 2011-05-10 01:53:37.000000000 +1000 +++ new/Makefile.PL 2011-06-18 09:01:46.000000000 +1000 @@ -203,7 +203,7 @@ ABSTRACT => 'PostgreSQL database driver for the DBI module', PREREQ_PM => { 'ExtUtils::MakeMaker' => '6.11', - 'DBI' => '1.52', + 'DBI' => '1.614', 'Test::More' => '0.61', 'version' => '0', }, diff -Naur old/Pg.pm new/Pg.pm --- old/Pg.pm 2011-05-10 01:53:37.000000000 +1000 +++ new/Pg.pm 2011-06-18 09:02:52.000000000 +1000 @@ -2196,6 +2196,12 @@ rewrite your application not to use use forking. See the section on L</Asynchronous Queries> for a way to have your script continue to work while the database is processing a request. +=head3 B<AutoInactiveDestroy> (boolean) + +The InactiveDestroy attribute, described above, needs to be explicitly set in the child +process after a fork. If the code that performs the fork is in a third party module, this +can present a problem. Use AutoInactiveDestroy to get around this problem. + =head3 B<RaiseError> (boolean, inherited) Forces errors to always raise an exception. Although it defaults to off, it is recommended that this diff -Naur old/t/02attribs.t new/t/02attribs.t --- old/t/02attribs.t 2011-02-25 12:12:14.000000000 +1100 +++ new/t/02attribs.t 2011-06-18 10:10:31.000000000 +1000 @@ -18,7 +18,7 @@ if (! $dbh) { plan skip_all => 'Connection to database failed, cannot continue testing'; } -plan tests => 249; +plan tests => 257; isnt ($dbh, undef, 'Connect to database for handle attributes testing'); @@ -101,6 +101,7 @@ a Profile (not tested) a ReadOnly +d AutoInactiveDestroy d InactiveDestroy (must be the last one tested) }; @@ -1539,12 +1540,12 @@ is ($attrib, '', $t); SKIP: { - skip ('Cannot test database handle "InactiveDestroy" on a non-forking system', 8) + skip ('Cannot test database handle "InactiveDestroy" or "AutoInactiveDestroy" on a non-forking system', 16) if $^O =~ /Win/; require Test::Simple; - skip ('Test::Simple version 0.47 or better required for testing of attribute "InactiveDestroy"', 8) + skip ('Test::Simple version 0.47 or better required for testing of attribute "InactiveDestroy" and "AutoInactiveDestroy', 16) if $Test::Simple::VERSION < 0.47; # Test of forking. Hang on to your hats @@ -1552,9 +1553,18 @@ my $answer = 42; $SQL = "SELECT $answer FROM dbd_pg_test WHERE id > ? LIMIT 1"; + for my $attribute ('AutoInactiveDestroy','InactiveDestroy') { for my $destroy (0,1) { $dbh = connect_database({nosetup => 1, AutoCommit => 1}); + if ($attribute eq 'AutoInactiveDestroy') { + $dbh->{'AutoInactiveDestroy'} = $destroy; + unless ($dbh->{'pg_server_prepare'}) { + die("The pg_server_prepare attribute should be set above"); + } + } else { + $dbh->{'AutoInactiveDestroy'} = undef; + } $sth = $dbh->prepare($SQL); $sth->execute(1); $sth->finish(); @@ -1562,7 +1572,7 @@ # Desired flow: parent test, child test, child kill, parent test if (fork) { - $t=qq{Parent in fork test is working properly ("InactiveDestroy" = $destroy)}; + $t=qq{Parent in fork test is working properly ("$attribute" = $destroy)}; $sth->execute(1); $val = $sth->fetchall_arrayref()->[0][0]; is ($val, $answer, $t); @@ -1570,13 +1580,15 @@ select(undef,undef,undef,0.3); } else { # Child - $dbh->{InactiveDestroy} = $destroy; + if ($attribute eq 'InactiveDestroy') { + $dbh->{InactiveDestroy} = $destroy; + } select(undef,undef,undef,0.1); # Age before beauty exit; ## Calls disconnect via DESTROY unless InactiveDestroy set } if ($destroy) { - $t=qq{Ping works after the child has exited ("InactiveDestroy" = $destroy)}; + $t=qq{Ping works after the child has exited ("$attribute" = $destroy)}; ok ($dbh->ping(), $t); $t='Successful ping returns a SQLSTATE code of 00000 (empty string)'; @@ -1589,17 +1601,18 @@ is ($val, $answer, $t); } else { - $t=qq{Ping fails after the child has exited ("InactiveDestroy" = $destroy)}; + $t=qq{Ping fails after the child has exited ("$attribute" = $destroy)}; is ( $dbh->ping(), 0, $t); $t='Failed ping returns a SQLSTATE code of 08000'; my $state = $dbh->state(); is ($state, '08000', $t); - $t=qq{pg_ping gives an error code of -2 after the child has exited ("InactiveDestroy" = $destroy)}; + $t=qq{pg_ping gives an error code of -2 after the child has exited ("$attribute" = $destroy)}; is ( $dbh->pg_ping(), -2,$t); } } + } } cleanup_database($dbh,'test');
Thanks for the patch. Why the raised DBI version requirement?
On Fri Feb 08 22:38:18 2013, TURNSTEP wrote: Show quoted text
> Thanks for the patch. Why the raised DBI version requirement?
1.614 was the DBI version that introduced AutoInactiveDestroy.
Show quoted text
> 1.614 was the DBI version that introduced AutoInactiveDestroy.
Ah ok, thanks.
Is issue #83882 related to this one?
On Mon Sep 30 17:21:23 2013, DWHEELER wrote: Show quoted text
> Is issue #83882 related to this one?
Yes. I deleted that one. This one should cover the testing.
Pushed in d6e7a6788a50e281b116268f231d9c883fec1d7e. Note that I switched the test from using pg_server_prepare of 1 to a pg_server_prepare of 0. I hope this is the expected behavior. Can I get a David to confirm that?
On Sat Nov 16 21:31:38 2013, TURNSTEP wrote: Show quoted text
> Pushed in d6e7a6788a50e281b116268f231d9c883fec1d7e. > > Note that I switched the test from using pg_server_prepare of 1 > to a pg_server_prepare of 0. I hope this is the expected behavior. > Can I get a David to confirm that? >
Hi Greg, This patch was intended to allow AutoInactiveDestroy to work when pg_server_prepare was set to "1", this is why that pg_server_prepare was set to "1" in the t/02attribs.t. To provide a good regression test, i think the pg_server_prepare setting should be restored back to "1".
Show quoted text
> This patch was intended to allow AutoInactiveDestroy to work when > pg_server_prepare was set to "1", this is why that pg_server_prepare > was set to "1" in the t/02attribs.t. To provide a good regression > test, i think the pg_server_prepare setting should be restored back to > "1".
Ah, okay. I was wondering if it wasn't reversed as 1 is the default, and the test fails when set to 1 :) I'll poke around a little bit, and restore it to 1 in the test.
Now the test is working as intended! Go figure. Going to mark this as patched for now, and we can re-open this if it breaks again. Thanks for the patch and the test!