Psycopg psycopg

Comments and changes to this ticket

• 

  Daniele Varrazzo February 23rd, 2012 @ 12:09 AM

  Hi Duncan, thank you for the patch.

  Unfortunately the behaviour you report is common to any unknown data type, for instance:

  >>> cur.execute("select array['<hi/>']::xml[]")
  >>> cur.fetchone()
  ('{<hi/>}',)
  

  since psycopg 2.4.3, we have exposed the array support to python (http://www.initd.org/psycopg/docs/extensions.html#psycopg2.extensio..., so what your patch does can be accomplished just in python without patching psycopg, using:

  >>> XMLARRAY = psycopg2.extensions.new_array_type((143,), "XMLARRAY", psycopg2.STRING)
  >>> psycopg2.extensions.register_type(XMLARRAY)
  >>> cur.execute("select array['<hi/>']::xml[]")
  >>> cur.fetchone()
  (['<hi/>'],)
  

  I don't know if it would be useful to support only inet[], when there are many unsupported types (returned as unparsed strings), and many other are being added (e.g. json). The inet[] case is even worse, because it is actually supported in "extras" (http://www.initd.org/psycopg/docs/extras.html#inet-data-type). I don't know much about the type returned, indeed I don't know if it is useful at all. However, I think register_inet() should register an inet_array as well.

  To summarize:

  1. I don't see the patch as proposed as particularly useful, because it leaves out a lot of types, and chasing after postgres adding new types doesn't seem a good idea

  1.a. If actually it was possible to identify a type as being an array without further lookups it would be great, but I don't think it's possible. that would be a killer patch!

  1. because we already have register_inet(), the right thing would be to add inet[] support to it

  2. we need to document the trick above to create "any array" typecasters. Maybe even create an ANYARRAY typecaster and put it in extensions would be good.

  Back to us: try adding on top of your program:

  import psycopg2.extras
  psycopg2.extras.register_inet()
  psycopg2.extensions.INETARRAY = psycopg2.extensions.new_array_type(
      (1041,), 'INETARRAY', psycopg2.extensions.INET)
  psycopg2.extensions.register_type(psycopg2.extensions.INETARRAY)
  

  after which you'll be able to:

  >>> cur.execute("select array['127.0.0.1']::inet[]")
  >>> cur.fetchone()
  ([Inet('127.0.0.1')],)
  >>> x = _
  >>> print x[0][0]
  127.0.0.1
  

  does the program do what you need? Do you want to provide a patch to automatically add array support to register_inet()?

  If you don't like the above, you can still use::

  psycopg2.extensions.new_array_type((1041,), "INETARRAY", psycopg2.STRING)
  

  on top of the program to do the same of your patch.

  If you fancy patching register_inet(), with tests and docs, and/or mention the array trick somewhere (in the faqs? the new_array_type() docs? Here: http://www.initd.org/psycopg/docs/usage.html#adapt-list? Here: http://www.initd.org/psycopg/docs/advanced.html#type-casting-of-sql-types-into-python-objects?) contributions are welcome. Otherwise I'll fix this stuff myself when I have time. Just tell me what you want to do so I won't wait forever for patches.

  Thank you!

• 

  Daniele Varrazzo February 24th, 2012 @ 12:35 AM

  • State changed from “new” to “resolved”

  Nevermind, all done.

  fixed register_inet() and added plenty of documentation about creating a generic typecaster.

• You flagged this item as spam.
  

  Duncan Burke February 24th, 2012 @ 10:29 AM

  Thankyou very much for your fix!

  My patch was certainly a bit of a half-arsed hack, as it ignored all the other array types without any good reason. I didn't actually know about register_inet(), but that's my fault for not reading all the docs.

  In the meantime, I took a look at the type system, and it seems quite feasible to fix this problem for all array types. The array types can be enumerated by

  grep "array_in array_out array_recv array_send" postgres/src/include/catalog/pg_type.h
  

  Giving:

  DATA(insert OID = 143 ( _xml    PGNSP PGUID -1 f b A f t \054 0 142 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ _null_ ));
  DATA(insert OID = 199 ( _json      PGNSP PGUID -1 f b A f t \054 0 114 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ _null_ ));
  DATA(insert OID = 629 (  _line     PGNSP PGUID  -1 f b A f t \054 0 628 0 array_in array_out array_recv array_send - - - d x f 0 -1 0 0 _null_ _null_ _null_ ));
  DATA(insert OID = 719 (  _circle   PGNSP PGUID  -1 f b A f t \054 0  718 0 array_in array_out array_recv array_send - - - d x f 0 -1 0 0 _null_ _null_ _null_ ));
  DATA(insert OID = 791 (  _money    PGNSP PGUID  -1 f b A f t \054 0  790 0 array_in array_out array_recv array_send - - - d x f 0 -1 0 0 _null_ _null_ _null_ ));
  DATA(insert OID = 1000 (  _bool      PGNSP PGUID -1 f b A f t \054 0    16 0 array_in array_out array_recv array_send - - - i x f 0 -1 0 0 _null_ _null_ _null_ ));
  

  (And so on...)

  From this, the oid for the array type and its base type can be trivially extracted; a few lines of awk or perl could write the entire patch

  In principle I don't see why a few types should be treated specially, especially as they are all equally built in. While some should have a special base typecaster, for most typecast_STRING should suffice.

  I'm intrigued by the idea of automatically identifying a type as an array. However, I believe identifying a type requires access to pg_type.h or postgres.bki. As far as I can tell, this is not exposed in libpq and so cannot be done at runtime. Nonetheless including all the types explicitly remains a practical option.

  Although there seems to be a clear demarcation of the module with any extraneous functionality going into extras or extensions, I think this would be more a case of completing the implementation of existing functionality. In this vein, I find it a bit odd that register_inet exists at all. Was it a case for not wanting to break backwards compatibility? Because it seems to be that the old functionality was more an oversight than a design decision.

  Anyway, I'm willing to patch all the builtin array types in (or as close as I can muster); though it will break backwards compatibility with programs depending on the old (broken?) behaviour.

• 

  Daniele Varrazzo February 24th, 2012 @ 11:23 AM

  > Duncan Burke updated this ticket at February 24th, 2012 @ 10:29 AM

  > In the meantime, I took a look at the type system, and it seems quite feasible to fix this problem for all array types. The array types can be enumerated by > grep "array_in array_out array_recv array_send" postgres/src/include/catalog/pg_type.h > > Giving:

  ... 66 lines on my few months old head

  > From this, the oid for the array type and its base type can be trivially extracted; a few lines of awk or perl could write the entire patch

  Yes, that's not a problem.

  > In principle I don't see why a few types should be treated specially, especially as they are all equally built in. While some should have a special base typecaster, for most typecast_STRING should suffice.

  I just don't want to have psycopg micro-releases every time postgres
  comes out with a new data type, i.e. about at each release. We already
  have the bad habit of introducing small features at micro-releases,
  but keeping track of which psycopg version emits arrays from JSON[]
  and which not becomes a burden.

  > I'm intrigued by the idea of automatically identifying a type as an array. However, I believe identifying a type requires access to pg_type.h or postgres.bki. As far as I can tell, this is not exposed in libpq and so cannot be done at runtime. Nonetheless including all the types explicitly remains a practical option.

  At runtime is very easy to read the catalog and add a typecaster for
  every array type which doesn't have a typecaster yet. It takes an
  extra query, so I wouldn't do it automatically, but it could be a
  function similar to register_hstore().

  > Although there seems to be a clear demarcation of the module with any extraneous functionality going into extras or extensions, I think this would be more a case of completing the implementation of existing functionality. In this vein, I find it a bit odd that register_inet exists at all. Was it a case for not wanting to break backwards compatibility? Because it seems to be that the old functionality was more an oversight than a design decision.

  register_inet() is a freak function I don't even know why exists.
  However it's there and is to be maintained. I wouldn't advice using
  it: a string seems just fine.

  > Anyway, I'm willing to patch all the builtin array types in (or as close as I can muster); though it will break backwards compatibility with programs depending on the old (broken?) behaviour.

  Well, it could be, I don't know what people does out there. No, I
  wouldn't patch everything out of the box: providing a
  "register_array()" sort of function s

• 

  Daniele Varrazzo February 24th, 2012 @ 11:26 AM

  (ops)

  was saying: register_array() seems a better option. We can have a chat on the ML about this functionality (it could take an oid, or none meaning all the ones you find in the catalog).