Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Postgres vacuum prevented #147

Closed
jimfulton opened this issue Dec 13, 2016 · 20 comments

Comments

@jimfulton
Copy link
Member

commented Dec 13, 2016

In #87 there's a mention of connections being left in the "idle in transaction" state which prevents autovacuum from working. That led me to:

https://blog.gocept.com/2012/05/22/dont-stop-postgresqls-autovacuum-with-your-application/

I'm interested because in looking at insert performance:

https://github.com/jimfulton/zodb-jsonb-experiment#insert-performance (Search for vacuum)

It looks like the object_state table wasn't getting vacuumed, presumably because of the idle transaction. Manually doing a full vacuum (because this was a staging DB and I could get an exclusive lock) increased performance 40x.

This appears to be a huge problem.

It seems we should do a commit (or close) when we close ZODB connections. I'd be happy to add a ZODB hook for that. This would address the common case of web processes that close connections when finished.

In the longer term, we could add a ZODB mode that requires explicit transaction begins.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2016

Although as long as all of the clients get restarted at some point, vacuuming up to that point should work. I suppose the database I was looking at had clients running for a very long time. I happened to have restarted them recently, before I realized this was an issue.

@jamadden

This comment has been minimized.

Copy link
Member

commented Dec 15, 2016

Based on Shane's comments, I just assumed that #87 would have alleviated these problems, but I didn't actually check.

Let me see if I understand the situation now:

