Psycopg psycopg

Comments and changes to this ticket

• 

  Daniele Varrazzo February 22nd, 2013 @ 10:17 AM

  It's actually interesting. What I'd do is to add a diag attribute to the exception, that could be used to fetch the relevant attribute, something like:

  try:
      cur.execute('select from nonexisting')
  except psycopg2.Error, e:
      print e.diag.table_name
  

  I wouldn't like to add all the 17 attributes to the exception, nor to read all of them every time an exception is generated. diag would be an exception attribute creating a wrapper object Diagnostics, internally pointing to the exception and calling PQresultErrorField upon attribute access (providing that the exception still points to the cursor and the cursor still contains the PGresult).

  There's a tricky dependency problem: compiling the exception against libpq 9.3 would make the extension not loadable with previous libpq versions. We have already had the same problem with secondary features in the past but I'm not sure I want them again. Maybe some form of dynamic loading would be a better option. I'm tempted to implement the class in Python using ctypes to verify the function exists.

• You flagged this item as spam.
  

  Masklinn February 25th, 2013 @ 10:56 AM

  It's actually interesting. What I'd do is to add a diag attribute to the exception, that could be used to fetch the relevant attribute, something like:

  That's pretty much the way I saw it.

  Maybe have diag be a mapping rather than an object, to make it easier to handle missing fields? Unless they're always set to None on the diag object?

  I wouldn't like to add all the 17 attributes to the exception

  Surely most of them are already used by psycopg and exposed elsewhere are they not? "Future 9.3" only adds the 5 listed above.

  Grepping lists at least PG_DIAG_SQLSTATE used in psycopg/pqpath.c, and some of the error fields are probably used to compose PQresultErrorMessage (all of the PG_DIAG_MESSAGE_* at least, I'd guess)

  There's a tricky dependency problem: compiling the exception against libpq 9.3 would make the extension not loadable with previous libpq versions. We have already had the same problem with secondary features in the past but I'm not sure I want them again. Maybe some form of dynamic loading would be a better option. I'm tempted to implement the class in Python using ctypes to verify the function exists.

  FWIW PQresultErrorField has been introduced in Postgres 7.4 and as noted above psycopg seems to already depend on it unconditionally in pq_raise (psycopg/pqpath.c:180). The one thing which would be missing are the 5 new PG_DIAG_*_NAME defines from postgres_ext.h which could either be re-defined in psycopg or checked for (e.g. with an #ifdef)? The first option would work as PQresultErrorField will return NULL on unknown field codes (as if they were just missing), producing no error.

• 

  Daniele Varrazzo February 25th, 2013 @ 11:33 AM

  Oh, I thought PQresultErrorField was a new api. This makes things easier.

  I'd avoid a mapping as I don't want to query everything on object creation, only on attribute access. The idea is to return None on missing fields, and that can be done dynamically.

• You flagged this item as spam.
  

  Masklinn February 25th, 2013 @ 01:36 PM

  I'd avoid a mapping as I don't want to query everything on object creation, only on attribute access.

  That can also be done on mappings FWIW, but if it's going to return None on missing fields anyway there's not much point to using one.

• You flagged this item as spam.
  

  Matthew Woodcraft March 9th, 2013 @ 09:18 PM

  Last summer, when an earlier version of this Postgres patch was being considered, I had a look at adding support in Psycopg.

  If you haven't started work on this, I'd be happy to try reworking what I had along the lines you're describing here.

  One thing I'm not sure about with the design you're describing is when PQclear would be called. At present Psycopg usually calls it immediately after pq_raise() returns, but that would make the new error fields inaccessible.

  Would it be better to have the psycopg2.Error instance hold a reference to the PGresult (and call PQclear when the instance is destroyed)?

• 

  Daniele Varrazzo March 16th, 2013 @ 05:44 PM

  The exception already contains a pointer to the cursor, hence it can reach the PQresult.

  If we need to keep the PQresult upon error longer than we currently do I think it's ok: I assume a PQresult containing an error doesn't contain so much data; it will be cleaned at the next use of the cursor, or at its destruction.

• You flagged this item as spam.
  

  Matthew Woodcraft March 17th, 2013 @ 11:48 AM

  Ok, that makes sense. I will have a go at writing it that way.

• 

  Daniele Varrazzo March 17th, 2013 @ 02:51 PM

  Are you planning to work on that in the next days? Because I'd like to release 2.5 soon, the code is actually ready. But I can wait a few days if you want to add that feature, otherwise we can wait for a later release.

• You flagged this item as spam.
  

  Matthew Woodcraft March 17th, 2013 @ 03:15 PM

  I have something basically working now, using the design you described. If you like I'll publish what I've got so you can judge whether it looks suitable, and how much more work it will need.

  Would you prefer me to post it to the mailing list or put it on github or something else?

• 

  Daniele Varrazzo March 17th, 2013 @ 04:31 PM

  Great! Please push it on github.

• You flagged this item as spam.
  

  Matthew Woodcraft March 17th, 2013 @ 05:14 PM

  Ok, I've pushed it to a 2013-03-17_diagnostics branch in mattheww/psycopg2 on github:
  https://github.com/mattheww/psycopg2/tree/2013-03-17_diagnostics

  It's based on the devel branch from psycopg/psycopg2 .

  It doesn't include documentation, but there is one test.

  The implementation is roughly equivalent to the following almost-Python code:

  def _diagnostics_get_field(diag, fieldcode):
      return PQresultErrorField(diag.err.cursor->pgres, fieldcode)
  
  class Diagnostics(object):
      def __init__(self, err):
          self.err = err
  
      @property
      def schema_name(self):
          return _diagnostics_get_field(PG_DIAG_SCHEMA_NAME)
  
      @property
      def table_name(self):
          return _diagnostics_get_field(PG_DIAG_TABLE_NAME)
  
      @property
      def column_name(self):
          return _diagnostics_get_field(PG_DIAG_COLUMN_NAME)
  
      @property
      def _datatype_name(self):
          return _diagnostics_get_field(PG_DIAG_DATATYPE_NAME)
  
      @property
      def constraint_name(self):
          return _diagnostics_get_field(PG_DIAG_CONSTRAINT_NAME)
  
  _psycopg.Error.diag = property(Diagnostics)
  

  I've only included the five new 'enhanced error fields', but it should be easy enough to add properties for the other PG_DIAG_* fields if you would like them.

  I've only built and tested on Python 2.7, against a recent PostgreSQL 9.3 development build.

  I've only tried simple curs.execute() calls. I haven't looked yet at whether things like COPY or async calls need to be told not to call PQclear on error.

  I haven't made the diagnostics object Py_TPFLAGS_HAVE_GC, but maybe it ought to be.

  I'm not sure whether I need to use conn_text_from_chars() on the string values (I'm not sure yet whether the error field values are in the connection's client encoding).

• 

  Daniele Varrazzo March 17th, 2013 @ 07:38 PM

  Thank you very much, great work.

  I've only included the five new 'enhanced error fields', but it should be easy enough 
  to add properties for the other PG_DIAG_* fields if you would like them.
  

  Yes, I think Diagnostics should expose all the available fields

  I've only built and tested on Python 2.7, against a recent PostgreSQL 9.3 development build.
  

  I will run the entire test grid before committing it.

  I've only tried simple curs.execute() calls. I haven't looked yet at whether things like COPY
  or async calls need to be told not to call PQclear on error.
  

  It probably deserves more testing

  I haven't made the diagnostics object Py_TPFLAGS_HAVE_GC, but maybe it ought to be.
  

  I'll look into that, I need to check the docs again about that.

  I'm not sure whether I need to use conn_text_from_chars() on the string values
  (I'm not sure yet whether the error field values are in the connection's client encoding).
  

  Yes, I'm pretty sure it will be in the connection encoding.

  Do you want to tackle any of these points or do you want me take a look at them?

• You flagged this item as spam.
  

  Matthew Woodcraft March 17th, 2013 @ 09:45 PM

  I hope to do some more work on it tomorrow. But of course I won't have any complaints if you fix things yourself.

• 

  Daniele Varrazzo March 18th, 2013 @ 12:41 AM

  I've done much of the above:

  • all the attributes exposed and tested
  • test after COPY
  • test with Python 3 (decoded from conn encoding)
• 

  Daniele Varrazzo March 18th, 2013 @ 02:26 AM

  • State changed from “new” to “resolved”

  I've reviewed the GC interaction and added documentation: this should complete the work.

  I've merged the branch into my devel to run the test with all the versions. If you have any comment let me know. Thank you very much!

• 

  Daniele Varrazzo March 18th, 2013 @ 12:22 PM

  Documentation is here:

  • http://initd.org/psycopg/docs/module.html#psycopg2.Error.diag
  • http://initd.org/psycopg/docs/extensions.html#psycopg2.extensions.D...

  If you spot anything wrong let me know, thanks.

• You flagged this item as spam.
  

  Matthew Woodcraft March 18th, 2013 @ 07:40 PM

  Looks good to me. Perhaps it's worth saying in the docs that the PG_DIAG_*_NAME fields won't be available unless the server is PostgreSQL 9.3 or later? (Also, I suppose in principle it's possible that those fields might end up changing in the final 9.3 release, though I don't imagine it's likely.)

• You flagged this item as spam.
  

  Matthew Woodcraft March 18th, 2013 @ 07:51 PM

  I do have one other small worry: it's possible for a cursor to be re-used after an error, so in principle 'diag' from an existing Error object could end up reporting the wrong data.

  It's hard to think of a very plausible example, but the following code shows what I mean:

  conn = psycopg2.connect("dbname=test")
  conn.autocommit = True
  curs = conn.cursor()
  
  curs.execute("create temp table log (message varchar)")
  
  def log(msg):
      curs.execute("insert into log values (%s)", (msg,))
  
  try:
      curs.execute("select * from nonexist")
  except psycopg2.DatabaseError, e:
      print e.diag.message_primary
      log(str(e))
      print e.diag.message_primary
  

  I hope nobody writes code like this, but maybe it's worth some mention in the documentation.

• 

  Daniele Varrazzo March 18th, 2013 @ 09:20 PM

  Yes, I've thought about this shortcoming. We may be more explicit in the docs. There's also a 'mark' field in the connection/cursor: not sure it's updated every time the cursors run a query but it could be improved. We could copy the connection mark into the diag and always return None if the cursor has executed another query. I'll take a look at it.

• You flagged this item as spam.
  

  Matthew Woodcraft March 18th, 2013 @ 09:50 PM

  I think diagnostics aren't working for errors which are raised from COMMIT. I suppose the PQresult is being cleared too early, but I'm not sure where.

  This test is failing for me:

  @skip_before_postgres(9, 3)
  def test_error_from_commit(self):
      cur = self.conn.cursor()
      cur.execute("""
          create temp table test_deferred (
             data int constraint data_uq unique deferrable initially deferred
          )""")
      cur.execute("insert into test_deferred values (1), (1)")
      try:
          self.conn.commit()
      except psycopg2.Error, exc:
          e = exc
      self.assertEqual(e.pgcode, '23505')
      self.assertEqual(e.diag.constraint_name, "data_uq")
  

• 

  Daniele Varrazzo March 18th, 2013 @ 10:02 PM

  Probably in pq_execute_command_locked, https://github.com/dvarrazzo/psycopg/blob/devel/psycopg/pqpath.c#L372

• 

  Daniele Varrazzo March 18th, 2013 @ 10:05 PM

  The test you are writing is not specific for PG 9.3: I'd drop the check for constraint_name, leave e.g. e.diag.sqlstate, which is present in all postgres versions.

• You flagged this item as spam.
  

  Matthew Woodcraft March 18th, 2013 @ 10:26 PM

  OK.

  I think now that the problem is that, because commit is called directly on the connection, there is no cursor associated with the exception. So I'm not sure how to make this work.

• 

  Daniele Varrazzo March 18th, 2013 @ 11:49 PM

  Oh well, that's true. Probably the idea of going diag->exception->cursor->PGresult is not so great after all.

  Because typically after the exception the result is destroyed (both in the cursor, IIRC, and in pq_execute_command_locket), we could move its ownership to the exception instead: this way we can get a diag for exceptions raised everywhere and we don't have the problem of clobbering the PGresult with later queries on the same cursor.

• 

  Daniele Varrazzo March 19th, 2013 @ 12:15 AM

  Adding PQresult to the exception requires the Error exception to have a C structure (a PsycoErrorObject): I'm taking a look at doing it, using the Python SystemError as an example.

• You flagged this item as spam.
  

  Matthew Woodcraft March 19th, 2013 @ 12:19 AM

  Thanks. One possible complication I thought of is that the Diagnostics still wants to see a connection in order to call conn_text_from_chars(); I suppose that pointer could go in the PsycoErrorObject too.

• 

  Daniele Varrazzo March 19th, 2013 @ 03:41 AM

  The connection is only used to get the encoding: we can store that too in the exception.

  I'm getting crazy subtyping StandardError... enough for tonight.

• 

  Daniele Varrazzo March 19th, 2013 @ 11:51 PM

  Fixed the problem: the exception now contains the PGresult. When the exception is created it takes ownership of the result, which is disposed of only when the exception is destroyed. Now diag doesn't need the cursor at all (nor the connection) so its attribute are independent from further actions on the cursor and can also be raised when there is no cursor at all, e.g. on commit.