Try to handle changes to type definitions#43
Conversation
tests.py
Outdated
There was a problem hiding this comment.
Why are we using an xfail here instead of a raises?
There was a problem hiding this comment.
Because ideally it shouldn't raise, but it's a problem we can't fix.
There was a problem hiding this comment.
But xfail will silently swallow all exceptions, no? Seems like we should test for the specific exception we're expecting.
There was a problem hiding this comment.
I've added the appropriate raises parameter. From pytest's documentation:
If you want to be more specific as to why the test is failing, you can specify a single exception, or a list of exceptions, in the
raisesargument. Then the test will be reported as a regular failure if it fails with an exception not mentioned inraises.
There was a problem hiding this comment.
Isn't biz the same type as bar? Why is this test "different_type"?
There was a problem hiding this comment.
Sorry, this is grok here, not foo.
|
Re IRC: a silent failure is when something bad happens but the problem is ignored. It's not the case here, the |
postgres/__init__.py
Outdated
There was a problem hiding this comment.
Where did we get this line from, and why do we not call it in the normal course of operation? Why are we doing something in this case that we don't otherwise do? Don't we need to configure the class when it's first instantiated? Why am I not seeing that in our code? Is it in psycopg2?
There was a problem hiding this comment.
I see, we're calling _from_db, which is a constructor classmethod, and then we're overwriting all possible attributes on ourself with the corresponding attributes from the new instance.
|
I'm partway through a commit to update the documentation. |
|
And I still haven't satisfied myself on the ways that our |
|
I'm dusting off my postgres.py dev env. I've got env's rebuilt for Python 2 and 3. Hitting an import error though ... |
|
Imports clean up in #45. |
|
I've got docs again locally. |
|
I'm unraveling what the In the |
|
|
>>> psycopg2._ext.string_types.keys()
[3904, 1028, 3905, 1005, 16, 17, 1042, 1043, 20, 21, 3909, 23, 25, 26, 3802, 1182, 1183, 1184, 1185, 1186, 1187, 1700, 3911, 114, 1082, 1083, 700, 701, 704, 705, 3906, 3907, 3908, 1270, 3910, 199, 3912, 3913, 1231, 3926, 3927, 1114, 1115, 3807, 1000, 1001, 1002, 1003, 18, 1006, 1007, 1009, 1266, 19, 1013, 1014, 1015, 1016, 1017, 1021, 1022]
>>> psycopg2._ext.binary_types.keys()
[]
>>> |
|
I'm trying to find where the adapter function |
|
Looks like |
|
|
|
So here's the call chain:
|
|
Yeah, binary casts appear to be broken: :-) |
|
Hmmm ... a little more complicated. Long story short, |
|
Alright. I'm back into |
|
We have a list of tokens and a list of types. We also have a list of field names. |
|
Here are the basic ways in which a type can change underneath us:
An arbitrary number of fields can be simultaneously changed in one of these three ways. |
|
Changes to the type definition affect both reads and writes. |
|
@Changaco Can you remind me why we need to support this? Why don't we just always restart the web app when the schema changes? Convenience? |
|
What if we have an object with an update method and we pass data to it that gets stored in the wrong field because of a schema change between when the object was created and when we updated it? Is that possible? |
|
More tomorrow ... |
|
I mean, now that I revisit this, it strikes me as really weird to try to expect that the Python layer can seamlessly deal with an underlying schema change. |
|
It's not weird to expect postgres.py to keep working when the schema changes, because it's not even supposed to know what the schema is. |
|
I've added a commit to simplify our |
Postgres.py itself isn't supposed to know what the schema is, but:
Any schema update is expected to result in other code changes, in the Model subclass as well as in consumers of subclass instances. If there is no code change, then what was the point of making the schema change? If there is a code change, then of course we need to restart the app. I hate to say it, but I think we should close this PR. :-( |
|
... though a documentation update could be in order. I can open a new PR for that. |
|
Firstly, this PR is useful. It will reduce the downtime of the apps that use postgres.py, because it will no longer be necessary to take them down every time a backwards-compatible change to the DB schema is made, like dropping an unused column, or adding a new column with a default. Secondly, what this PR (partially) fixes is a bug, I don't think it needs any other justification: bugs should be fixed whenever possible. |
Is #26 a bug? The fact that we can only partially fix it seems to be due to the fundamental constraints of a networked database, suggesting that, while it's certainly a constraint, it's not a bug. Can you explain why you see #26 strictly as a bug? Very early on (I just spent some time looking for a relevant commit, but didn't find one) I worked around something like #26 on Gittip (at the time) with a three-step deploy process:
I believe, given Python and Postgres, that's the only possible way to avoid downtime when we want to make a non-backwards-compatible schema change to the database.
I can see the value in that. I still need to satisfy myself that supporting this use case doesn't introduce too powerful a footgun. For the record, I'm being more conservative with this PR than I would for one on Gratipay or Aspen, because postgres.py has already achieved the status of a thoroughly-documented software product in a way that Gratipay and Aspen haven't yet. I want to change postgres.py deliberately and carefully. |
#26 is a caching bug, psycopg caches type definitions but it doesn't refresh them when they've become stale. Does it seem normal to you for a caching system to fail instead of refetching the data from upstream? The fact that we can't fully fix this is not a "fundamental constraint of a networked database", it's just that postgresql wasn't designed to be used the way postgres.py uses it. It might be possible to modify postgresql to make the problem disappear entirely, but even if it is I don't want to wait for it.
You can't avoid downtime without this PR unless the changes only add or drop tables but don't modify any. |
|
Alright, @Changaco, I'm merging this without further review, because a) I'm sure you'll want this for Liberapay, and b) I'm sure you'll quickly fix any bugs we're not seeing with it right now. :-) |
Try to handle changes to type definitions
|
Thanks, better late than never. :-) |
|
FTR I hit a corner case that this PR couldn't fix while deploying Liberapay schema changes today. It was a type change of columns from |
Fixes #26 (partially, because we can't detect all schema changes).