When the storage has tpc_finish called on it, it goes to _finish where it explicitly rolls back the load connection. Then it commits the store connection, followed by releasing the commit lock (in the store connection). Last, it tells the cache that tpc_finish is done (which doesn't interact with the DB at all).

In postgres, this amounts to:

load_connection.rollback() # rollback load 
store_connection.commit() # commit phase 2
pass # no-op to release the lock, released by commit
pass # no-op to cache

So after tpc_finish, it's not clear to me how we could have a connection idle in a transaction.

The next time load is called, the storage will notice that the load connection has been rolled back and will open it and perform a poll. At that point we should be idle in a transaction.

If no persistent objects are added or updated, and thus the Connection never joins the transaction, then the storage's tpc_finish will never be called, and we will remain idle in a transaction for the load connection.

That is, if a particular transaction is read-only, this state can arise until the next time a transaction is commited that has written something?

Having the Connection alert the storage when it (the connection) is returned to the DB pool (Connection.close) would allow the storage to take action at that point. Interestingly, rolling back the load connection at that point would be a subtle change of semantics: the next load would restart the poll cycle, jumping our MVCC view forward; currently that only happens when a Connection/storage write an object.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2016

@jamadden

This comment has been minimized.

Copy link
Member

commented Dec 15, 2016

Gah, I always forget about afterCompletion. So yes, that calls Connection.newTransaction which calls storage.poll_invalidations which restarts the load connection/transaction. If the Connection is then closed, that transaction sits idle without being used.

Connection.open also calls Connection.newTransaction. Beginning a transaction also calls newTransaction for all registered synchronizers.

The comments in afterCompletion indicate that it is there mostly as an optimization and for compatibility with non-hygeinic applications. Long-term, I wouldn't mind seeing that go away so that transaction boundaries only start when actively using intending for them to do so. But I know that's probably a big change.

(It does seem like there's potentially a lot of extra polling/synching going on, though. Commiting a transaction calls afterCompletion calls newTransaction calls poll_invalidations. The user then begins a new transaction, which calls newTransaction calls poll_invalidations. Or the transaction is commited which calls afterCompletion calls newTransaction calls poll_invalidation. The Connection is then closed and put at the front of the pool, where as soon at is it opened again newTransaction calls poll_invalidations.)

The next time load is called, the storage will notice that the load
connection has been rolled back and will open it and perform a poll. At
that point we should be idle in a transaction.
I don't think that's how it works. But perhaps I'm mistaken, but see above
(for ZODB 5).

From a pure storage perspective, RelStorage assumes it can be in that state. Connection may not use it that way, but the storage code supports it.

In the meantime, knowing that Connection.open calls newTransaction calls poll_invalidations, and knowing that the storage itself will handle other uses, a callback to let the storage know when the connection is being closed and it can rollback its RDBMS connections seems fine to me.

RelStorage today could just rollback the load connection after poll_invalidations and it would notice that and open it again when it was next used to load something, but there's a window for a race condition there where the Connection's PickleCache gets out of sync. If a storage could push invalidations up to the connection, then that could be solved

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2016

@jamadden

This comment has been minimized.

Copy link
Member

commented Dec 15, 2016

It seems a core tenet of RelStorage that it relies on the underlying RDBMS
to provide MVCC. Rolling back after poll would break that.

That's the race condition I was talking about.

  1. add an afterCompletion (feel free to suggest another name) method to
    IMVCCStorage (or a subinterface) that's called from
    Connection.afterCompletion and when a connection is closed.

on_transaction_idle?

So presumably in RelStorage this function would rollback the load connection after we poll, otherwise it doesn't solve the problem.

But if this is called from Connection.afterCompletion, then the currently working "implicit" transaction support is broken by the same race condition we were talking about above, isn't it? I debugged through some examples and can easily produce cases that don't call newTransaction:

conn = db.open() # calls newTransaction
root = conn.root()
transaction.commit() 
# That called afterCompletion/newTransaction. Imagine that we rolled back the load connection there
root['a thing'] = 'a value'
# That called conn.register, which joined a transaction via self.transaction_manager.get().join(self)
# which *does not* result in a call to newTransaction. At some point in the future, 
# if Connection.get/storage.load
# is called, the load connection will get restarted at some point in time
# ahead of what the picklecache has valid data for.

That's why I was suggesting this be done only from Connection.close, because then we're guaranteed to call newTransaction again (from open()) before using the Connection.

But maybe I'm missing something? Are you suggesting that this new hook only be called when the DB is in explicit-transaction mode?

Maybe the problem is that TransactionManager.get() bypasses calling the synchronizers even when it begins a new transaction. I have a vague memory of that being discussed somewhere before.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2016

@jamadden

This comment has been minimized.

Copy link
Member

commented Dec 15, 2016

Does that make sense?

Yes, I see what I had confused now. For one thing, I had the order of steps 1 and 2 backwards. For another thing, I was treating the changes to Connection.afterCompletion is isolation of the changes to Connection.close. They need to happen together (or at least, Connection.close changes still have to happen) to solve the problem.

Thanks for the detailed explanation.

I decided to document this:
zopefoundation/transaction#40

Another thought I had was to add an explicit transaction mode. In that
mode, get would error if called without a preceding begin call.

I had that same thought...

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2016

OK. So I'm going to:

  • Update Connection to call afterCompletion, if present, on the storage as described above.

  • Add an explicit-transaction option to DB. I'll add the seatbelts in Connection so you don't have to in RelStorage.

Later we can talk about adding an explicit transaction mode on the transaction list.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2016

I'll also make a RelStorage PR to add the afterCompletion method. I can tests this with PG to make sure these changes actually fix things. :)

@jamadden

This comment has been minimized.

Copy link
Member

commented Dec 15, 2016

Regarding timeframe: Python 3.6 is expected very soon (at one point it was expected tomorrow, that may have slipped a bit due to a last-minute dict patch, but I don't know). Given that RelStorage 2.0rc1 went out on Monday, I was expecting to do a 2.0 final just after 3.6 becomes available, probably next Monday. However, given that I had hoped this was fixed already, it seems reasonable to include these changes in a 2.0rc2 if you're planning to get to them soon-ish. If not, they can go in the first 2.0 bugfix.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2016

I wouldn't hold 2.0 for this. This issue has existed for a long time.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2017

I just found another reason to fix this. New indexes don't get used is there are any clients have been idle in transaction since before the index was created. Gaaaa.

I need to fix this soon.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

FTR, I'm working on this now. ZODB PR soon.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

@jmuchemb

This comment has been minimized.

Copy link

commented Feb 8, 2017

The comments in afterCompletion indicate that it is there mostly as an optimization and for compatibility with non-hygeinic applications. Long-term, I wouldn't mind seeing that go away so that transaction boundaries only start when actively using intending for them to do so. But I know that's probably a big change.

In the past, I thought the same thing and that's one of the reason I committed Nexedi/erp5@8995b64 in ERP5.

IOW, applications include unit tests, which are naturally written to start transaction implicitly, without issue. I don't think ZODB tests are ready to run in this mode. How do you plan to run them for RelStorage (I didn't check if you reuse them, as we do in NEO) ?

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

See zopefoundation/transaction#43

Explicit mode is optional, so existing tests should be unaffected.

ZODB will continue the current behavior in non-explicit mode.

jimfulton added a commit to zopefoundation/ZODB that referenced this issue Feb 8, 2017
  resources after transaction complete.  See:
  zodb/relstorage#147

- Take advantage of the new transaction-manager explicit mode to avoid
  starting transactions unnecessarily when transactions end.
@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

Smaller ZODB PR:

zopefoundation/ZODB#146

jimfulton added a commit that referenced this issue Feb 8, 2017
RelStorage storages to be notified of transaction endings for
transactions that don't call the two-phase commit API.  This allows
resources to be used more efficiently because it prevents RDBMS
transactions from being held open.

Fixes: #147

(At least for ZODB 5.2.)
@jamadden jamadden closed this in #167 Feb 9, 2017
@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2017

Sharing some experience.

Getting transactions rolled back on close if a win for web application frameworks that close connections at the end of a request. Yay.

But in a legacy pyramid (:-)) app I'm working on, I'm having to deal with daemon processes that tend to keep connections open more or less forever. Sometimes, these frameworks are so helpful, it's hard to change the connection management (or impractical for legacy apps).

I just resorted to:

transaction.manager.explicit = True
root._p_jar.explicit_transactions = True

(and added a begin() call.)

It's not advertized that you can change the explicit setting after construction. I think it should be. Doing so makes it much easier to deal with the standard manager.

I'm thinking that connection.expliciit_transactions should be replaced with a query of the transaction managers's explicit attribute.

@jimfulton

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2017

Note that this also affects mysql with an innodb back end. From:

https://dev.mysql.com/doc/refman/5.7/en/innodb-multi-versioning.html

"Commit your transactions regularly, including those transactions that issue only consistent reads. Otherwise, InnoDB cannot discard data from the update undo logs, and the rollback segment may grow too big, filling up your tablespace."

I bet $.05 that this is an issue for Oracle as well. I suggest adding a note about this to the RelStorage docs. I'll create an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.