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

Jump to bottom

Harden against Postgres interactions with C++ destructors #254

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Harden against Postgres interactions with C++ destructors #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1a79fd
Simplify `DuckDBQueryOrThrow`
Y-- Oct 31, 2024
3c8a7ff
SPI_exec_or_throw
Y-- Oct 31, 2024
2daf4ee
Throw C++ exceptions
Y-- Nov 1, 2024
ad0a63c
C++ version
Y-- Nov 1, 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
2 changes: 2 additions & 0 deletions include/pgduckdb/pgduckdb_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ __CPPFunctionGuard__(const char *func_name, FuncArgs... args) {

#define InvokeCPPFunc(FUNC, ...) pgduckdb::__CPPFunctionGuard__<decltype(&FUNC), &FUNC>(__FUNCTION__, __VA_ARGS__)

int SPI_exec_or_throw(const char *query, int tcount, int expected_ret_code);

// Wrappers

#define DECLARE_PG_FUNCTION(func_name) \
Expand Down
4 changes: 3 additions & 1 deletion include/pgduckdb/types/decimal.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ extern "C" {
#include "utils/numeric.h"
}

#include "duckdb/common/exception/conversion_exception.hpp"

typedef int16_t NumericDigit;

struct NumericShort {
Expand Down Expand Up @@ -258,7 +260,7 @@ CreateNumeric(const NumericVar &var, bool *have_error) {
*have_error = true;
return NULL;
} else {
ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("value overflows numeric format")));
throw duckdb::ConversionException("value overflows numeric format");
}
}
return result;
Expand Down
2 changes: 1 addition & 1 deletion src/catalog/pgduckdb_transaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ SchemaItems::GetTable(const string &entry_name) {
// Check if the Relation is a VIEW
auto tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(rel_oid));
if (!HeapTupleIsValid(tuple)) {
elog(ERROR, "Cache lookup failed for relation %u", rel_oid);
throw std::runtime_error("Cache lookup failed for relation " + std::to_string(rel_oid));
}

auto relForm = (Form_pg_class)GETSTRUCT(tuple);
Expand Down
71 changes: 31 additions & 40 deletions src/pgduckdb_ddl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ DuckdbHandleDDL(Node *parsetree, const char *queryString) {
} else if (IsA(parsetree, CreateSchemaStmt) && !pgduckdb::doing_motherduck_sync) {
auto stmt = castNode(CreateSchemaStmt, parsetree);
if (strncmp("ddb$", stmt->schemaname, 4) == 0) {
elog(ERROR, "Creating ddb$ schemas is currently not supported");
throw duckdb::NotImplementedException("Creating ddb$ schemas is currently not supported");
}
return;
} else if (IsA(parsetree, RenameStmt)) {
Expand All @@ -85,10 +85,10 @@ DuckdbHandleDDL(Node *parsetree, const char *queryString) {
return;
}
if (strncmp("ddb$", stmt->subname, 4) == 0) {
elog(ERROR, "Changing the name of a ddb$ schema is currently not supported");
throw duckdb::NotImplementedException("Changing the name of a ddb$ schema is currently not supported");
}
if (strncmp("ddb$", stmt->newname, 4) == 0) {
elog(ERROR, "Changing a schema to a ddb$ schema is currently not supported");
throw duckdb::NotImplementedException("Changing a schema to a ddb$ schema is currently not supported");
}
return;
}
Expand All @@ -110,9 +110,9 @@ CheckOnCommitSupport(OnCommitAction on_commit) {
case ONCOMMIT_DELETE_ROWS:
break;
case ONCOMMIT_DROP:
elog(ERROR, "DuckDB does not support ON COMMIT DROP");
throw duckdb::NotImplementedException("DuckDB does not support ON COMMIT DROP");
default:
elog(ERROR, "Unsupported ON COMMIT clause: %d", on_commit);
throw duckdb::NotImplementedException("Unsupported ON COMMIT clause: ", on_commit);
}
}

