Transaction BEGIN statements should use the query timeout rather than…

[dasl-]

… the transaction pool timeout. According to docs: "query server transaction pool timeout, it is how long vttablet waits if tx pool is full (default 1)". When we execute the BEGIN statment, we have already grabbed a connection from the transaction pool. Thus it does not make sense to use the transaction pool timeout here. Furthermore, this makes BEGIN behave more like COMMIT -- they will both use the query timeout now.

At Etsy, we have our TxPoolTimeout set relatively low (one second), whereas we have an unlimited query timeout. We were seeing timeouts occur here for the BEGIN statement:

vitess/go/vt/vttablet/tabletserver/connpool/dbconn.go

Line 140 in 5019c28

return nil, fmt.Errorf("%v before execution started", ctx.Err())

It's still an open question why one second was not enough (it should have been).

[sougou]

I think you misunderstood: the tabletserver BeginTimeout is (mostly) the time it takes to get a connection from the tx pool. If you drill down into the call graph, you'll see that this timeout is eventually used here: https://github.com/vitessio/vitess/blob/master/go/vt/vttablet/tabletserver/tx_pool.go#L236.

Basically, the Begin call is the one that allocates a connection for the transaction.

SREs wanted a different timeout for this because applications want to fail fast if they're not able to begin a transaction, whereas a query can reasonably take longer to complete because it has to do more work accessing the necessary data.

[dasl-]

@sougou that makes sense. Previously, the BeginTimeout was used for two things:

1. As you mentioned, getting a connection from the transaction pool:

   vitess/go/vt/vttablet/tabletserver/tx_pool.go

   Line 236 in c876709

   conn, err = axp.conns.Get(ctx)

2. It is also used for executing the actual BEGIN statement:

   vitess/go/vt/vttablet/tabletserver/tx_pool.go

   Line 260 in f645c11

   if _, err := conn.Exec(ctx, queries.openTransaction, 1, false); err != nil {

Thanks for pointing out (1) -- I missed that.

The intention of this PR is to use the TxPoolTimeout solely for (1) and to use the query timeout for (2).

[sougou]

I see what you're trying to do, and I like this approach. The improvement I'd suggest is to delete BeginTimeout and use QueryTimeout everywhere.

[sougou]

Nice catch. We shoudl add a test for TransactionTimeout.

[sougou]

And one for TxPoolTimeout here.