Fails value insertion if type unsupported by duckdb

[dentiny]

I would like to followup on issue: #467

I think current implementation is not ideal:

• If user creates a table with duckdb unsupported numeric type, we silently fallback to double type
• Later write operation doesn't always fail, based on the input numeric value, which adds more confusion to the users

Here I propose two improvements:

• method-1: No silent fallback on type conversion "failure"; so we print a warning message with elog(WARNING, ...), and get users aware of the fallback behavior;
• method-2: Instead of falling back to double type, we fallback to user-defined type, since it's not supported by duckdb (system) anyway.

In this PR, I selected method-2, but I'm open to other feedbacks and suggestions.
The goal here is to disallow silent fallback and surprising write behavior.

Edit:
Followup on feedback #471 (comment), I fail the table creation if column type is not supported by duckdb.

[JelteF]

Thank you for the contribution.

The fact that we silently fall back to double has been bothering me too (see #309). I think it would be nice to make this a configurable setting that should probably be off by default, called something like duckdb.use_double_for_large_numerics.

However, afaict in the case you're describing this is actually not what is happening. In this case NUMERIC is converted to DECIMAL(18, 3), not double.

> CREATE TEMP TABLE t (large_number NUMERIC) USING duckdb;
CREATE TABLE
> select * from duckdb.raw_query($$ describe pg_temp.main.t $$);
NOTICE:  00000: result: column_name column_type null    key default extra
VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR VARCHAR
[ Rows: 1]
large_number    DECIMAL(18,3)   YES NULL    NULL    NULL

[JelteF]

To be clear, I think it's probably reasonable to disallow specifying plain NUMERIC as the type for duckdb tables and require specifying a width and scale. But I don't think the current code actually has that effect.

[dentiny]

Both suggestion makes sense to me, thank you! I will make changes these days.

[dentiny]

Hi @JelteF , I tried to implemented your suggestion, which fails to create table if column type is not supported by duckdb.
I verified by existing SQL test statements with make check.

[dpxcc]

nit: prob move this check to pgduckdb_get_tabledef(), where other lockdowns (e.g., generated columns, identity columns) are handled

[dentiny]

I think it makes sense, my concern is, other DDL statements (i.e. ALTER TABLE) require the same validation logic.
I would prefer to put the validation logic in the same file.

[dentiny]

where other lockdowns (e.g., generated columns, identity columns) are handled

I fully agree to place all validation logic into one function / one place.
My personal preference is to move all logic into DDL file, since other validations (i.e. generated columns, as you mentioned) should also apply to other DDL statements.

Would like to know how pg_duckdb's owner think about it. :)

[dentiny]

Hi @JelteF , I'm wondering if you could take another look when you have some time? Thank you!
Let me know if there's anything else I should change.

The fact that we silently fall back to double has been bothering me too (see #309). I think it would be nice to make this a configurable setting that should probably be off by default, called something like duckdb.use_double_for_large_numerics.

On this proposal, I will address it in another followup PR.

[dentiny]

There's branch conflict and unit test failure, let me resolve them before re-requesting reviews.