Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow out of range numerics type on table creation #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Disallow out of range numerics type on table creation #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4537341
disallow out of range numerics type
dentiny Dec 4, 2024
3dc77c4
fix missing return and revert change on oid
dentiny Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 52 additions & 15 deletions src/columnstore_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,38 @@ extern "C" {
#include "utils/syscache.h"
}

namespace {

// Get precision for postgres "numerics" type, copied from pg_duckdb.
int numeric_typmod_precision(int32 typmod) {
return ((typmod - VARHDRSZ) >> 16) & 0xffff;
}
// Get scale for postgres "numerics" type, copied from pg_duckdb.
int numeric_typmod_scale(int32 typmod) {
return (((typmod - VARHDRSZ) & 0x7ff) ^ 1024) - 1024;
}

// If column is of type "numeric", check whether it's acceptable for mooncake;
// If not, exception is thrown via `elog`.
void ValidateColumnNumericType(Form_pg_attribute &attribute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems good to also have the same lockdown for pg_duckdb's temp DuckDB table?
So it might be better to make this change to pg_duckdb instead?

Copy link
Contributor Author

@dentiny dentiny Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I don't read it wrong, this is how pg_duckdb handles temp table creation:
https://github.com/duckdb/pg_duckdb/blob/bb82c937936b97824bb766893745e1456eb1aa08/src/pgduckdb_ddl.cpp#L199-L364

Considering the fact that pg_duckdb doesn't support complete table creation functionality, I think it would be nice if:

  • pg_duckdb locks down temp table creation on duckdb-incompatible types
  • pg_mooncake locks down table creation on duckdb-incompatible types

It seems good to also have the same lockdown for pg_duckdb's temp DuckDB table?

To your question, yes.

So it might be better to make this change to pg_duckdb instead?

But I'm not sure how we could place the whole lock down logic into pg_duckdb?
Since it only handles part of the table creation logic as of now.

I mentioned it briefly in the PR description:

Method-2: propagate the logic to table creation logic into pg_duckdb
It doesn't work because pg_duckdb doesn't support general-purpose table creation as of now;
Moving the logic into pg type -> duck type conversion is also not proper.

Let me know if I mis-understand pg_duckdb's capability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the creation of columnstore tables and DuckDB temporary tables follow different code paths
My suggestion was only to move the helper functions numeric_typmod_precision, numeric_typmod_scale, and ValidateColumnNumericType in this file to pg_duckdb
I noticed you just opened duckdb/pg_duckdb#471, which already addresses this
We just need to wait for that diff to land and re-pull from pg_duckdb

auto &type = attribute->atttypid;
if (type != NUMERICOID) {
return;
}
auto &typmod = attribute->atttypmod;
auto precision = numeric_typmod_precision(typmod);
auto scale = numeric_typmod_scale(typmod);
// duckdb's "numeric" type's max supported precision is 38.
if (typmod == -1 || precision < 0 || scale < 0 || precision > 38) {
elog(ERROR,
"Unsupported type when creating column: type precision %d with scale %d is not supported by duckdb, which "
"only allows maximum precision 38",
precision, scale);
}
}

} // namespace

const TupleTableSlotOps *columnstore_slot_callbacks(Relation rel) {
elog(ERROR, "columnstore_slot_callbacks not implemented");
}
Expand Down Expand Up @@ -129,24 +161,29 @@ void columnstore_relation_set_new_filenode(Relation rel, const RelFileNode *newr
TransactionId *freezeXid, MultiXactId *minmulti) {
#endif
HeapTuple tp = SearchSysCache1(RELOID, ObjectIdGetDatum(rel->rd_id));
if (!HeapTupleIsValid(tp)) {
TupleDesc desc = RelationGetDescr(rel);
for (int i = 0; i < desc->natts; i++) {
Form_pg_attribute attr = &desc->attrs[i];
auto duck_type = pgduckdb::ConvertPostgresToDuckColumnType(attr);
if (duck_type.id() == duckdb::LogicalTypeId::USER) {
elog(ERROR, "column \"%s\" has unsupported type", NameStr(attr->attname));
}
if (attr->attgenerated) {
elog(ERROR, "unsupported generated column \"%s\"", NameStr(attr->attname));
}
}

duckdb::Columnstore::CreateTable(rel->rd_id);
} else {
if (HeapTupleIsValid(tp)) {
ReleaseSysCache(tp);
duckdb::Columnstore::TruncateTable(rel->rd_id);
return;
}

TupleDesc desc = RelationGetDescr(rel);
for (int i = 0; i < desc->natts; i++) {
// Check whether column type is acceptable.
Form_pg_attribute attr = &desc->attrs[i];
auto duck_type = pgduckdb::ConvertPostgresToDuckColumnType(attr);
if (duck_type.id() == duckdb::LogicalTypeId::USER) {
elog(ERROR, "column \"%s\" has unsupported user type", NameStr(attr->attname));
}
if (attr->attgenerated) {
elog(ERROR, "unsupported generated column \"%s\"", NameStr(attr->attname));
}

// Check numeric types, which have different support for duckdb and postgres.
ValidateColumnNumericType(attr);
}

duckdb::Columnstore::CreateTable(rel->rd_id);
}

void columnstore_relation_nontransactional_truncate(Relation rel) {
Expand Down
4 changes: 4 additions & 0 deletions test/expected/unsupported/types.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@ ERROR: column "a" has unsupported type
CREATE TYPE point AS (x int, y int);
CREATE TABLE t (a point) USING columnstore;
ERROR: column "a" has unsupported type
CREATE TABLE mooncake_numeric_tbl (val numeric(40, 5)) USING columnstore;
dentiny marked this conversation as resolved.
Show resolved Hide resolved
ERROR: Unsupported type when creating column: type precision 40 with scale 5 is not supported by duckdb, which only allows maximum precision 38
CREATE TABLE mooncake_numeric_tbl (val numeric) USING columnstore;
ERROR: Unsupported type when creating column: type precision 65535 with scale -5 is not supported by duckdb, which only allows maximum precision 38
3 changes: 3 additions & 0 deletions test/sql/unsupported/types.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,6 @@ CREATE TABLE t (a jsonb) USING columnstore;

CREATE TYPE point AS (x int, y int);
CREATE TABLE t (a point) USING columnstore;

CREATE TABLE mooncake_numeric_tbl (val numeric(40, 5)) USING columnstore;
CREATE TABLE mooncake_numeric_tbl (val numeric) USING columnstore;