djangonauts / django-hstore

PostgreSQL HStore support for Django.
http://django-hstore.readthedocs.io/
Other
517 stars 142 forks source link

Made sure hstore is registered on every connection #35

Closed KristianOellegaard closed 10 years ago

KristianOellegaard commented 10 years ago

Hi there,

Please see my mailing list entry: https://groups.google.com/forum/#!topic/django-hstore/sI_3vdrfeP0

If this is not the correct fix, be sure to let me know and I'll adjust it.

Kind regards,

Kristian

nemesifier commented 10 years ago

@niwibe you think there would be any bad consequences in changing that line?

Naddiseo commented 10 years ago

Seems like it could be related to #32

niwinz commented 10 years ago

In my opinion, it has some performance overhead that should be avoided...

It can be related to:

I might be wrong but before merge this change, it should be researched. I will dedicate time for it tomorrow.

In any case, all tests pass, as first impression seems specific config of your application causes this.

fabiocorneti commented 10 years ago

I think that the issue can be solved by deferring registration as shown below, could you test it locally? I'll push this change as soon as Travis will run the test suite against the psycopg2 backend (see #34 ).

def register_hstore_handler(connection, **kwargs):
    # do not register hstore if DB is not postgres
    # do not register if HAS_HSTORE flag is set to false
    if connection.vendor != 'postgresql' or \
       connection.settings_dict.get('HAS_HSTORE', True) is False:
        return

    # if the ``NAME`` of the database in the connection settings is ``None``
    # defer hstore registration by setting up a new unique handler
    if connection.settings_dict['NAME'] is None:
        connection_handler.attach_handler(register_hstore_handler,
                                          vendor="postgresql", unique=True)
        return

    if sys.version_info[0] < 3:
        register_hstore(connection.connection, globally=True, unicode=True)
    else:
        register_hstore(connection.connection, globally=True)

connection_handler.attach_handler(register_hstore_handler,
                                  vendor="postgresql", unique=True)
niwinz commented 10 years ago

Thanks @fabiocorneti An other solution for it, is having hstore installed on template1. This makes hstore oid identical on all databases ;)

But,... is not bad solution for this problem!

KristianOellegaard commented 10 years ago

Closing, seems like #34 will solve this as well.

KristianOellegaard commented 10 years ago

I'm still experiencing the same problem with the newest release (1.2.3). As before, if I change unique=True to unique=False it seems to work. Our app is fairly big and we have several plugin systems that scans INSTALLED_APPS to discover certain files, such as the admin does with admin.py - could it be that these multiple scans trigger multiple register calls and that the last one will end up without a hstore register function?

niwinz commented 10 years ago

yes, it's possible I don't know that is the good solution, Personally I don't like execute this handler on each connection.

In django > 1.6 persistent connections are introduced and it can avoid repeated calls to this but registring the handler with unique=False harms in performance for all django < 1.6 users.

nemesifier commented 10 years ago

is it possible to replicate the issue in the testing environement?

niwinz commented 10 years ago

Hi everyone!

I have reproduced the error!

