Change json datatype to jsonb

[clemensg]

Hi,

as the json datatype needs to be parsed over and over again and queue_classic does not make assumptions about the ordering of args' keys, a switch to jsonb seems reasonable to me.

Quote from the PostgreSQL docs: In general, most applications should prefer to store JSON data as jsonb, unless there are quite specialized needs, such as legacy assumptions about ordering of object keys.
http://www.postgresql.org/docs/current/static/datatype-json.html

In case somebody uses 9.3 or earlier, I think it is OK to fall back to text and not check for both jsonb and json. With text we have at least no DB performance penalty, but json is reparsed with every insert. jsonb is parsed only once.

[senny]

Transitioning to jsonb makes sense. However we need to take existing users into account. There should be a migration from json to jsonb.

Not sure wether we should keep falling back to json or go straight to text.

/cc @jipiboily

[clemensg]

Wouldn't it be less intrusive to only change the datatype of new migrations instead of migrating every existing args column to jsonb?
There is also less benefit for using json alone, so a fallback to text seems reasonable to me. However, I can add a fallback from jsonb to json to text if you want.

[senny]

@clemensg this change only makes sense if we think jsonb is the thing people should be using (if available). Given that is the case, why should existing applications keep using something we are moving away from?

One should still be able to skip the migration if one pleases but we should make it easy to follow along.

[clemensg]

OK, good point. But then we should also migrate old text columns, right?

I mean:

• text on < 9.2 stays text
• text on 9.2 and 9.3 becomes json
• text and json on 9.4 and newer become jsonb

Or:

• text on < 9.4 stays text
• text and json on 9.4 and newer become jsonb

[senny]

I don't know the details but it depends how easily we can migrate a column. I'm expecting that PostgreSQL allows us to ALTER TABLE from json to jsonb. With text, this could be more demanding.

Currently I'm thinking about a simple migration which checks for jsonb availability and migrates json to jsonb.

@clemensg can you investigate?

[clemensg]

@senny Yes.

[clemensg]

@senny I updated the PR. Btw.: In the update_to_3_2_0.sql file, I also set the args column to text, in case json and jsonb are both not available. It's just a safety measure to fix the schema in case someone reverts to an older PostgreSQL version and still used jsonb. Then we would automatically migrate back to text.

Regarding your concerns about the text data type: PostgreSQL does allow us to change the column data type from text to json / jsonb and vice versa with a simple alter table t alter column c type x using c::x;.

[senny]

Could you add a corresponding downgrade script? Also, the Rails generator needs to be updated to generate an upgrade migration. See this PR for details https://github.com/QueueClassic/queue_classic/pull/217/files#diff-e455d018c3ab4c611d786db00a9c1458R1

[clemensg]

Yes, I updated the PR.

[senny]

@clemensg awesome 💛

@jipiboily thoughts?

[clemensg]

🐌

[senny]

@clemensg please be respectful with your comments. We are investing our own personal time for Open Source projects. I understand that delays can be frustrating at times but please remember that this is on our list and sometimes we can't drop everything to get it finished. Thank you for your understanding.

[clemensg]

Hey, I don't think a (cute) snail emoji is disrespectful.. do you really think so? Just to clarify: I meant "Any blockers? Anything left to do before this can get merged?"

Of course you are investing your own personal time, so am I.

[senny]

@clemensg I'm saying a snail emoji could easily be interpreted as "why is this moving so slow?". Also bumping just leads to a bigger email flood. If you have big repositories like rails/rails on watching, you get more sensible to these kinds of comments as they flood your inbox every day.

[jipiboily]

This sounds good generally speaking. My only worry is that so far we didn't change the column type at all. We're still using text. It might be perfectly fine but it could also break things for some people, like for queue_classic_admin or other things built around it. This is quite unlikely and should be easy to fix though so I'm 👍.

[senny]

I was thinking about this some more. Maybe it's better to be conservative and don't upgrade existing databases at all. We can add a section to the README explaining how a user could perform the upgrade. This way new users get jsonb out of the box and existing ones get to choose what way they want to go.

@clemensg what do you think about that approach?

[clemensg]

@senny This was my initial approach in this PR. That's why I asked Wouldn't it be less intrusive to only change the datatype of new migrations instead of migrating every existing args column to jsonb? So yes, I think so too, we should go for the more conservative approach.

I'll change / revert the PR 😉

[senny]

@clemensg sorry for the roundtrip 😓. Thanks for helping out and adjusting the PR.

[clemensg]

No problem. Sometimes it's worth thinking about a problem for a longer time.
By the way, one benchmark test failed, is this a Travis issue?

[senny]

mmm, I remember a test that failed for a longer period of time but If I remember correctly I added a skip to that one. So, if I'm not mistaken the build was green. Benchmark could fail for a number of reasons related to the Travis setup though. Let's wait for the run of the update PR and see how that goes.

[clemensg]

Looks like another test is asking for skip.

[clemensg]

@senny Interesting, now it's green after I re-triggered the Travis build.

[senny]

yea, seems to be flapping. Maybe we get various speeds from Travis when running the build. Due to server load or something like that. I'll try to look at it in the near future.

[senny]

Worked fine when testing on my machine. @clemensg thanks for your patience and the roundtrip 💛!