Fix column type OIDs in pgduck_server row description metadata#327
Fix column type OIDs in pgduck_server row description metadata#327Khoulii wants to merge 5 commits into
Conversation
…validOid Signed-off-by: Khoulii <k.khouli@yahoo.com>
…Snowflake-Labs#326) When PQconsumeInput() returned false (broken connection), WaitForResult called ReleasePGDuckConnection() before re-throwing the error. The PG_FINALLY block in ExecuteCopyToCommandOnPGDuckConnection (and every other caller that wraps GetPGDuckConnection/ExecuteQueryOnPGDuckConnection in a PG_TRY) then called ReleasePGDuckConnection() a second time on the same hash entry. After HASH_REMOVE the entry's memory is returned to dynahash's freelist. If the slot was reused for the retry connection created inside ExecuteQueryOnPGDuckConnection, the second call freed the *new* connection. If the slot was not reused the second PQfinish() was called on an already- freed PGconn, producing the bogus address seen in the crash: #0 __GI___libc_free (mem=0xf5c9c67aca47e118) at malloc.c:3375 Snowflake-Labs#1 pqReleaseConnHosts (conn=0x30e138c0) at fe-connect.c:4723 Snowflake-Labs#4 ReleasePGDuckConnection at src/pgduck/client.c:186 Fix: remove the ReleasePGDuckConnection() call from WaitForResult. Connection lifetime is exclusively the caller's responsibility, managed via the PG_FINALLY block. Signed-off-by: Khoulii <k.khouli@yahoo.com>
sfc-gh-mslot
left a comment
There was a problem hiding this comment.
Thanks for looking into this! Did you run into a specific issue with the invalid OIDs?
@sfc-gh-mslot this issue reports that Grafana was unable to draw graphs ..etc. because of the bad OIDs. again, we did not observe this on production but it's very convenient for local/dev use cases. |
…id instead of TEXTOID. Signed-off-by: Khoulii <k.khouli@yahoo.com>
Signed-off-by: Khoulii <k.khouli@yahoo.com>
|
Triggering PR vaildation now. Thanks for the contribution! |
|
@sfc-gh-dachristensen this requires extensive test changes I think I got most of them at |
|
@sfc-gh-mslot branch looks good; we want to force push here? |
Map DuckDB types to their proper PostgreSQL type OIDs in the row description metadata, instead of returning InvalidOid (which downstream clients silently coerce to TEXTOID and lose type information). Originally submitted as #327 by @Khoulii. Transplanted onto a maintainer branch to run the full CI suite (which only triggers on push, not on pull_request from forks). Co-authored-by: Khoulii <k.khouli@yahoo.com> Signed-off-by: Khaled Alkhouli <khaled.alkhouli@opensooq.com> Signed-off-by: Khoulii <k.khouli@yahoo.com> Signed-off-by: Marco Slot <marco.slot@snowflake.com>
Description
Fixes #67
Currently,
duckdb_query_result_send_column_metadataalways writesInvalidOid(0) as the column type OID in the row description message, regardless of the actual column type. This breaks clients that rely onPQftype()for type introspection (e.g. ORMs, psycopg2, JDBC drivers).This PR adds
duckdb_type_to_pg_oid()intype_conversion.cwhich maps duckdb_type values to their closest PostgreSQL OID equivalents, and wires it into the column metadata sender in place of the hardcodedInvalidOid.Mapping strategy:
BOOLEAN→BOOLOID,SMALLINT→INT2OID,INTEGER→INT4OID,BIGINT→INT8OID,FLOAT→FLOAT4OID,DOUBLE→FLOAT8OID,DATE→DATEOID,TIME→TIMEOID,TIMESTAMP→TIMESTAMPOID,TIMESTAMPTZ→TIMESTAMPTZOID,INTERVAL→INTERVALOID,UUID→UUIDOID,BLOB→BYTEAOIDUTINYINT→INT2OID,USMALLINT→INT4OID,UINTEGER→INT8OID)UBIGINT,HUGEINT,UHUGEINT,DECIMAL) →NUMERICOIDLIST,ARRAY,STRUCT,MAP,UNION,ENUM) →TEXTOIDsince they are serialised as text. These types should be considered to be handled more properly in future PRs.Checklist