search

kaleidos / grails-postgresql-extensions

Grails plugin to use postgresql native elements such as arrays, hstores,...
Apache License 2.0
78 stars 63 forks source link

Fixes #30 #117

Closed butters16 closed 5 years ago

butters16 commented 5 years ago

Admittedly, the placeholders are a bit wonky, but the best way I could think to not touch the iterator logic.

butters16 commented 5 years ago

@ilopmar Can you please let me know if this is likely to be pulled? My company is going to work from a local fork in the meantime, but it our obvious long-term goal is to get back to this repo. Also, it would be nice to know whether the solution is feasible in the eyes of the maintainers. If there are flaws, I'd be willing to try again. Thanks in advance.

ilopmar commented 5 years ago

Sorry to taking so long in responding. Are this changes backwards compatible or this breaks the compatibility? Honestly since we added support for json and specially for jsonb I've never used hstore anymore. I have everything I need with jsonb.

butters16 commented 5 years ago

We are actually using org.grails.plugins:postgresql-extensions:4.6.8 and org.postgresql:postgresql:42.0.0. My PR is against master because we expect to update our library dependencies soon and would likely use org.grails.plugins:postgresql-extensions:6.x. Does that and the fact they have unit tests answer your question or do you need me to do further investigation against something specific? Thanks again.

ilopmar commented 5 years ago

I'm asking because you're modifying how the backslash and the quotes are stored in the db, so I think this is a breaking change and I need to be careful with it. If you plan to upgrade to 6.x to have support for Hibernate 5.2, I can merge this into master and then release 6.1.0. Keep in mind that 4.6.8 is for Hibernate 4 so until you upgrade you won't be able to have your patch applied. If you need it I can also apply the changes to that branch and release 4.8.0. Let me know what do you prefer.

butters16 commented 5 years ago

Forgive my ignorance, but I didn't understand that this parser class was modifying anything in the db. My intent was to work with an in-memory representation and treat the backslashes as part of the next token (when that token is the double-quote or backslash). I thought the scope of this solution was only for the caller. If there is a larger application of this parser that I'm missing please let me know.

To answer your question, it would be great if you could merge this into a 6.1.0 release as well as a 4.8.0 release, but I don't want to be greedy. If I had to pick one, I prefer 6.1.0 so there is at least an eventual migration plan back to this repo. Thanks.

jglapa commented 5 years ago

Hi, I'm the one who raised the issue. I'm just wondering if using regexp here won't have a negative impact on the performance. I use this extension together with hstore extensively and some of them are pretty large. I fear this could significantly slow it down. Also the patterns are not compiled once but on every set?

ilopmar commented 5 years ago

@butters16 I'm sorry, you're right nothing changes on the database. It's been a while since I used this last time. @jglapa Do you have the possibility to make some performance test? I think I can release a BUILD-SNAPSHOT release so you can try it out in your application.

jglapa commented 5 years ago

@jglapa Do you have the possibility to make some performance test? I think I can release a BUILD-SNAPSHOT release so you can try it out in your application.

I could benchmark a bit... but right now I'm stuck with version 5.1.0 and Grails 3.2.11. From what I see I could upgrade most likely to 5.2.0 but 6.0 is out of question now due to Hibernate upgrade.

ilopmar commented 5 years ago

The change doesn't seem to complicated so I can ported also to that branch and release 5.3.0 so you can try it. Give me a few days and I'll try to release a version for you.

jglapa commented 5 years ago

The change doesn't seem to complicated so I can ported also to that branch and release 5.3.0 so you can try it. Give me a few days and I'll try to release a version for you.

:ok_hand:

butters16 commented 5 years ago

I'm just wondering if using regexp here won't have a negative impact on the performance.

Good suggestion, String.replace will work just as well, so I'll switch to that and update the PR. You got me thinking about the performance impacts, so I ran my own simple test. Here's a summary of what I found (averaged over 1 million executions, see below):

So, this change does make the processing 6-7 times slower, but we are talking about the microsecond level, so that seems acceptable to me. I was using a very small example (see below), so maybe @jglapa can help stress test it. The alternative would be to modify the HStoreIterator code, but that seems dangerous without any existing tests.

Also the patterns are not compiled once but on every set?

I tried this as well, but it didn't make an improvement at 2.960 μs. It's possible something else is optimizing here, so I don't plan to make this change.

Here's what I was using to get some metrics (although I had to remove the backslashes for testing the original code since that makes it thrown an exception):

    void "Performance"() {
        expect:
        for (int i = 0; i < 1000000; i++) {
            HstoreParser parser = new HstoreParser('')
            parser.setValue(/"key"=>"\"value\""/)
            parser.asMap()
        }
    }
butters16 commented 5 years ago

@jglapa You reported this bug a long time ago. How have you been working around it since then, or are you doing something to avoid the backslashes in the data on the write side?

jglapa commented 5 years ago

@jglapa You reported this bug a long time ago. How have you been working around it since then, or are you doing something to avoid the backslashes in the data on the write side?

Yeah, I wanted to fix this in the HStoreIterator but got ... scared;) We decided to sanitize the input to avoid the issue but would be really great to have that working finally.

butters16 commented 5 years ago

@ilopmar Is there anything else I can do to help with merging this PR with official release(s) for 6.1.0 (and possibly 4.8.0)? I updated the PR with one of the suggestions from @jglapa about removing the RegEx.

ilopmar commented 5 years ago

@butters16 Nothing more on your side. I'll try to merge it and release the new versions tonight.

ilopmar commented 5 years ago

I've released the fix in 4.8.0, 5.3.0 and 6.1.0. Thank you very much for your help!

butters16 commented 5 years ago

I've released the fix in 4.8.0, 5.3.0 and 6.1.0. Thank you very much for your help!

4.8.0 is working for me. Thank you! I'm glad I could help out!