Per-connection decoders with PoC citext and hstore support
#89
Conversation
No-op for now but it introduces some indirection to experiment with decoders for known types whose OIDs differ from database-to-database, such as `hstore` or `citext`.
Probably this could do with its own error type or even fail gracefully to the built-in static list.
Duh... `oid` is a hidden column that I can just query. No need to monkey around to figure it out.
This refactors the existing behaviour to have a shared interface (which could
be an abstract module) for registering decoders:
Decoders.register_decoder(decoder : Decoders::Decoder, oid)
PG::Connection#register_decoder(decoder : Decoders::Decoder, oid)
While the `Decoders` module maintains responsibility for discovering types, it
adds them to the connection's map via this interface, instead of returning a
new `DecoderMap` for the `PG::Connection` to assign.
This also means the connection's `DecoderMap` is never nil and isn't swapped
out. However, it is mutated. This only happens automatically before the
connection is available for use by the caller but the `#register_decoder`
method could be called later.
I just reverse engineered this by looking at the output of the `ByteaDecoder` on these test values so this may not handle some other scenarios I haven't accounted for.
I think this could probably take most maps (e.g. Map(Symbol, T)) by always `#to_s`ing everything except `nil`. This encoder will need some stronger test cases to ensure values are escaped properly (I don't think `#inspect` will necessarily cut it).
|
Added an |
bjeanes
force-pushed
the
per-connection-decoders
branch
from
aa24fb6
to
2dc4bda
Compare
bjeanes
changed the title
citext supportcitext and hstore support
|
Further thoughts:
|
spec/pg/encoder_spec.cr
Outdated
| PG_DB.exec "drop table if exists test_table" | ||
| PG_DB.exec "create table test_table (v #{datatype})" | ||
| begin | ||
| PG_DB.exec "create extension \"#{extension}\"" if extension |
There was a problem hiding this comment.
Maybe add if exists here and in the drop extension later on, to avoid errors if the test database isn't 100% clean
|
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! |
|
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, ***@***.***> 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
<#89 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAKAAtCKVYzpPzSWA5HNqQ0Bhy0z90rks5rqs6ngaJpZM4MpF_T>
.
|
|
Hey @bjeanes did you have any progress on the other approach? |
|
@will ah no I got sidetracked with other things. I was basically wanting to turn the 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? |
|
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
force-pushed
the
per-connection-decoders
branch
from
d5ffde1
to
58e1211
Compare
| def self.register_connection_decoders(connection : PG::Connection) : Void | ||
| types = connection.query_all(TYPE_SQL, as: {UInt32, String, Char}) | ||
| types.each do |oid, name, category| | ||
| oid = oid.to_i32 # Query execution if I read straight to Int32 :\ |
There was a problem hiding this comment.
Oops this comment should read "query execution hangs if...".
That was a reminder to me to ask @will why that is? I was fighting a lot of mysterious hangs if I didn't get things right. It can be hard to debug sometimes. Any advice?
|
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? |
|
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. |
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_typeand expand on the statically-defined global decoder map.This mechanism is used to implement
citextsupport.Notes
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.CREATE EXTENSION). There's a few options here, but they don't seem deeply important to address now. It does mean the test forcitexthas to jump through some hoops, though.pg_type.HstoreDecodercould be wired up, too.