Expand Down Expand Up @@ -162,18 +162,15 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) {
SetConfigOption("search_path", "pg_catalog, pg_temp", PGC_USERSET, PGC_S_SESSION);
SetConfigOption("duckdb.force_execution", "false", PGC_USERSET, PGC_S_SESSION);

int ret = SPI_exec(R"(
pgduckdb::SPI_exec_or_throw(R"(
SELECT DISTINCT objid AS relid, pg_class.relpersistence = 't' AS is_temporary
FROM pg_catalog.pg_event_trigger_ddl_commands() cmds
JOIN pg_catalog.pg_class
ON cmds.objid = pg_class.oid
WHERE cmds.object_type = 'table'
AND pg_class.relam = (SELECT oid FROM pg_am WHERE amname = 'duckdb')
)",
0);

if (ret != SPI_OK_SELECT)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));
0, SPI_OK_SELECT);

/* if we selected a row it was a duckdb table */
auto is_duckdb_table = SPI_processed > 0;
Expand All @@ -185,19 +182,19 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) {
}

if (SPI_processed != 1) {
elog(ERROR, "Expected single table to be created, but found %" PRIu64, static_cast<uint64_t>(SPI_processed));
throw std::runtime_error("Expected single table to be created, but found " + std::to_string(SPI_processed));
}

HeapTuple tuple = SPI_tuptable->vals[0];
bool isnull;
Datum relid_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 1, &isnull);
if (isnull) {
elog(ERROR, "Expected relid to be returned, but found NULL");
throw std::runtime_error("Expected relid to be returned, but found NULL");
}

Datum is_temporary_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 2, &isnull);
if (isnull) {
elog(ERROR, "Expected temporary boolean to be returned, but found NULL");
throw std::runtime_error("Expected temporary boolean to be returned, but found NULL");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we discussed this in person, but I'll write it down here as well:

I'm not entirely convinced yet that changing all the elogs to exceptions in this file and in pgduckdb_ruleutils.cpp is all that helpful. It doesn't hurt either functionally, because you've wrapped this fuction in an InvokeCPPFunc call, and there's no PG_TRY around them. So there is no problem with throwing exceptions now.

But I think these specific functions probably won't benefit from being rewritten to C++ completely, and would be easier to maintain in Postgres C style. Mainly because they call a lot of Postgres functions and very few DuckDB functions. Wrapping all of those Postgres calls in PostgresFunctionGuard calls doesn't necessarily sound like it makes the code easier to read and maintain. I'm happily proven wrong though.

If you're fairly sure this is the right direction for these functions I'm fine with merging this now. But I think it should be a separate commit from most of the other changes (specifically the new and improved macros). To make reverting it easier if it turns out we want to make these functions fully Postgres C instead of fully C++.

}

Oid relid = DatumGetObjectId(relid_datum);
Expand Down Expand Up @@ -227,17 +224,18 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) {
values[2] = CStringGetTextDatum(pgduckdb::current_motherduck_catalog_version);
nulls[2] = ' ';
}
ret = SPI_execute_with_args(R"(
int ret = SPI_execute_with_args(R"(
INSERT INTO duckdb.tables (relid, duckdb_db, motherduck_catalog_version)
VALUES ($1, $2, $3)
)",
lengthof(arg_types), arg_types, values, nulls, false, 0);
lengthof(arg_types), arg_types, values, nulls, false, 0);

/* Revert back to original privileges */
SetUserIdAndSecContext(saved_userid, sec_context);

if (ret != SPI_OK_INSERT)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));
if (ret != SPI_OK_INSERT) {
throw std::runtime_error("SPI_exec failed: error code " + std::string(SPI_result_code_string(ret)));
}
}

AtEOXact_GUC(false, save_nestlevel);
Expand Down Expand Up @@ -265,7 +263,7 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) {
auto stmt = castNode(CreateTableAsStmt, parsetree);
CheckOnCommitSupport(stmt->into->onCommit);
} else {
elog(ERROR, "Unexpected parsetree type: %d", nodeTag(parsetree));
throw std::runtime_error("Unexpected parsetree type: " + std::to_string(nodeTag(parsetree)));
}

