Try to handle changes to type definitions

[Changaco]

Fixes #26 (partially, because we can't detect all schema changes).

[chadwhitacre]

Why are we using an xfail here instead of a raises?

[Changaco]

Because ideally it shouldn't raise, but it's a problem we can't fix.

[chadwhitacre]

But xfail will silently swallow all exceptions, no? Seems like we should test for the specific exception we're expecting.

[Changaco]

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 raises argument. Then the test will be reported as a regular failure if it fails with an exception not mentioned in raises.

[chadwhitacre]

Isn't biz the same type as bar? Why is this test "different_type"?

[chadwhitacre]

Sorry, this is grok here, not foo.

[chadwhitacre]

IRC

[Changaco]

Re IRC: a silent failure is when something bad happens but the problem is ignored. It's not the case here, the ValueError is only masked if it can be remedied, if not we re-raise it.

[chadwhitacre]

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?

[chadwhitacre]

Here's CompositeCaster on master.

[chadwhitacre]

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.

[chadwhitacre]

Clarified in 0f422eb.

[chadwhitacre]

I'm partway through a commit to update the documentation.

[chadwhitacre]

And I still haven't satisfied myself on the ways that our parse override could fail.

[chadwhitacre]

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

[chadwhitacre]

Imports clean up in #45.

[chadwhitacre]

I've got docs again locally.

[chadwhitacre]

I'm unraveling what the parse method is about to understand exactly what we're doing with it. I want to understand it and in particular how our implementation of it can fail, so that I can document it properly.

In the CompositeCaster constructor, parse is used as the adapter function, aka, typecaster, for new_type. It's this typecaster that is then passed to register_type underneath register_composite.

[chadwhitacre]

register_type ends up adding the type adapter to a dictionary that is ... somehow referenced when hydrating result sets.

[chadwhitacre]

>>> 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()
[]
>>>

[chadwhitacre]

I'm trying to find where the adapter function parse is called.

[chadwhitacre]

Looks like curs_get_cast dereferences the string_types dict (I'm not sure where binary_types gets looked at) and returns back to psyco_curs_cast, which is where the caster is actually called.

[chadwhitacre]

psyco_curs_cast ends up at Cursor.cast, but I'm not seeing where that's actually called outside of CompositeCaster.parse! Unless what we're getting is the result of curs_get_cast in pqpath? Looks likely, that's where DB-API 2.0's fetch seems to be implemented. Actually, fetch* is implemented in cursor_type.c, and pq_fetch is called under there. Okay!

[chadwhitacre]

So here's the call chain:

• one/all (us)
• fetch* (psycopg2)
• pq_fetch
• curs_get_cast
• parse

[chadwhitacre]

Yeah, binary casts appear to be broken:

FIXME: what the hell am I trying to do here? This just can't work..

:-)

[chadwhitacre]

psycopg/psycopg2#297 💃

[chadwhitacre]

Hmmm ... a little more complicated. Long story short, parse is called inside typecast_cast.

[chadwhitacre]

Alright. I'm back into parse, understanding how it works.

[chadwhitacre]

We have a list of tokens and a list of types. We also have a list of field names.

[chadwhitacre]

Here are the basic ways in which a type can change underneath us:

• a field can be removed
• a field can be added
• a field can change type

An arbitrary number of fields can be simultaneously changed in one of these three ways.

[chadwhitacre]

Changes to the type definition affect both reads and writes.

[chadwhitacre]

@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?

[chadwhitacre]

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?

[chadwhitacre]

More tomorrow ...

[chadwhitacre]

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.

[Changaco]

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.

[Changaco]

I've added a commit to simplify our parse method: it occurred to me that we don't need to duplicate the code of psycopg's parse method like it was suggested in #26 (comment).

[chadwhitacre]

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.

Postgres.py itself isn't supposed to know what the schema is, but:

• Model is intended to be subclassed, and the subclass probably knows what the schema is, especially if it implements update methods.
• Instances of Model subclasses will have their attributes consumed by other code, which will expect a certain schema.

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. :-(

[chadwhitacre]

... though a documentation update could be in order. I can open a new PR for that.

[Changaco]

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.

[chadwhitacre]

Secondly, what this PR (partially) fixes is a bug

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:

• deploy code that supports both the old and new schemas
• deploy the new schema
• deploy code that drops support for the old schema

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.

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

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.

[Changaco]

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?

#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.

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.

You can't avoid downtime without this PR unless the changes only add or drop tables but don't modify any.

[chadwhitacre]

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

[Changaco]

Thanks, better late than never. :-)

[Changaco]

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 bool to int.