Bug #56744 for DBIx-Class: Issues when using prepare_chached in the presence of pg-bouncer

Tue Apr 20 08:10:15 2010 TIMB [...] cpan.org - Ticket created

Subject:

t/72pg.t test fails: relation "artist_artistid_seq" does not exist [PATCH]

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

Tue Apr 20 08:22:45 2010 TIMB [...] cpan.org - Correspondence added

FYI pg client was 8.4.3 and the server 8.4.2.

Tue Apr 20 08:22:46 2010 TIMB [...] cpan.org - Status changed from 'new' to 'open'

Tue Apr 20 08:49:44 2010 TIMB [...] cpan.org - Correspondence added

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.

Tue Apr 20 11:01:14 2010 TIMB [...] cpan.org - Correspondence added

Subject:

t/72pg.t test fails: relation "artist_artistid_seq" does not exist [needs to be in transaction]

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.

Tue May 04 04:06:08 2010 ribasushi [...] leporine.io - Correspondence added

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?

Wed May 05 05:51:54 2010 TIMB [...] cpan.org - Correspondence added

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.

Wed May 05 07:29:17 2010 ribasushi [...] leporine.io - Correspondence added

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.

Wed May 05 17:09:26 2010 Tim.Bunce [...] pobox.com - Correspondence added

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.

Sat May 08 07:39:32 2010 ribasushi [...] leporine.io - Correspondence added

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

Mon May 31 16:22:51 2010 ribasushi [...] leporine.io - Severity Critical changed to Unimportant

Mon May 31 16:22:51 2010 ribasushi [...] leporine.io - Correspondence added

Downgrading bugreport severity until a robust way for pgbouncer detection is available.

Mon May 31 16:22:52 2010 ribasushi [...] leporine.io - Status changed from 'open' to 'stalled'

Mon May 31 18:07:17 2010 ribasushi [...] leporine.io - Correspondence added

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)

Mon May 31 18:07:18 2010 The RT System itself - Status changed from 'stalled' to 'open'

Sun Sep 12 08:25:01 2010 ribasushi [...] leporine.io - Subject changed from 't/72pg.t test fails: relation "artist_artistid_seq" does not exist [PATCH]' to 'Issues when using prepare_chached in the presence of pg-bouncer'

Thu Jan 13 17:50:49 2011 ribasushi [...] leporine.io - Correspondence added

Blocked by lack of functionality as per RT64756

Thu Jan 13 17:50:50 2011 ribasushi [...] leporine.io - Status changed from 'open' to 'stalled'

Thu Jan 13 17:50:50 2011 ribasushi [...] leporine.io - Dependency on ticket #64756 added