/*
Expand Down Expand Up @@ -345,18 +343,16 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) {
SetConfigOption("duckdb.force_execution", "false", PGC_USERSET, PGC_S_SESSION);

if (!pgduckdb::doing_motherduck_sync) {
int ret = SPI_exec(R"(
pgduckdb::SPI_exec_or_throw(R"(
SELECT object_identity
FROM pg_catalog.pg_event_trigger_dropped_objects()
WHERE object_type = 'schema'
AND object_identity LIKE 'ddb$%'
)",
0);
if (ret != SPI_OK_SELECT)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));
0, SPI_OK_SELECT);

if (SPI_processed > 0) {
elog(ERROR, "Currently it's not possible to drop ddb$ schemas");
throw std::runtime_error("Currently it's not possible to drop ddb$ schemas");
}
}

Expand All @@ -375,7 +371,7 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) {
* Here we first handle the non-temporary tables.
*/

int ret = SPI_exec(R"(
pgduckdb::SPI_exec_or_throw(R"(
DELETE FROM duckdb.tables
USING (
SELECT objid, schema_name, object_name
Expand All @@ -385,14 +381,11 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) {
WHERE relid = objid
RETURNING objs.schema_name, objs.object_name
)",
0);
0, SPI_OK_DELETE_RETURNING);

/* Revert back to original privileges */
SetUserIdAndSecContext(saved_userid, sec_context);

if (ret != SPI_OK_DELETE_RETURNING)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));

/*
* We lazily create a connection to DuckDB when we need it. That's
* important because we don't want to impose our "prevent in transaction
Expand Down Expand Up @@ -442,24 +435,21 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) {
* And now we basically do the same thing as above, but for TEMP tables.
* For those we need to check our in-memory temporary_duckdb_tables set.
*/
ret = SPI_exec(R"(
pgduckdb::SPI_exec_or_throw(R"(
SELECT objid, object_name
FROM pg_catalog.pg_event_trigger_dropped_objects()
WHERE object_type = 'table'
AND schema_name = 'pg_temp'
)",
0);

if (ret != SPI_OK_SELECT)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));
0, SPI_OK_SELECT);

for (auto proc = 0; proc < SPI_processed; ++proc) {
HeapTuple tuple = SPI_tuptable->vals[proc];

bool isnull;
Datum relid_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 1, &isnull);
if (isnull) {
elog(ERROR, "Expected relid to be returned, but found NULL");
std::runtime_error("Expected relid to be returned, but found NULL");
}
Oid relid = DatumGetObjectId(relid_datum);
if (temporary_duckdb_tables.count(relid) == 0) {
Expand Down Expand Up @@ -556,8 +546,9 @@ DECLARE_PG_FUNCTION(duckdb_alter_table_trigger) {
SetUserIdAndSecContext(saved_userid, sec_context);
AtEOXact_GUC(false, save_nestlevel);

if (ret != SPI_OK_SELECT)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));
if (ret != SPI_OK_SELECT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be converted to SPI_exec_or_throw afaict.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm don't we want to reset the user id etc (L558-559) before throwing? I thought about adding a version with a callback to execute before throwing but that felt overkill.

throw std::runtime_error("SPI_exec failed: error code '" + std::string(SPI_result_code_string(ret)) + "'");
}

/* if we inserted a row it was a duckdb table */
auto is_possibly_duckdb_table = SPI_processed > 0;
Expand All @@ -572,11 +563,11 @@ DECLARE_PG_FUNCTION(duckdb_alter_table_trigger) {
bool isnull;
Datum relid_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 1, &isnull);
if (isnull) {
elog(ERROR, "Expected relid to be returned, but found NULL");
throw std::runtime_error("Expected relid to be returned, but found NULL");
}
Datum needs_to_check_temporary_set_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 2, &isnull);
if (isnull) {
elog(ERROR, "Expected temporary boolean to be returned, but found NULL");
throw std::runtime_error("Expected temporary boolean to be returned, but found NULL");
}

