Made sure hstore is registered on every connection #35

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@KristianOellegaard

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

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign Apr 14, 2014

Member

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

Member

nemesisdesign commented Apr 14, 2014

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

@Naddiseo

This comment has been minimized.

Show comment
Hide comment
@Naddiseo

Naddiseo Apr 14, 2014

Contributor

Seems like it could be related to #32

Contributor

Naddiseo commented Apr 14, 2014

Seems like it could be related to #32

@niwinz

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz Apr 14, 2014

Member

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.

Member

niwinz commented Apr 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@fabiocorneti

fabiocorneti Apr 14, 2014

Contributor

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)
Contributor

fabiocorneti commented Apr 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz Apr 14, 2014

Member

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!

Member

niwinz commented Apr 14, 2014

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!

@niwinz niwinz referenced this pull request Apr 14, 2014

Merged

Django 1.7 compatibility #34

@KristianOellegaard

This comment has been minimized.

Show comment
Hide comment
@KristianOellegaard

KristianOellegaard Apr 15, 2014

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

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

@KristianOellegaard

This comment has been minimized.

Show comment
Hide comment
@KristianOellegaard

KristianOellegaard Apr 22, 2014

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?

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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz Apr 22, 2014

Member

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.

Member

niwinz commented Apr 22, 2014

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.

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign Apr 23, 2014

Member

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

Member

nemesisdesign commented Apr 23, 2014

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

@niwinz

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 4, 2014

Member

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.

Member

niwinz commented May 4, 2014

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

This comment has been minimized.

Show comment
Hide comment
@KristianOellegaard

KristianOellegaard May 4, 2014

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

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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 4, 2014

Member

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)

Member

niwinz commented May 4, 2014

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)

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign May 4, 2014

Member

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.

Member

nemesisdesign commented May 4, 2014

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.

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign May 4, 2014

Member

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

Member

nemesisdesign commented May 4, 2014

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

@KristianOellegaard

This comment has been minimized.

Show comment
Hide comment
@KristianOellegaard

KristianOellegaard May 4, 2014

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

@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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 4, 2014

Member

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

Member

niwinz commented May 4, 2014

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

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign May 4, 2014

Member

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

Member

nemesisdesign commented May 4, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 4, 2014

Member

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

Member

niwinz commented May 4, 2014

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

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign May 4, 2014

Member

thanks :-)

Member

nemesisdesign commented May 4, 2014

thanks :-)

@niwinz

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 4, 2014

Member

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

Member

niwinz commented May 4, 2014

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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 4, 2014

Member

In any case: #40 pull request is ready with new settings for disable global registering ;)

Member

niwinz commented May 4, 2014

In any case: #40 pull request is ready with new settings for disable global registering ;)

@loop0

This comment has been minimized.

Show comment
Hide comment
@loop0

loop0 May 8, 2014

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!

loop0 commented May 8, 2014

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

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 8, 2014

Member

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
.

Member

niwinz commented May 8, 2014

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
.

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign May 8, 2014

Member

for me is ok to merge

Member

nemesisdesign commented May 8, 2014

for me is ok to merge

@loop0

This comment has been minimized.

Show comment
Hide comment
@loop0

loop0 May 8, 2014

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

loop0 commented May 8, 2014

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

@niwinz

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz May 12, 2014

Member

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

Member

niwinz commented May 12, 2014

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

@niwinz niwinz closed this May 12, 2014

@nemesisdesign

This comment has been minimized.

Show comment
Hide comment
@nemesisdesign

nemesisdesign May 12, 2014

Member

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

Member

nemesisdesign commented May 12, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment