feat(c/driver/postgresql): TimestampTz write #868
Conversation
c/driver/postgresql/connection.cc
Outdated
| @@ -1025,6 +1025,18 @@ AdbcStatusCode PostgresConnection::SetOption(const char* key, const char* value, | |||
| } | |||
| return ADBC_STATUS_OK; | |||
| } | |||
|
|
|||
| if (std::strcmp(key, "TIME ZONE") == 0) { | |||
| std::string query = std::string("SET TIME ZONE '") + std::string(value) + "'"; | |||
There was a problem hiding this comment.
Could also set / unset this in the lifecycle of the statement
There was a problem hiding this comment.
If we do want an option for this, it should be something like adbc.postgresql.time_zone based on other drivers
But if we just need it for ingestion, maybe we could do everything inside a transaction, and set/restore the timezone within the transaction?
There was a problem hiding this comment.
Yea either of those are viable; I think better to start with the latter, though I'm not sure where to manage that lifecycle. Is the BindStream struct in statement.cc supposed to be a suitable home for managing a transaction? The type introspection happens there, but I don't know that is the best place to put BEGIN/COMMIT statements.
There was a problem hiding this comment.
Hmm, I think for this case it might be, but we probably are going to need some more intensive refactoring at some point
|
Think the polars issue is unrelated |
| bool has_tz_field = false; | ||
| std::string tz_setting; |
There was a problem hiding this comment.
agh, I was going to ask for this to be std::optional...but we can't do that
c/driver/postgresql/statement.cc
Outdated
| (strcmp("", bind_schema_fields[col].timezone))) { | ||
| has_tz_field = true; | ||
|
|
||
| PGresult* begin_result = PQexec(conn, "BEGIN"); |
There was a problem hiding this comment.
Don't we need to hoist this out of the loop? Or else, only do this if has_tz_field was previously false. Otherwise, it looks like we'll try to begin the transaction twice.
(Also: I wonder if we shouldn't always begin/end a transaction when dealing with multiple bind parameters...)
There was a problem hiding this comment.
+how does this interact with auto-commit off?
There was a problem hiding this comment.
I have the break at the end of the loop to make sure this only happens once, but that could be more clearly signaled. Could even just set has_tz_field within the loop then check if (has_tz_field) right after before potentially starting a transaction
There was a problem hiding this comment.
As far as autocommit goes I do think you are right about that being murky. I'm not sure how to best resolve that yet either - beginning / ending the transaction here within the BindStream class is probably at odds with most of the autocommit stuff being managed via the connection.
Are there tests already for autocommit in the ADBC suite? I was looking for a reference on how those are expected to behave. Wasn't sure if I was overlooking those of if it is something that hasn't been heavily implemented yet
There was a problem hiding this comment.
ah, I missed that little break at the end.
There are some simple tests that test the options, but not really the semantics.
At least here, I think you 'just' need to check if autocommit is off, if so, there's no need to BEGIN (you're already in a transaction), and you can still commit at the end (~though maybe semantically we shouldn't? This isn't a feature that's normally available in other APIs so I'm not sure what people expect)
There was a problem hiding this comment.
Any ideas on how to best forward the autocommit setting to the BindStream class? Right now I only see that as a private member of the connection, so it is pretty far removed from this
There was a problem hiding this comment.
A statement already holds a connection, so we can just add a getter to the PostgresConnection class
closes #867
The lifecycle of setting the session time zone in this isn't great. I think should be handled during connect somehow? But not sure if postgres even supports that reading things like:
https://stackoverflow.com/a/11779621/621736
I think there is also something awry with the release callback for timezone schemas; needs further investigation