Skip Menu |
 

This queue is for tickets about the DBIx-Class CPAN distribution.

Report information
The Basics
Id: 56744
Status: stalled
Priority: 0/
Queue: DBIx-Class

People
Owner: Nobody in particular
Requestors: TIMB [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Unimportant
Broken in:
  • 0.08120
  • 0.08121
Fixed in: (no value)



Subject: t/72pg.t test fails: relation "artist_artistid_seq" does not exist [PATCH]
Download (untitled) / with headers
text/plain 961b
t/72pg.t ......................................... 2/? DBIx::Class::Storage::DBI::__ANON__(): DBI Exception: DBD::Pg::db do failed: ERROR: relation "artist_artistid_seq" does not exist at t/72pg.t line 403 # Tests were run but no plan was declared and done_testing() was not seen. t/72pg.t ......................................... Dubious, test returned 255 (wstat 65280, 0xff00) [12:28] ribasushi: timbunce++ # looks reasonable, I'll try to purge the db and test again [12:28] ribasushi: timbunce: nobody caught it because we all run things on the same db I posted a possible patch here: http://paste.scsys.co.uk/42462 though I'm not at all sure it's correct. There are no comments to indicate the intent of the 'ambiguous' reference to artist_artistid_seq. Also, after applying that change I now get a different failure (relation "artist" does not exist) that seems unrelated but may share the same origins ("db was stale when developer was testing").
FYI pg client was 8.4.3 and the server 8.4.2.
Download (untitled) / with headers
text/plain 797b
Here's an SQL script that demonstrates the follow-on problem: DROP SCHEMA dbic_t_schema CASCADE; NOTICE: drop cascades to table dbic_t_schema.artist DROP SCHEMA CREATE SCHEMA dbic_t_schema; CREATE SCHEMA CREATE TABLE dbic_t_schema.artist ( artistid serial PRIMARY KEY , name VARCHAR(100)); NOTICE: CREATE TABLE will create implicit sequence "artist_artistid_seq" for serial column "artist.artistid" NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "artist_pkey" for table "artist" CREATE TABLE SET search_path = dbic_t_schema,public; SET SELECT COUNT(*) FROM artist; ERROR: relation "artist" does not exist LINE 1: SELECT COUNT(*) FROM artist; ^ I suspect this connection is going via a proxy that's interfering. I'll get back with an update soon.
Subject: t/72pg.t test fails: relation "artist_artistid_seq" does not exist [needs to be in transaction]
Download (untitled) / with headers
text/plain 1.1k
timbunce: ribasushi & rbuels: turns out the failures I reported in http://paste.scsys.co.uk/42459 are due to talking to a postgresql instance via a pgbouncer proxy. [15:56] ribasushi: timbunce: how is that? [15:57] timbunce: ribasushi: it can route statements (outside of a transaction) to different connections. [15:57] ribasushi: ugh [15:57] ribasushi: so you get different views of what is what [15:57] ribasushi: neat [15:57] timbunce: ribasushi: I haven't investigated the underlying cause beyond the fact that not using pgbouncer makes the tests pass. [15:58] timbunce: ribasushi: I'd guess the key issue of use of 'set search_path' which wouldn't be visible across different connections. [15:58] ribasushi: timbunce: right, which is a part of the test [15:59] timbunce: ribasushi: if it doesn't need to be it would be nice to remove it. [15:59] ribasushi: timbunce: hmmm... I don't see why we can't wrap thise part in transactions though [15:59] timbunce: ribasushi: that'll be fine too. [15:59] ribasushi: timbunce: no, the tests are *testing* what happens if you have a non-standard search_path [15:59] purl: okay, ribasushi. [15:59] rbuels: yeah, those tests could be in transactions i guess.
Download (untitled) / with headers
text/plain 1.7k
On Tue Apr 20 11:01:14 2010, TIMB wrote: Show quoted text
> timbunce: ribasushi & rbuels: turns out the failures I reported in > http://paste.scsys.co.uk/42459 are due to talking to a postgresql > instance via a pgbouncer proxy. > [15:56] ribasushi: timbunce: how is that? > [15:57] timbunce: ribasushi: it can route statements (outside of a > transaction) to different connections. > [15:57] ribasushi: ugh > [15:57] ribasushi: so you get different views of what is what > [15:57] ribasushi: neat > [15:57] timbunce: ribasushi: I haven't investigated the underlying > cause beyond the fact that not using pgbouncer makes the tests pass. > [15:58] timbunce: ribasushi: I'd guess the key issue of use of 'set > search_path' which wouldn't be visible across different connections. > [15:58] ribasushi: timbunce: right, which is a part of the test > [15:59] timbunce: ribasushi: if it doesn't need to be it would be nice > to remove it. > [15:59] ribasushi: timbunce: hmmm... I don't see why we can't wrap > thise part in transactions though > [15:59] timbunce: ribasushi: that'll be fine too. > [15:59] ribasushi: timbunce: no, the tests are *testing* what happens > if you have a non-standard search_path > [15:59] purl: okay, ribasushi. > [15:59] rbuels: yeah, those tests could be in transactions i guess.
It slipped my mind what exactly were we supposed to wrap in transactions... Or did we decide that a Pg.pm needs to be adjusted so that it will recognize pgbouncer and rebless into the storage subclass, which will then deal with whatever caveats there are (not sure what was the exhaustive list either... I think nobody replied to me). How do we move forward to resolve this? Or do we just close the ticket until better days when there is a clear pgbouncer use case?
Download (untitled) / with headers
text/plain 265b
Reblessing and a subclass seems too heavy for this case. A fix would be to use transactions and not reuse a $sth across transactions. Alternatively, just document in the README (eg) the fact that tests will fail if the test db connection is routed via pgbouncer.
Download (untitled) / with headers
text/plain 750b
On Wed May 05 05:51:54 2010, TIMB wrote: Show quoted text
> Reblessing and a subclass seems too heavy for this case.
See below Show quoted text
> A fix would be to use transactions and not reuse a $sth across > transactions.
There is no mechanism for this in DBIC, as we simply rely on DBI's prepare_cached, which in turn means that we don't track which transaction spanned what. So the proper solution would be a subclass of the Pg storage which simply disables use of prepare_cached, so that $sth sharing may not occur. Show quoted text
> Alternatively, just document in the README (eg) the fact that tests > will fail if the test db connection is > routed via pgbouncer.
It's not really about the test itself (which is trivial to fix). It's about using dbic with such a setup as shown above.
CC: TIMB [...] cpan.org
Subject: Re: [rt.cpan.org #56744] t/72pg.t test fails: relation "artist_artistid_seq" does not exist [PATCH]
Date: Wed, 5 May 2010 22:09:08 +0100
To: Peter Rabbitson via RT <bug-DBIx-Class [...] rt.cpan.org>
From: Tim Bunce <Tim.Bunce [...] pobox.com>
On Wed, May 05, 2010 at 07:29:19AM -0400, Peter Rabbitson via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=56744 > > > On Wed May 05 05:51:54 2010, TIMB wrote:
> > Reblessing and a subclass seems too heavy for this case.
> > See below >
> > A fix would be to use transactions and not reuse a $sth across > > transactions.
> > There is no mechanism for this in DBIC, as we simply rely on DBI's > prepare_cached, which in turn means that we don't track which > transaction spanned what. So the proper solution would be a subclass of > the Pg storage which simply disables use of prepare_cached, so that $sth > sharing may not occur. >
> > Alternatively, just document in the README (eg) the fact that tests > > will fail if the test db connection is > > routed via pgbouncer.
> > It's not really about the test itself (which is trivial to fix). It's > about using dbic with such a setup as shown above.
I see your point. I'd suggest you keep using prepare_cached but hook into commit/rollback to clear the cached statement handles when the transaction ends. Tim.
Download (untitled) / with headers
text/plain 1.7k
On Wed May 05 17:09:26 2010, Tim.Bunce@pobox.com wrote: Show quoted text
> On Wed, May 05, 2010 at 07:29:19AM -0400, Peter Rabbitson via RT wrote:
> > <URL: https://rt.cpan.org/Ticket/Display.html?id=56744 > > > > > On Wed May 05 05:51:54 2010, TIMB wrote:
> > > Reblessing and a subclass seems too heavy for this case.
> > > > See below > >
> > > A fix would be to use transactions and not reuse a $sth across > > > transactions.
> > > > There is no mechanism for this in DBIC, as we simply rely on DBI's > > prepare_cached, which in turn means that we don't track which > > transaction spanned what. So the proper solution would be a subclass of > > the Pg storage which simply disables use of prepare_cached, so that $sth > > sharing may not occur. > >
> > > Alternatively, just document in the README (eg) the fact that tests > > > will fail if the test db connection is > > > routed via pgbouncer.
> > > > It's not really about the test itself (which is trivial to fix). It's > > about using dbic with such a setup as shown above.
> > I see your point. I'd suggest you keep using prepare_cached but hook > into commit/rollback to clear the cached statement handles when the > transaction ends. >
Thinking more about this, and consulting with mst it seems that the way forward is: If we are in a transaction - cache prepared statements, and blow up the cachedkids stash on a commit. If we are outside of a transaction (autocommit=>1) simply re-prepare every statement without caching Let me know if you have objections to the above plan. If no - I can implement it, if provided with a reliable way to detect a pg-bouncer environment. If you can contribute a detector for the reblessing mechanism, it'll be all the better. Example: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/0.08/trunk/lib/DBIx/Class/Storage/DBI/Sybase.pm
Downgrading bugreport severity until a robust way for pgbouncer detection is available.
Download (untitled) / with headers
text/plain 1.1k
Un-stalling ticket after more info became available. Final solution is to implement a "flush statement cache on commit" connect_info flag, which can be set on a storage when necessary and keep things sane: <ribasushi> thing is we don't know how to detect such an environment :) <timbunce> Neither do I right now. Also pgbouncer is quite configurable. It might not be possible to detect the config being used. <timbunce> The best approach is probably to add an option to enable a "flush sth cache at end of transaction" mode <timbunce> then let the app specify the option <timbunce> See http://pgbouncer.projects.postgresql.org/doc/usage.html <timbunce> The difference between transaction pooling and statement pooling is the issue. <ribasushi> oooh <ribasushi> "Multi-statement transactions are disallowed in this mode as they would break". <ribasushi> nasty :) <timbunce> so the app would need to ask for sth cache flush at end of tx because we can't detect the need automatically. ie we need an option. <timbunce> Also a "sth cache flush at end of tx" might be of more general use beyond pgbouncer (though I can't think of any cases off-hand)
Blocked by lack of functionality as per RT64756


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.