From f7acf86a1ad7762a4f3839e9073425209291848c Mon Sep 17 00:00:00 2001 From: Lakshmi Narayanan Sreethar Date: Fri, 15 Sep 2023 11:28:06 +0530 Subject: [PATCH] PG16: Update query relation permission checking PG16 moves the permission checking information out of the range table entries into a new struct - RTEPermissionInfo. This commit updates timescaledb to use the this new mechanism when compiling with PG16. postgres/postgres@a61b1f74 postgres/postgres@b803b7d1 postgres/postgres@47bb9db7 --- src/copy.c | 37 ++++++++++------ src/nodes/chunk_dispatch/chunk_insert_state.c | 4 ++ src/planner/expand_hypertable.c | 16 ++++++- src/planner/planner.c | 13 +++++- tsl/src/continuous_aggs/create.c | 3 ++ tsl/src/continuous_aggs/finalize.c | 43 +++++++++++++++++-- tsl/src/fdw/modify_exec.c | 9 +++- tsl/src/fdw/modify_plan.c | 15 +++++-- tsl/src/fdw/scan_exec.c | 18 +++++++- .../nodes/decompress_chunk/decompress_chunk.c | 15 +++++-- tsl/src/nodes/decompress_chunk/planner.c | 15 +++++-- 11 files changed, 159 insertions(+), 29 deletions(-) diff --git a/src/copy.c b/src/copy.c index 4a8da45dff4..484fee598bd 100644 --- a/src/copy.c +++ b/src/copy.c @@ -694,7 +694,7 @@ has_other_before_insert_row_trigger_than_ts(ResultRelInfo *resultRelInfo) * Use COPY FROM to copy data from file to relation. */ static uint64 -copyfrom(CopyChunkState *ccstate, List *range_table, Hypertable *ht, MemoryContext copycontext, +copyfrom(CopyChunkState *ccstate, ParseState *pstate, Hypertable *ht, MemoryContext copycontext, void (*callback)(void *), void *arg) { ResultRelInfo *resultRelInfo; @@ -719,7 +719,7 @@ copyfrom(CopyChunkState *ccstate, List *range_table, Hypertable *ht, MemoryConte ExprState *qualexpr = NULL; ChunkDispatch *dispatch = ccstate->dispatch; - Assert(range_table); + Assert(pstate->p_rtable); if (ccstate->rel->rd_rel->relkind != RELKIND_RELATION) { @@ -813,7 +813,12 @@ copyfrom(CopyChunkState *ccstate, List *range_table, Hypertable *ht, MemoryConte NULL, 0); #else - ExecInitRangeTable(estate, range_table); +#if PG16_LT + ExecInitRangeTable(estate, pstate->p_rtable); +#else + Assert(pstate->p_rteperminfos != NULL); + ExecInitRangeTable(estate, pstate->p_rtable, pstate->p_rteperminfos); +#endif ExecInitResultRelation(estate, resultRelInfo, 1); #endif @@ -825,7 +830,7 @@ copyfrom(CopyChunkState *ccstate, List *range_table, Hypertable *ht, MemoryConte estate->es_result_relations = resultRelInfo; estate->es_num_result_relations = 1; estate->es_result_relation_info = resultRelInfo; - estate->es_range_table = range_table; + estate->es_range_table = pstate->p_rtable; ExecInitRangeTable(estate, estate->es_range_table); #endif @@ -1259,6 +1264,8 @@ copy_constraints_and_check(ParseState *pstate, Relation rel, List *attnums) addRangeTableEntryForRelation(pstate, rel, RowExclusiveLock, NULL, false, false); RangeTblEntry *rte = nsitem->p_rte; addNSItemToQuery(pstate, nsitem, true, true, true); + +#if PG16_LT rte->requiredPerms = ACL_INSERT; foreach (cur, attnums) @@ -1268,6 +1275,18 @@ copy_constraints_and_check(ParseState *pstate, Relation rel, List *attnums) } ExecCheckRTPerms(pstate->p_rtable, true); +#else + RTEPermissionInfo *perminfo = nsitem->p_perminfo; + perminfo->requiredPerms = ACL_INSERT; + + foreach (cur, attnums) + { + int attno = lfirst_int(cur) - FirstLowInvalidHeapAttributeNumber; + perminfo->insertedCols = bms_add_member(perminfo->insertedCols, attno); + } + + ExecCheckPermissions(pstate->p_rtable, list_make1(perminfo), true); +#endif /* * Permission check for row security policies. @@ -1390,8 +1409,7 @@ timescaledb_DoCopy(const CopyStmt *stmt, const char *queryString, uint64 *proces /* Or create a new memory context. */ copycontext = AllocSetContextCreate(CurrentMemoryContext, "COPY", ALLOCSET_DEFAULT_SIZES); #endif - *processed = - copyfrom(ccstate, pstate->p_rtable, ht, copycontext, CopyFromErrorCallback, cstate); + *processed = copyfrom(ccstate, pstate, ht, copycontext, CopyFromErrorCallback, cstate); } copy_chunk_state_destroy(ccstate); @@ -1467,12 +1485,7 @@ timescaledb_move_from_table_to_chunks(Hypertable *ht, LOCKMODE lockmode) snapshot = RegisterSnapshot(GetLatestSnapshot()); scandesc = table_beginscan(rel, snapshot, 0, NULL); ccstate = copy_chunk_state_create(ht, rel, next_copy_from_table_to_chunks, NULL, scandesc); - copyfrom(ccstate, - pstate->p_rtable, - ht, - copycontext, - copy_table_to_chunk_error_callback, - scandesc); + copyfrom(ccstate, pstate, ht, copycontext, copy_table_to_chunk_error_callback, scandesc); copy_chunk_state_destroy(ccstate); table_endscan(scandesc); UnregisterSnapshot(snapshot); diff --git a/src/nodes/chunk_dispatch/chunk_insert_state.c b/src/nodes/chunk_dispatch/chunk_insert_state.c index 35dd0dc2ec2..c44779a6531 100644 --- a/src/nodes/chunk_dispatch/chunk_insert_state.c +++ b/src/nodes/chunk_dispatch/chunk_insert_state.c @@ -650,12 +650,16 @@ ts_chunk_insert_state_create(const Chunk *chunk, ChunkDispatch *dispatch) if (chunk->relkind == RELKIND_FOREIGN_TABLE) { +#if PG16_LT RangeTblEntry *rte = rt_fetch(relinfo->ri_RangeTableIndex, dispatch->estate->es_range_table); Assert(rte != NULL); state->user_id = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId(); +#else + state->user_id = ExecGetResultRelCheckAsUser(relinfo, state->estate); +#endif state->chunk_data_nodes = ts_chunk_data_nodes_copy(chunk); } diff --git a/src/planner/expand_hypertable.c b/src/planner/expand_hypertable.c index bffc60068b2..befba9ecd5c 100644 --- a/src/planner/expand_hypertable.c +++ b/src/planner/expand_hypertable.c @@ -1424,7 +1424,14 @@ ts_plan_expand_hypertable_chunks(Hypertable *ht, PlannerInfo *root, RelOptInfo * childrte->inh = false; /* clear the magic bit */ childrte->ctename = NULL; +#if PG16_LT childrte->requiredPerms = 0; +#else + /* Since PG16, the permission info is maintained separetely. Unlink + * the old perminfo from the RTE to disable permission checking. + */ + childrte->perminfoindex = 0; +#endif childrte->securityQuals = NIL; parse->rtable = lappend(parse->rtable, childrte); child_rtindex = list_length(parse->rtable); @@ -1474,8 +1481,15 @@ ts_plan_expand_hypertable_chunks(Hypertable *ht, PlannerInfo *root, RelOptInfo * data_node_rte->inh = false; data_node_rte->ctename = NULL; - data_node_rte->requiredPerms = 0; data_node_rte->securityQuals = NIL; +#if PG16_LT + data_node_rte->requiredPerms = 0; +#else + /* Since PG16, the permission info is maintained separetely. Unlink + * the old perminfo from the RTE to disable permission checking. + */ + data_node_rte->perminfoindex = 0; +#endif parse->rtable = lappend(parse->rtable, data_node_rte); rti = list_length(parse->rtable); root->simple_rte_array[rti] = data_node_rte; diff --git a/src/planner/planner.c b/src/planner/planner.c index fe0c1a26c8a..65e48f16f28 100644 --- a/src/planner/planner.c +++ b/src/planner/planner.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -1319,6 +1320,14 @@ timescaledb_get_relation_info_hook(PlannerInfo *root, Oid relation_objectid, boo Query *query = root->parse; Hypertable *ht; const TsRelType type = ts_classify_relation(root, rel, &ht); + +#if PG16_LT + AclMode requiredPerms = rte->requiredPerms; +#else + RTEPermissionInfo *perminfo = getRTEPermissionInfo(query->rteperminfos, rte); + AclMode requiredPerms = perminfo->requiredPerms; +#endif + switch (type) { case TS_REL_HYPERTABLE: @@ -1334,11 +1343,11 @@ timescaledb_get_relation_info_hook(PlannerInfo *root, Oid relation_objectid, boo * assumes permission checking has been done already during the first planner call. * We don't want to touch the UPDATE/DELETEs, so we need to check all the regular * conditions here that are checked during preprocess_query, as well as the - * condition that rte->requiredPerms is not requiring UPDATE/DELETE on this rel. + * condition that requiredPerms is not requiring UPDATE/DELETE on this rel. */ if (ts_guc_enable_optimizations && ts_guc_enable_constraint_exclusion && inhparent && rte->ctename == NULL && !IS_UPDL_CMD(query) && query->resultRelation == 0 && - query->rowMarks == NIL && (rte->requiredPerms & (ACL_UPDATE | ACL_DELETE)) == 0) + query->rowMarks == NIL && (requiredPerms & (ACL_UPDATE | ACL_DELETE)) == 0) { rte_mark_for_expansion(rte); } diff --git a/tsl/src/continuous_aggs/create.c b/tsl/src/continuous_aggs/create.c index 1077ec5c474..72fa1bb846e 100644 --- a/tsl/src/continuous_aggs/create.c +++ b/tsl/src/continuous_aggs/create.c @@ -557,6 +557,9 @@ mattablecolumninfo_get_partial_select_query(MatTableColumnInfo *mattblinfo, Quer CAGG_MAKEQUERY(partial_selquery, userview_query); partial_selquery->rtable = copyObject(userview_query->rtable); partial_selquery->jointree = copyObject(userview_query->jointree); +#if PG16_GE + partial_selquery->rteperminfos = copyObject(userview_query->rteperminfos); +#endif partial_selquery->targetList = mattblinfo->partial_seltlist; partial_selquery->groupClause = mattblinfo->partial_grouplist; diff --git a/tsl/src/continuous_aggs/finalize.c b/tsl/src/continuous_aggs/finalize.c index 85b929a19fc..2b8818adb50 100644 --- a/tsl/src/continuous_aggs/finalize.c +++ b/tsl/src/continuous_aggs/finalize.c @@ -9,6 +9,9 @@ * Continuous Aggregates (with partials) */ #include "finalize.h" + +#include + #include "create.h" #include "common.h" #include @@ -609,6 +612,9 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, ListCell *lc; FromExpr *fromexpr; RangeTblEntry *rte; +#if PG16_GE + RTEPermissionInfo *perminfo; +#endif /* * For initial cagg creation rtable will have only 1 entry, @@ -628,6 +634,9 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, rte->inh = true; rte->rellockmode = 1; rte->eref = copyObject(rte->alias); +#if PG16_GE + perminfo = addRTEPermissionInfo(&final_selquery->rteperminfos, rte); +#endif ListCell *l; foreach (l, inp->final_userquery->jointree->fromlist) { @@ -648,7 +657,13 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, #if PG14_GE rte->join_using_alias = jrte->join_using_alias; #endif +#if PG16_LT rte->selectedCols = jrte->selectedCols; +#else + RTEPermissionInfo *jperminfo = + getRTEPermissionInfo(inp->final_userquery->rteperminfos, jrte); + perminfo->selectedCols = jperminfo->selectedCols; +#endif } } } @@ -656,7 +671,12 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, { rte = llast_node(RangeTblEntry, inp->final_userquery->rtable); rte->eref->colnames = NIL; +#if PG16_LT rte->selectedCols = NULL; +#else + perminfo = getRTEPermissionInfo(inp->final_userquery->rteperminfos, rte); + perminfo->selectedCols = NULL; +#endif } if (rte->eref->colnames == NIL) { @@ -671,18 +691,28 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, { ColumnDef *cdef = lfirst_node(ColumnDef, lc); rte->eref->colnames = lappend(rte->eref->colnames, makeString(cdef->colname)); - rte->selectedCols = bms_add_member(rte->selectedCols, - list_length(rte->eref->colnames) - - FirstLowInvalidHeapAttributeNumber); + int attno = list_length(rte->eref->colnames) - FirstLowInvalidHeapAttributeNumber; +#if PG16_LT + rte->selectedCols = bms_add_member(rte->selectedCols, attno); +#else + perminfo->selectedCols = bms_add_member(perminfo->selectedCols, attno); +#endif } } rte->relid = mattbladdress->objectId; rte->rtekind = RTE_RELATION; rte->relkind = RELKIND_RELATION; rte->tablesample = NULL; +#if PG16_LT rte->requiredPerms |= ACL_SELECT; rte->insertedCols = NULL; rte->updatedCols = NULL; +#else + perminfo->relid = mattbladdress->objectId; + perminfo->requiredPerms |= ACL_SELECT; + perminfo->insertedCols = NULL; + perminfo->updatedCols = NULL; +#endif /* 2. Fixup targetlist with the correct rel information. */ foreach (lc, inp->final_seltlist) @@ -709,6 +739,10 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, { RangeTblRef *rtr; final_selquery->rtable = list_make1(rte); +#if PG16_GE + /* perminfo has been set already in the previous if/else */ + Assert(list_length(final_selquery->rteperminfos) == 1); +#endif rtr = makeNode(RangeTblRef); rtr->rtindex = 1; fromexpr = makeFromExpr(list_make1(rtr), NULL); @@ -716,6 +750,9 @@ finalizequery_get_select_query(FinalizeQueryInfo *inp, List *matcollist, else { final_selquery->rtable = inp->final_userquery->rtable; +#if PG16_GE + final_selquery->rteperminfos = inp->final_userquery->rteperminfos; +#endif fromexpr = inp->final_userquery->jointree; fromexpr->quals = NULL; } diff --git a/tsl/src/fdw/modify_exec.c b/tsl/src/fdw/modify_exec.c index 458c378771e..632c099ef59 100644 --- a/tsl/src/fdw/modify_exec.c +++ b/tsl/src/fdw/modify_exec.c @@ -5,6 +5,7 @@ */ #include #include +#include #include #include #include @@ -347,10 +348,16 @@ fdw_begin_foreign_modify(PlanState *pstate, ResultRelInfo *rri, CmdType operatio } /* Construct an execution state. */ +#if PG16_LT + Oid checkAsUser = rte->checkAsUser; +#else + RTEPermissionInfo *perminfo = getRTEPermissionInfo(estate->es_rteperminfos, rte); + Oid checkAsUser = perminfo->checkAsUser; +#endif fmstate = create_foreign_modify(estate, rri->ri_RelationDesc, operation, - rte->checkAsUser, + checkAsUser, subplan, query, target_attrs, diff --git a/tsl/src/fdw/modify_plan.c b/tsl/src/fdw/modify_plan.c index 28c88f9ed74..9cd5b4cbf1d 100644 --- a/tsl/src/fdw/modify_plan.c +++ b/tsl/src/fdw/modify_plan.c @@ -4,6 +4,7 @@ * LICENSE-TIMESCALE for a copy of the license. */ #include +#include #include #include #include @@ -33,12 +34,12 @@ get_insert_attrs(Relation rel) } static List * -get_update_attrs(RangeTblEntry *rte) +get_update_attrs(Bitmapset *updatedCols) { List *attrs = NIL; int col = -1; - while ((col = bms_next_member(rte->updatedCols, col)) >= 0) + while ((col = bms_next_member(updatedCols, col)) >= 0) { /* bit numbers are offset by FirstLowInvalidHeapAttributeNumber */ AttrNumber attno = col + FirstLowInvalidHeapAttributeNumber; @@ -187,7 +188,14 @@ fdw_plan_foreign_modify(PlannerInfo *root, ModifyTable *plan, Index result_relat &retrieved_attrs); break; case CMD_UPDATE: - target_attrs = get_update_attrs(rte); + { +#if PG16_LT + Bitmapset *updatedCols = rte->updatedCols; +#else + RTEPermissionInfo *perminfo = getRTEPermissionInfo(root->parse->rteperminfos, rte); + Bitmapset *updatedCols = perminfo->updatedCols; +#endif + target_attrs = get_update_attrs(updatedCols); deparseUpdateSql(&sql, rte, result_relation, @@ -197,6 +205,7 @@ fdw_plan_foreign_modify(PlannerInfo *root, ModifyTable *plan, Index result_relat &retrieved_attrs); data_nodes = get_chunk_data_nodes(rel->rd_id); break; + } case CMD_DELETE: deparseDeleteSql(&sql, rte, result_relation, rel, returning_list, &retrieved_attrs); data_nodes = get_chunk_data_nodes(rel->rd_id); diff --git a/tsl/src/fdw/scan_exec.c b/tsl/src/fdw/scan_exec.c index 44b809cd6c2..c6f799df6d0 100644 --- a/tsl/src/fdw/scan_exec.c +++ b/tsl/src/fdw/scan_exec.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -220,6 +221,7 @@ get_connection(ScanState *ss, Oid const server_id, Bitmapset *scanrelids, List * RangeTblEntry *rte; TSConnectionId id; int rtindex; + Oid user_oid; /* * Identify which user to do the remote access as. This should match what @@ -234,7 +236,21 @@ get_connection(ScanState *ss, Oid const server_id, Bitmapset *scanrelids, List * rte = rt_fetch(rtindex, estate->es_range_table); - remote_connection_id_set(&id, server_id, rte->checkAsUser ? rte->checkAsUser : GetUserId()); +#if PG16_LT + user_oid = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId(); +#else + if (rte->perminfoindex > 0) + { + RTEPermissionInfo *perminfo = getRTEPermissionInfo(estate->es_rteperminfos, rte); + user_oid = OidIsValid(perminfo->checkAsUser) ? perminfo->checkAsUser : GetUserId(); + } + else + { + user_oid = GetUserId(); + } +#endif + + remote_connection_id_set(&id, server_id, user_oid); return remote_dist_txn_get_connection(id, list_length(exprs) ? REMOTE_TXN_USE_PREP_STMT : diff --git a/tsl/src/nodes/decompress_chunk/decompress_chunk.c b/tsl/src/nodes/decompress_chunk/decompress_chunk.c index 8e06940ed97..13e634a07b8 100644 --- a/tsl/src/nodes/decompress_chunk/decompress_chunk.c +++ b/tsl/src/nodes/decompress_chunk/decompress_chunk.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -65,7 +66,8 @@ typedef enum MergeBatchResult SCAN_BACKWARD } MergeBatchResult; -static RangeTblEntry *decompress_chunk_make_rte(Oid compressed_relid, LOCKMODE lockmode); +static RangeTblEntry *decompress_chunk_make_rte(Oid compressed_relid, LOCKMODE lockmode, + Query *parse); static void create_compressed_scan_paths(PlannerInfo *root, RelOptInfo *compressed_rel, CompressionInfo *info, SortInfo *sort_info); @@ -1530,7 +1532,8 @@ decompress_chunk_add_plannerinfo(PlannerInfo *root, CompressionInfo *info, Chunk CACHE_FLAG_NONE)); expand_planner_arrays(root, 1); - info->compressed_rte = decompress_chunk_make_rte(compressed_reloid, AccessShareLock); + info->compressed_rte = + decompress_chunk_make_rte(compressed_reloid, AccessShareLock, root->parse); root->simple_rte_array[compressed_index] = info->compressed_rte; root->parse->rtable = lappend(root->parse->rtable, info->compressed_rte); @@ -1703,7 +1706,7 @@ create_compressed_scan_paths(PlannerInfo *root, RelOptInfo *compressed_rel, Comp * create RangeTblEntry for compressed chunk */ static RangeTblEntry * -decompress_chunk_make_rte(Oid compressed_relid, LOCKMODE lockmode) +decompress_chunk_make_rte(Oid compressed_relid, LOCKMODE lockmode, Query *parse) { RangeTblEntry *rte = makeNode(RangeTblEntry); Relation r = table_open(compressed_relid, lockmode); @@ -1745,11 +1748,17 @@ decompress_chunk_make_rte(Oid compressed_relid, LOCKMODE lockmode) rte->inh = false; rte->inFromCl = false; +#if PG16_LT rte->requiredPerms = 0; rte->checkAsUser = InvalidOid; /* not set-uid by default, either */ rte->selectedCols = NULL; rte->insertedCols = NULL; rte->updatedCols = NULL; +#else + /* add perminfo for the new RTE */ + RTEPermissionInfo *perminfo = addRTEPermissionInfo(&parse->rteperminfos, rte); + perminfo->requiredPerms |= ACL_SELECT; +#endif return rte; } diff --git a/tsl/src/nodes/decompress_chunk/planner.c b/tsl/src/nodes/decompress_chunk/planner.c index 27af0cc85e6..c7116875ab2 100644 --- a/tsl/src/nodes/decompress_chunk/planner.c +++ b/tsl/src/nodes/decompress_chunk/planner.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -67,7 +68,8 @@ check_for_system_columns(Bitmapset *attrs_used) * attnos. */ static void -build_decompression_map(DecompressChunkPath *path, List *scan_tlist, Bitmapset *chunk_attrs_needed) +build_decompression_map(PlannerInfo *root, DecompressChunkPath *path, List *scan_tlist, + Bitmapset *chunk_attrs_needed) { /* * Track which normal and metadata columns we were able to find in the @@ -77,13 +79,20 @@ build_decompression_map(DecompressChunkPath *path, List *scan_tlist, Bitmapset * bool missing_sequence = path->needs_sequence_num; Bitmapset *chunk_attrs_found = NULL; +#if PG16_LT + Bitmapset *selectedCols = path->info->ht_rte->selectedCols; +#else + RTEPermissionInfo *perminfo = + getRTEPermissionInfo(root->parse->rteperminfos, path->info->ht_rte); + Bitmapset *selectedCols = perminfo->selectedCols; +#endif /* * FIXME this way to determine which columns are used is actually wrong, see * https://github.com/timescale/timescaledb/issues/4195#issuecomment-1104238863 * Left as is for now, because changing it uncovers a whole new story with * ctid. */ - check_for_system_columns(path->info->ht_rte->selectedCols); + check_for_system_columns(selectedCols); /* * We allow tableoid system column, it won't be in the targetlist but will @@ -562,7 +571,7 @@ decompress_chunk_plan_create(PlannerInfo *root, RelOptInfo *rel, CustomPath *pat /* * Determine which compressed colum goes to which output column. */ - build_decompression_map(dcpath, compressed_scan->plan.targetlist, chunk_attrs_needed); + build_decompression_map(root, dcpath, compressed_scan->plan.targetlist, chunk_attrs_needed); /* Build heap sort info for sorted_merge_append */ List *sort_options = NIL;