======================================================================
FAIL: test_properties_hstore (tests.django_hstore_tests.tests.HstoreTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/niwi/devel/django-hstore/tests/django_hstore_tests/tests.py", line 471, in test_properties_hstore
    self.assertEqual(type(instance.data), HStoreDict) # TEST FAILS HERE
AssertionError: <class 'str'> != <class 'django_hstore.fields.HStoreDict'>

The scenario is, having installed first hstore extension in a main database and second installed hstore in a template1/postgis_template

This fails because django makes twice connections, to different databases (i don't know why) and registers incorrect oid for hstore field on first connection.

This on my opinion is not django-hstore bug, because it happens when you have different oids for hstore field in different databases. Because of this known issue, the recommendation for make work properly hstore fields, you should install hstore on template1, and create all databases/templates from template1. This will ensure that all databases have the same hstore field oid and it will avoid strange behaviors like this.

This can not be reproduced in tests because it depends on how you are created hstore extension on postgresql.

KristianOellegaard commented 10 years ago

Hi again,

I don't see why this should not be fixed in django-hstore. After all, it seems to be supported in PostgreSQL to have this configuration.

For instance, it wouldn't be possible to upgrade a production DB if you used a previous version of django-hstore, as far as I can tell.

After all, isn't the whole point of making a library, to make it easier for people to consume the service the library provides?

Kind regards,

Kristian

niwinz commented 10 years ago

Production environments should simply install hstore in a database. I have some production environments with this setup and every thing works well. The problems come when different databases and templates are different hstore oid.

In any case, enabling registring hstore on each connection as default behavior solves problems to one users and add problem to an other users. I don't know how much performance impact is, but is not a good idea register extension on each connection when you can not do it.

My purpose is, put new settings for it, allowing to users that need it, enable registering hstore on each connection. (I make a pull request in five minuts)

nemesifier commented 10 years ago

Another interesting point wrote on the ML by "Artem Kostiuk"

Hey guys,

I have been having the same issue for the last 2 hours. I was using Nose test runner + psycopg2==2.5.2, django-hstore==1.2.3 and postgres 9.1+.

Note, that django-hstore docs say Psycopg 2.4.3+ in their dependencies.

So to the business: downgrading to psycopg2==2.4.3 fixed this issue for me. Tests are running okay and passing now.

nemesifier commented 10 years ago

@KristianOellegaard what is your current setting? psycopg2 version, django version, python version, using multile databases or not, ecc

KristianOellegaard commented 10 years ago

@niwibe A setting would be great for me at least! Fixing how hstore is installed on a large production DB might not be straight forward - also, it allows people to use django-hstore, even if they are not the admin of the database.

@nemesisdesign I tried downgrading to that version without luck - I also tried 5 random version between the latest supported and newest version of psycopg2. I was testing with Django 1.6 and python 2.7. I'm just using a single database.

niwinz commented 10 years ago

@nemesisdesign Any opinion about new settings, allowing users set the default registering behavior? If you agree with it, I make pull request for it with doc and warning in the docs.

nemesifier commented 10 years ago

@niwibe I was thinking the same as @KristianOellegaard, a setting with the default behaviour unchanged would make everyone happy.

There is one little drawback though.. maybe we are not finding the real cause of the issue.. if there's a bug somewhere maybe we should report it to somebody! Any thoughts on this?

niwinz commented 10 years ago

I'll try spend more time investigate this and additionally make the proposed workaround ;)

nemesifier commented 10 years ago

thanks :-)

niwinz commented 10 years ago

Now, on my current environment:

postgres=# \c template_postgis 
You are now connected to database "template_postgis" as user "niwi".
template_postgis=# select oid, typname, typnamespace from pg_type where typname = 'hstore';
  oid   | typname | typnamespace 
--------+---------+--------------
 575999 | hstore  |         2200
(1 row)

template_postgis=# \c django_hstore
You are now connected to database "django_hstore" as user "niwi".
django_hstore=# select oid, typname, typnamespace from pg_type where typname = 'hstore';
  oid   | typname | typnamespace 
--------+---------+--------------
 575879 | hstore  |         2200
(1 row)

Running test with python2.7 and django 1.6 or 1.5 with any version of psycopg2 even with different oid on database and template, works perfectly with unique=True (unique=False obviously fixes it)

But, if I run the same tests but with django 1.7b1 -> b3, the test fails with any version of psycopg2. The unique conclusion that I can get from it, every thing depends on the environment, order on installed apps, I don't know that is really happens :S

This is not django-hstore specific ugly bug, this strange behavior is also happened on old djorm-ext-hstore...

niwinz commented 10 years ago

In any case: https://github.com/djangonauts/django-hstore/pull/40 pull request is ready with new settings for disable global registering ;)

loop0 commented 10 years ago

I'm having a strange bug that seems to be related to this issue. I'm trying to create a simple instance from my model, so I wrote a test that makes a POST to a view that creates this instance and what I observed is that inside the view that creates this instance the type of the field is <class 'django_hstore.fields.HStoreDict'>, but after running this view in the test and fetching the instance again by it's pk inside the test the same field is returned as <type 'str'>.

Like: view: print instance.field results in {"foo": "bar"}

inside the test method: print instance.field results in "foo"=>"bar"

Is it the same bug or am I doing something wrong?

Thanks!

niwinz commented 10 years ago

Is a same bug. Now I have created a pull request with simple workaround for not ideal configurations.

In any case, I'm currently reserarching about it...

Thanks for report your case.

Andrey

Andrey.

Sent from my Nexus 4 On May 8, 2014 7:36 PM, "Bruno Ribeiro da Silva" notifications@github.com wrote:

I'm having a strange bug that seems to be related to this issue. I'm trying to create a simple instance from my model, so I wrote a test that a test that makes a POST to a view that creates this instance and what I observed is that inside the view that creates this instance the type of the field is <class 'django_hstore.fields.HStoreDict'>, but after running this view in the test and fetching the instance again by it's pk inside the test the same field is returned as <type 'str'>.

Like: view: print instance.field results in {"foo": "bar"}

inside the test method: print instance.field results in "foo"=>"bar"

Is it the same bug or am I doing something wrong?

Thanks!

— Reply to this email directly or view it on GitHubhttps://github.com/djangonauts/django-hstore/pull/35#issuecomment-42580477 .

nemesifier commented 10 years ago

for me is ok to merge

loop0 commented 10 years ago

I can confirm that if I create my development database using template1 (hstore already installed) my tests run fine.

niwinz commented 10 years ago

This now should be fixed/workaround with #40 ;)

nemesifier commented 10 years ago

@KristianOellegaard let us know, will publish a new release as soon as i have time