Oid relid = DatumGetObjectId(relid_datum);
Expand All @@ -599,7 +590,7 @@ DECLARE_PG_FUNCTION(duckdb_alter_table_trigger) {
*/
DECLARE_PG_FUNCTION(duckdb_grant_trigger) {
if (!CALLED_AS_EVENT_TRIGGER(fcinfo)) /* internal error */
elog(ERROR, "not fired by event trigger manager");
throw std::runtime_error("not fired by event trigger manager");

if (!pgduckdb::IsExtensionRegistered()) {
/*
Expand Down Expand Up @@ -637,7 +628,7 @@ DECLARE_PG_FUNCTION(duckdb_grant_trigger) {
Oid relation_oid = RangeVarGetRelid(object, AccessShareLock, false);
Relation relation = RelationIdGetRelation(relation_oid);
if (pgduckdb::IsMotherDuckTable(relation)) {
elog(ERROR, "MotherDuck tables do not support GRANT");
throw std::runtime_error("MotherDuck tables do not support GRANT");
}
RelationClose(relation);
}
Expand Down
27 changes: 13 additions & 14 deletions src/pgduckdb_duckdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,38 +132,36 @@ DuckDBManager::LoadSecrets(duckdb::ClientContext &context) {

int secret_id = 0;
for (auto &secret : duckdb_secrets) {
StringInfo secret_key = makeStringInfo();
std::ostringstream query;
bool is_r2_cloud_secret = (secret.type.rfind("R2", 0) == 0);
appendStringInfo(secret_key, "CREATE SECRET pgduckb_secret_%d ", secret_id);
appendStringInfo(secret_key, "(TYPE %s, KEY_ID '%s', SECRET '%s'", secret.type.c_str(), secret.key_id.c_str(),
secret.secret.c_str());
query << "CREATE SECRET pgduckb_secret_" << secret_id << " ";
query << "(TYPE " << secret.type << ", KEY_ID '" << secret.key_id << "', SECRET '" << secret.secret << "'";
if (secret.region.length() && !is_r2_cloud_secret) {
appendStringInfo(secret_key, ", REGION '%s'", secret.region.c_str());
query << ", REGION '" << secret.region << "'";
}
if (secret.session_token.length() && !is_r2_cloud_secret) {
appendStringInfo(secret_key, ", SESSION_TOKEN '%s'", secret.session_token.c_str());
query << ", SESSION_TOKEN '" << secret.session_token << "'";
}
if (secret.endpoint.length() && !is_r2_cloud_secret) {
appendStringInfo(secret_key, ", ENDPOINT '%s'", secret.endpoint.c_str());
query << ", ENDPOINT '" << secret.endpoint << "'";
}
if (is_r2_cloud_secret) {
appendStringInfo(secret_key, ", ACCOUNT_ID '%s'", secret.endpoint.c_str());
query << ", ACCOUNT_ID '" << secret.endpoint << "'";
}
if (!secret.use_ssl) {
appendStringInfo(secret_key, ", USE_SSL 'FALSE'");
query << ", USE_SSL 'FALSE'";
}
appendStringInfo(secret_key, ");");
DuckDBQueryOrThrow(context, secret_key->data);
query << ");";

pfree(secret_key->data);
DuckDBQueryOrThrow(context, query.str());
secret_id++;
secret_table_num_rows = secret_id;
}
}

void
DuckDBManager::DropSecrets(duckdb::ClientContext &context) {
for (auto secret_id = 0; secret_id < secret_table_num_rows; secret_id++) {
for (auto secret_id = 0; secret_id < secret_table_num_rows; ++secret_id) {
auto drop_secret_cmd = duckdb::StringUtil::Format("DROP SECRET pgduckb_secret_%d;", secret_id);
pgduckdb::DuckDBQueryOrThrow(context, drop_secret_cmd);
}
Expand Down Expand Up @@ -200,7 +198,8 @@ DuckDBManager::LoadExtensions(duckdb::ClientContext &context) {
duckdb::unique_ptr<duckdb::Connection>
DuckDBManager::CreateConnection() {
if (!pgduckdb::IsDuckdbExecutionAllowed()) {
elog(ERROR, "DuckDB execution is not allowed because you have not been granted the duckdb.postgres_role");
throw std::runtime_error(
"DuckDB execution is not allowed because you have not been granted the duckdb.postgres_role");
}

auto &instance = Get();
Expand Down
5 changes: 3 additions & 2 deletions src/pgduckdb_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,9 @@ DuckdbInstallExtension(Datum name_datum) {
ON CONFLICT (name) DO UPDATE SET enabled = true
)",
lengthof(arg_types), arg_types, values, NULL, false, 0);
if (ret != SPI_OK_INSERT)
elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret));
if (ret != SPI_OK_INSERT) {
throw std::runtime_error("SPI_exec failed: error code " + std::string(SPI_result_code_string(ret)));
}
SPI_finish();

return true;
Expand Down
Loading