Per-connection decoders with PoC `citext` and `hstore` support

[bjeanes]

This is a follow up to my thoughts in #88.

Summary

Reading from a result set picks a decoder via a connection-local decoder map instead of the global list. Misses on the local map fallback to the existing global map of handlers.

When a connection is first initialised, it is immediately used to interrogate pg_type and expand on the statically-defined global decoder map.

This mechanism is used to implement citext support.

Notes

• This will cause types to be determined multiple types in a connection pooling scenario. It may instead be preferable to do this once per DB. However, given that connections may or may not be pooled, the current approach seemed the most prudent, at least from a proof-of-concept standpoint.
• Changes to the types during a connection won't be visible (e.g. CREATE EXTENSION). There's a few options here, but they don't seem deeply important to address now. It does mean the test for citext has to jump through some hoops, though.
• The statically-defined global map of decoder handlers is depended upon to interpret the result set from querying pg_type.
• I've left more comments and commented-out code than I usually would as the point of this PR is foremost to communicate the idea. E.g. A small snippet of code shows how a specialised HstoreDecoder could be wired up, too.

[bjeanes]

Added an HstoreDecoder and param handling but it's probably pretty naïve.

[bjeanes]

Further thoughts:

• A lot of types will use the same decoder and as more types are supported, the number of these decoder instances will grow with the connections. They aren't stateful so perhaps stand to be made singletons.
• This first pass just demonstrates loading a fallback decoder based on the type category and loading one based on the type name. However, for other DOMAINs actually looking up the decoder based on the base type's OID would be good.
• Array decoding doesn't (yet) benefit from this approach. pg_type does contain enough information that we could add extra array decoders. Though I don't yet know enough about Crystal to see how this dynamic approach could support the recursive type definition employed now.
• Is there any reason read_array and read approaches can't be unified? Array entries in pg_type include OID for the elements' type, which could be used to pass in the element decoder.
• This code still needs a bit of factoring (I don't think this new code all belongs in Decoders directly—probably in a child namespace).

[will]

I need to think more on your questions. I'm at a postgres conf this week (and giving a talk including the protocol parts of this project 😀), so I'll dive in next week. Thanks for putting this together!

[bjeanes]

No worries! I actually have another approach that may generalise better and handle more cases so may have another similar PR next week anyway (if i can spare the time–otherwise I'll leave a describing comment instead)

On Thu., 30 Mar. 2017, 8:35 am Will Leinweber, [email protected] wrote:

I need to think more on your questions. I'm at a postgres conf this week (and giving a talk including the protocol parts of this project 😀), so I'll dive in next week. Thanks for putting this together!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/will/crystal-pg/pull/89#issuecomment-290233332, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAKAAtCKVYzpPzSWA5HNqQ0Bhy0z90rks5rqs6ngaJpZM4MpF_T .

[will]

Hey @bjeanes did you have any progress on the other approach?

[bjeanes]

@will ah no I got sidetracked with other things.

I was basically wanting to turn the DecoderMap as Hash(OID, Decoder) to TypeMap as Hash(OID, PG::Type). PG::Type is a struct which includes a (singleton) reference to decoder but also also more information from the pg_type table. I don't know if this is actually valuable but I have a hunch that it could make array decoding less of a special-case and allow for some sane assumptions for newly encountered types based on the category (geometric, numeric, etc).

If not that approach, I think my actual PoC here could be factored to be less ad-hoc as I kinda just dumped some code in the existing decoder code.

I dunno... what do you think? Did you have any imagined direction when you've contemplated per-connection type support? Anything you'd want to see to have a this or a derivative of this be merged?

[bjeanes]

Oh I also think there's more opportunity for composite or higher-order decoders to leverage the primitive decoders and perhaps for some symmetry between encoding/decoding, which might make it a bit clearer when adding support for future types.

[bjeanes]

I've been thinking about picking back up my side project that needed this functionality and drove this PR. @will are you still working on this lib and if so what do you see as the path forward for hstore/citext support?

[will]

Yeah, this is still a project. I think the only way for any sort of extension data type where the oid changes from database to database will require a per-connection mapping. If you're interested in working on this more, I'm happy to help if you run into anything.