Skip to content

Conversation

Maxxen
Copy link
Member

@Maxxen Maxxen commented Sep 17, 2024

When using the (undocumented) .dump command in the shell, while opening a database that contains blob-aliased types provided by an extension that is unloaded, the CLI would crash as the shell would detect a value to be present, but the the unknown type would fail to cast to BLOB (as the extension, and thus any cast functions are not available) and return a nullptr.

I've modified the shell and the sqlite3 api wrapper to handle this case, but I'm not sure if this is the best fix or what the intended behavior should be when interacting with unknown types from the sqlite api. If we were to fall-back into a reinterpret cast, im not sure that's desirable when casting within the sqlite3_column_has_value as the type check then become essentially meaningless. Also there is no guarantee that the extension in question actually provides a valid reverse cast from the underlying type (e.g. BLOB) to the extension type even when loaded. It feels like the only sane thing is to either error out or turn all unknown types to NULL.

Closes duckdb/duckdb-spatial#391

@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 17, 2024 10:39
@Maxxen Maxxen marked this pull request as ready for review September 17, 2024 11:17
@Mytherin
Copy link
Collaborator

Mytherin commented Sep 17, 2024

Thanks for the PR!

I think the problem here is mostly that SQLITE_BLOB is returned at all for extension types. The crash is a side-effect of this, but when loading the extension, .dump still does not function as intended and is instead emitting the underlying blob data which is not usable:

CREATE TABLE t1(geom GEOMETRY);;
INSERT INTO t1 VALUES(X'0000000000000000000000000100000000400a923db6b64000a039b27e6bab40');
...
COMMIT;

Perhaps sqlite3_column_type can be modified to return SQLITE_TEXT for all extension types instead (i.e. when column_type.HasAlias())?

@Maxxen
Copy link
Member Author

Maxxen commented Sep 17, 2024

That sounds good! I wasn't really aware that SQLITE_TEXT was different from SQLITE_BLOB, but it makes a lot of sense that we would convert any aliased type to text - I guess its mostly safe to assume that any custom type will at the very least provide a reversible VARCHAR cast.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft September 18, 2024 08:41
@Maxxen Maxxen marked this pull request as ready for review September 18, 2024 09:18
@Mytherin Mytherin merged commit 9daac6a into duckdb:main Sep 18, 2024
25 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Sep 27, 2024
[Binding] Always try binding with the schema of the `UserType` first if it's set (duckdb/duckdb#13995)
Fix crash in the shell caused by printing blobs that failed to cast (duckdb/duckdb#13983)
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Sep 27, 2024
[Binding] Always try binding with the schema of the `UserType` first if it's set (duckdb/duckdb#13995)
Fix crash in the shell caused by printing blobs that failed to cast (duckdb/duckdb#13983)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.dump crashes with duckdb 1.1 on a database with a RTree if not loading the spatial extension
2 participants