Skip to content

Commit

Permalink
Move to other spot for less hacky approach
Browse files Browse the repository at this point in the history
  • Loading branch information
JelteF committed Jan 10, 2025
1 parent 4eb1673 commit 6bbbbc3
Showing 1 changed file with 21 additions and 22 deletions.
43 changes: 21 additions & 22 deletions src/pgduckdb_hooks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,26 @@ NeedsDuckdbExecution(Query *query) {
return ContainsDuckdbItems((Node *)query, NULL);
}

/*
* We only check ContainsFromClause if duckdb.force_execution is set to true.
*
* If there's no FROM clause, we're only selecting constants. From a
* performance perspective there's not really a point in using DuckDB. If we
* remove this check many common queries that are used to inspect postgres will
* throw a warning or return incorrect results. For example:
*
* SELECT current_setting('work_mem');
*
* So even if a user sets duckdb.force_execution = true, we still won't
* forward such queries to DuckDB. With one exception: If the query
* requires duckdb e.g. due to a duckdb-only function being used, we'll
* still executing this in DuckDB.
*/
static bool
ContainsFromClause(Query *query, bool throw_error = false) {
return query->rtable;
}

static bool
IsAllowedStatement(Query *query, bool throw_error = false) {
int elevel = throw_error ? ERROR : DEBUG4;
Expand All @@ -166,27 +186,6 @@ IsAllowedStatement(Query *query, bool throw_error = false) {
}
}

/*
* XXX: This is a bit of a weird one, because it is only allowed when
* IsAllowedStatement is called with throw_error.
*
* If there's no rtable, we're only selecting constants. From a performance
* perspective there's not really a point in using DuckDB. If we remove
* this heck many common queries that are used to inspect postgres will
* throw a warning or return incorrect results. For example:
*
* SELECT current_setting('work_mem');
*
* So even if a user sets duckdb.force_execution = true, we still won't
* forward such queries to DuckDB. With one exception: If the query
* requires duckdb e.g. due to a duckdb-only function being used, we'll
* still executing this in DuckDB.
*/
if (!query->rtable && !throw_error) {
elog(elevel, "DuckDB usage requires at least one table");
return false;
}

/*
* If any table is from pg_catalog, we don't want to use DuckDB. This is
* because DuckDB has its own pg_catalog tables that contain different data
Expand Down Expand Up @@ -216,7 +215,7 @@ DuckdbPlannerHook_Cpp(Query *parse, const char *query_string, int cursor_options
IsAllowedStatement(parse, true);

return DuckdbPlanNode(parse, query_string, cursor_options, bound_params, true);
} else if (duckdb_force_execution && IsAllowedStatement(parse)) {
} else if (duckdb_force_execution && IsAllowedStatement(parse) && ContainsFromClause(parse)) {
PlannedStmt *duckdbPlan = DuckdbPlanNode(parse, query_string, cursor_options, bound_params, false);
if (duckdbPlan) {
return duckdbPlan;
Expand Down

0 comments on commit 6bbbbc3

Please sign in to comment.