-
Notifications
You must be signed in to change notification settings - Fork 82
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.
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
Support the PostgreSQL domain #532
Conversation
I have simply merged and updated the project code. Could you please review it for me again? I have thought about some parts and made some adjustments. Although there might be some places where I didn't follow your way of adjustment completely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for continuing to update this. Left some final comments. After that this can be merged.
src/pg/types.cpp
Outdated
Oid | ||
GetBaseDuckColumnType(Oid attribute_typoid) { | ||
std::lock_guard<std::recursive_mutex> lock(pgduckdb::GlobalProcessLock::GetLock()); | ||
Oid typoid = attribute_typoid; | ||
if (get_typtype(attribute_typoid) == TYPTYPE_DOMAIN) { | ||
/* It is a domain type that needs to be reduced to its base type */ | ||
typoid = getBaseType(attribute_typoid); | ||
} else if (type_is_array(attribute_typoid)) { | ||
Oid eltoid = get_base_element_type(attribute_typoid); | ||
if (OidIsValid(eltoid) && get_typtype(eltoid) == TYPTYPE_DOMAIN) { | ||
/* When the member type of an array is domain, you need to build a base array type */ | ||
typoid = get_array_type(getBaseType(eltoid)); | ||
} | ||
} | ||
return typoid; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of these functions can throw Postgres errors (like get_typtype
and getBaseType
). They all need to be wrapped in PostgresFunctionGuard. It's probably easiest to create a small wrapper function like this:
Oid | |
GetBaseDuckColumnType(Oid attribute_typoid) { | |
std::lock_guard<std::recursive_mutex> lock(pgduckdb::GlobalProcessLock::GetLock()); | |
Oid typoid = attribute_typoid; | |
if (get_typtype(attribute_typoid) == TYPTYPE_DOMAIN) { | |
/* It is a domain type that needs to be reduced to its base type */ | |
typoid = getBaseType(attribute_typoid); | |
} else if (type_is_array(attribute_typoid)) { | |
Oid eltoid = get_base_element_type(attribute_typoid); | |
if (OidIsValid(eltoid) && get_typtype(eltoid) == TYPTYPE_DOMAIN) { | |
/* When the member type of an array is domain, you need to build a base array type */ | |
typoid = get_array_type(getBaseType(eltoid)); | |
} | |
} | |
return typoid; | |
} | |
static Oid | |
GetBaseDuckColumnType_C(Oid attribute_typoid) { | |
std::lock_guard<std::recursive_mutex> lock(pgduckdb::GlobalProcessLock::GetLock()); | |
Oid typoid = attribute_typoid; | |
if (get_typtype(attribute_typoid) == TYPTYPE_DOMAIN) { | |
/* It is a domain type that needs to be reduced to its base type */ | |
typoid = getBaseType(attribute_typoid); | |
} else if (type_is_array(attribute_typoid)) { | |
Oid eltoid = get_base_element_type(attribute_typoid); | |
if (OidIsValid(eltoid) && get_typtype(eltoid) == TYPTYPE_DOMAIN) { | |
/* When the member type of an array is domain, you need to build a base array type */ | |
typoid = get_array_type(getBaseType(eltoid)); | |
} | |
} | |
return typoid; | |
} | |
Oid | |
GetBaseDuckColumnType(Oid attribute_type_oid) { | |
return PostgresFunctionGuard(GetBaseDuckColumnType_C, attribute_type_oid); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you very much for your guidance. I feel that the lock in the c function seems to have no effect at this time, so I try to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agreed.
Domain types are basically user defined aliases of existing types. This starts supporting columns of such domain types pg_duckdb. One interesting thing is that domain types can also contain constraints, e.g. "an int that is less than 10". This doesn't really matter for reading them though, their actual representation is still always the same as the underlying type. It also doesn't matter when doing operations on them, because Postgres already changes a domain type to its underlying representation automatically. So any constraints on the domain don't apply to a result of an operation (e.g. addition).
Fixes #514