Disabling prepared statements for pgbouncer support

[film42]

Hey there!

I see go-kallax is using squirrel's statement cacher, which is a good thing, but will not work when using pgbouncer in transaction pooling mode. The lib/pq maintainers have added support to pass binary_parameters=yes to disable prepared statements for this very reason. However, the statement cacher explicitly uses a prepared statement for each query, which isn't something lib/pq can work around. Would you consider accepting a patch that optionally disables the statement cacher to avoid prepared statements to work with pgbouncer in transaction pooling mode? If so, I'll get right on it. Thanks!

cc @roobre

[nadiamoe]

Hi, thanks for your suggestion.

First of all I'd need some background on what is pgbouncer exactly, and what are the benefits/use cases for it (to get an idea of how it would benefit kallax).

Then I'll need to evaluate how much this would impact kallax' codebase. If it's just setting a flag here and there there would be no problem for me. If we have to manually check for this in several places, then it would be more complicated.

Please tell me what you think about these two points.

[film42]

Hey @roobre . pgbouncer is a database connection pooler. It's very common in ruby shops because ruby on rails tends to opens one DB connection for each request thread, but hardly ever saturates those connections. Since db connections are very expensive, pgbouncer is often used in "transaction pooling" which allows rails to "think" it's speaking with postgres the whole time, but pgbouncer will only checkout a real db connection for the duration of some statement or transaction. There's a catch to transaction pooling though, and that's that prepared statements won't work unless we keep track of them manually and they recommend disabling them on the client side. There are many other reasons shops will us pgbouncer, such as being able to pause connections to failover a db with minimal downtime. We use pgbouncer primarily because of ruby, but the added tools have proved to be very useful.

Since our shop uses pgbouncer with transaction pooling, it would be nice to be able to disable the statement cache (which explicitly uses prepared statements). I'm hoping the implementation is as simple as adding a single flag to

// turn this
	proxy := squirrel.NewStmtCacher(db)
	builder := squirrel.StatementBuilder.PlaceholderFormat(squirrel.Dollar).RunWith(proxy)
// into this
	builder := squirrel.StatementBuilder.PlaceholderFormat(squirrel.Dollar).RunWith(db)

in store.go.

Now how to make the interface simple enough to work with it? I'm not entirely sure. The first thing that popped into my head was adding a Store.DisableStatmentCache method, which would:

1. Set the builder to run with db
2. Add a flag to indicate we're in "no cache mode" so Store.newStoreWithTransaction continues to work properly as well

But I know you're not a big fan of adding exported methods.

P.S. Sorry for the half-way written comment. I accidentally hit ctrl+enter.

[nadiamoe]

Looks like we can simplify the first point like this:

func NewStore(db *sql.DB) *Store {
	builder := squirrel.StatementBuilder.PlaceholderFormat(squirrel.Dollar).RunWith(db)
	if enableCaching { // get this somewhere
		builder = builder.RunWith(squirrel.NewStmtCacher(db))
	}
	return &Store{
		db:      db,
		proxy:   proxy,
		builder: builder,
	}
}

as StatementBuilderType.RunWith() seems to be idempotent. This way, new stores will automatically inherit the proxy (when #254 gets fixed and merged).

Regarding WHERE the flag should be specified... it's more tricky. Of course, adding an additional parameter to NewStore is a no-no. We could, however, add func NewStoreWithCaching(db *sql.DB, enableCaching bool) *Store and make NewStore(db *sql.DB) call NewStoreWithCaching(db, true).

This way we can avoid adding new flags to Store, which I think it shouldn't be avoided as caching is transparent to squirrel.

Tell me what you think about this approach. Take in mind, however, this would have to wait until #254 gets fixed (which will hopefully be soon (this week maybe)).

[nadiamoe]

Added in 75a6119.

[film42]

Woo hoo!

@roobre Looks like you might need to make one more small change here:

go-kallax/store.go

Line 495 in 75a6119

useCacher: true,

to

	txStore := (&Store{
		db:        tx,
		logger:    s.logger,
		useCacher: s.useCacher,
	}).init()

[nadiamoe]

Woops, totally missed it. Should be done right now.

Check if it works for you either by manually pulling master or waiting for the next release (probably tomorrow).

[film42]

@roobre I think we're going to need to add these methods to the generator for the same reasons Debug and DebugWith were added to the generator. Without them something like

var userStore *models.UserStore
userStore = models.NewUserStore(db).DisableCacher()
userStore.WithClient() // etc...

doesn't work.

I can work around it, but it should probably be added before a stable release.

[nadiamoe]

The generator is being looked into due to #248, I'll add the method there after fixing it hopefully tomorrow, probably this week :)