From d10cc3ed4815b00b021a22ffe884ac76b8113b84 Mon Sep 17 00:00:00 2001 From: Deepakshi Mittal <78574784+deepakshi-mittal@users.noreply.github.com> Date: Wed, 11 Oct 2023 15:54:40 -0700 Subject: [PATCH 1/2] Support for Instead of Trigger On Views in Babelfish (#225) * Support for Instead of Trigger On Views in Babelfish Currently Postgres does not support Instead of Trigger On Views for DML queries. On running such a query on VIEW, query rewriting substitutes view with underlying base table. Added a hook for skipping the substitution for DML query on the VIEW. This change will add support for Instaed of Triggers for DML queries when using Babelfish. Task: BABEL-2170 Signed-off-by: Deepakshi Mittal --- src/backend/commands/trigger.c | 2 +- src/backend/executor/execMain.c | 7 ++++--- src/backend/rewrite/rewriteHandler.c | 8 ++++++-- src/include/executor/executor.h | 3 +++ 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 295a424229c..bfabcdd0a4c 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -489,7 +489,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, RelationGetRelationName(rel)), errdetail("Triggers on foreign tables cannot have transition tables."))); - if (rel->rd_rel->relkind == RELKIND_VIEW) + if (rel->rd_rel->relkind == RELKIND_VIEW && sql_dialect != SQL_DIALECT_TSQL) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("\"%s\" is a view", diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 44566a34281..d6aafe37483 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -54,6 +54,7 @@ #include "jit/jit.h" #include "mb/pg_wchar.h" #include "miscadmin.h" +#include "parser/parser.h" #include "parser/parsetree.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" @@ -1029,7 +1030,7 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation) switch (operation) { case CMD_INSERT: - if (!trigDesc || !trigDesc->trig_insert_instead_row) + if (!trigDesc || (!trigDesc->trig_insert_instead_row && (sql_dialect == SQL_DIALECT_TSQL && !trigDesc->trig_insert_instead_statement))) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot insert into view \"%s\"", @@ -1037,7 +1038,7 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation) errhint("To enable inserting into the view, provide an INSTEAD OF INSERT trigger or an unconditional ON INSERT DO INSTEAD rule."))); break; case CMD_UPDATE: - if (!trigDesc || !trigDesc->trig_update_instead_row) + if (!trigDesc || (!trigDesc->trig_update_instead_row && (sql_dialect == SQL_DIALECT_TSQL && !trigDesc->trig_update_instead_statement))) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot update view \"%s\"", @@ -1045,7 +1046,7 @@ CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation) errhint("To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule."))); break; case CMD_DELETE: - if (!trigDesc || !trigDesc->trig_delete_instead_row) + if (!trigDesc || (!trigDesc->trig_delete_instead_row && (sql_dialect == SQL_DIALECT_TSQL && !trigDesc->trig_delete_instead_statement))) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot delete from view \"%s\"", diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 95849951a2b..ffbdf2b4a00 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -33,6 +33,7 @@ #include "nodes/nodeFuncs.h" #include "optimizer/optimizer.h" #include "parser/analyze.h" +#include "parser/parser.h" #include "parser/parse_coerce.h" #include "parser/parse_relation.h" #include "parser/parsetree.h" @@ -45,6 +46,7 @@ #include "utils/lsyscache.h" #include "utils/rel.h" +bbfViewHasInsteadofTrigger_hook_type bbfViewHasInsteadofTrigger_hook = NULL; /** BBF Hook to check Instead Of trigger on View */ /* We use a list of these to detect recursion in RewriteQuery */ typedef struct rewrite_event @@ -1472,7 +1474,8 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti, */ isAutoUpdatableView = false; if (target_relation->rd_rel->relkind == RELKIND_VIEW && - !view_has_instead_trigger(target_relation, CMD_INSERT)) + (!view_has_instead_trigger(target_relation, CMD_INSERT) && + !(sql_dialect == SQL_DIALECT_TSQL && bbfViewHasInsteadofTrigger_hook && (bbfViewHasInsteadofTrigger_hook)(target_relation, CMD_INSERT)))) { List *locks; bool hasUpdate; @@ -3953,7 +3956,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length) */ if (!instead && rt_entry_relation->rd_rel->relkind == RELKIND_VIEW && - !view_has_instead_trigger(rt_entry_relation, event)) + (!view_has_instead_trigger(rt_entry_relation, event) + && !(sql_dialect == SQL_DIALECT_TSQL && bbfViewHasInsteadofTrigger_hook && (bbfViewHasInsteadofTrigger_hook)(rt_entry_relation, event)))) { /* * If there were any qualified INSTEAD rules, don't allow the view diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index e04021d0213..0f010de4251 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -87,6 +87,9 @@ extern PGDLLIMPORT ExecutorCheckPerms_hook_type ExecutorCheckPerms_hook; typedef bool (*TriggerRecuresiveCheck_hook_type) (ResultRelInfo *resultRelInfo); extern PGDLLIMPORT TriggerRecuresiveCheck_hook_type TriggerRecuresiveCheck_hook; +typedef bool (*bbfViewHasInsteadofTrigger_hook_type) (Relation view, CmdType event); +extern PGDLLIMPORT bbfViewHasInsteadofTrigger_hook_type bbfViewHasInsteadofTrigger_hook; + typedef bool (*check_rowcount_hook_type) (int es_processed); extern PGDLLIMPORT check_rowcount_hook_type check_rowcount_hook; From f492997643b8584bf7e5cef670fb41727199c600 Mon Sep 17 00:00:00 2001 From: Dipesh Dhameliya Date: Thu, 12 Oct 2023 17:59:22 +0530 Subject: [PATCH 2/2] Do not re-do permission checks on range table for parallel worker (#233) With current design of postgres, workflow of parallel worker looks like something below: ``` -- main node ExecutorRun standard_ExecutorStart InitPlan ExecCheckRTPerms <-- does the permission check on all the range table entries from planner stmt standard_ExecutorRun ExecutePlan . . . ExecGather gather_getnext -- spawns initialises and run parallel workers . . . -- parallel worker ParallelQueryMain ExecutorRun standard_ExecutorStart InitPlan ExecCheckRTPerms <- redo the permission check on same set of table standard_ExecutorRun . . . ``` Here, Main worker would not have spawned the worker if there is any permission check failures on any of the range table. And parallel worker is again doing the permission check on same set of tables which is kind of redundant. So this commit avoid that unnecessary permission check. This change is also helpful when select query or subquery involves T-SQL temp tables. Currently, Babelfish is throwing error like "relation with OID 19401 does not exist" because metadata for temp tables is being stored in ENR metadata (which is backend memory) and is not being shared with parallel worker. So this changes will also avoid such error for temp table from parallel worker. Task: BABEL-4450 Signed-off-by: Dipesh Dhameliya --- src/backend/executor/execMain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index d6aafe37483..a2f2e2049a2 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -54,7 +54,7 @@ #include "jit/jit.h" #include "mb/pg_wchar.h" #include "miscadmin.h" -#include "parser/parser.h" +#include "parser/parser.h" #include "parser/parsetree.h" #include "storage/bufmgr.h" #include "storage/lmgr.h" @@ -818,9 +818,10 @@ InitPlan(QueryDesc *queryDesc, int eflags) int i; /* - * Do permissions checks + * Do permissions checks if not parallel worker */ - ExecCheckRTPerms(rangeTable, true); + if (!(sql_dialect == SQL_DIALECT_TSQL && IsParallelWorker())) + ExecCheckRTPerms(rangeTable, true); /* * initialize the node's execution state