Обсуждение: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

Farias de Oliveira <matheusfarias519@gmail.com> writes:
> With further inspection in AGE's code, after executing the SET query,
> it goes inside transform_cypher_clause_as_subquery() function and the
> ParseNamespaceItem has the following values:

>  {p_names = 0x1205638, p_rte = 0x11edb70, p_rtindex = 1, p_perminfo =
> 0x7f7f7f7f7f7f7f7f,
>   p_nscolumns = 0x1205848, p_rel_visible = true, p_cols_visible =
> true, p_lateral_only = false,
>   p_lateral_ok = true}

Hmm, that uninitialized value for p_perminfo is pretty suspicious.
I see that transformFromClauseItem and buildNSItemFromLists both
create ParseNamespaceItems without bothering to fill p_perminfo,
while buildNSItemFromTupleDesc fills it per the caller and
addRangeTableEntryForJoin always sets it to NULL.  I think we
ought to make the first two set it to NULL as well, because
uninitialized fields are invariably a bad idea (even though the
lack of valgrind complaints says that the core code is managing
to avoid touching those fields).

If we do that, is it sufficient to resolve your problem?

            regards, tom lane

On Fri, Jul 14, 2023 at 7:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Farias de Oliveira <matheusfarias519@gmail.com> writes:
> > With further inspection in AGE's code, after executing the SET query,
> > it goes inside transform_cypher_clause_as_subquery() function and the
> > ParseNamespaceItem has the following values:
>
> >  {p_names = 0x1205638, p_rte = 0x11edb70, p_rtindex = 1, p_perminfo =
> > 0x7f7f7f7f7f7f7f7f,
> >   p_nscolumns = 0x1205848, p_rel_visible = true, p_cols_visible =
> > true, p_lateral_only = false,
> >   p_lateral_ok = true}
>
> Hmm, that uninitialized value for p_perminfo is pretty suspicious.
> I see that transformFromClauseItem and buildNSItemFromLists both
> create ParseNamespaceItems without bothering to fill p_perminfo,
> while buildNSItemFromTupleDesc fills it per the caller and
> addRangeTableEntryForJoin always sets it to NULL.  I think we
> ought to make the first two set it to NULL as well, because
> uninitialized fields are invariably a bad idea (even though the
> lack of valgrind complaints says that the core code is managing
> to avoid touching those fields).

Agreed, I'll go ahead and fix that.

> If we do that, is it sufficient to resolve your problem?

Hmm, I'm afraid maybe not, because if the above were the root issue,
we'd have seen a segfault and not the error the OP mentioned?  I'm
thinking the issue is that their code appears to be consing up an RTE
that the core code (getRTEPermissionInfo() most likely via
markRTEForSelectPriv()) is not expecting to be called with?  I would
be helpful to see a backtrace when the error occurs to be sure.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Farias de Oliveira <matheusfarias519@gmail.com> writes:
> 3905            elog(ERROR, "invalid perminfoindex %u in RTE with relid %u",
> (gdb) bt
> #0  getRTEPermissionInfo (rteperminfos=0x138a500, rte=0x138a6b0) at parse_relation.c:3905
> #1  0x0000000000676e29 in GetResultRTEPermissionInfo (relinfo=relinfo@entry=0x13b8f50, estate=estate@entry=0x138ce48)
>     at execUtils.c:1412
> #2  0x0000000000677c30 in ExecGetUpdatedCols (relinfo=relinfo@entry=0x13b8f50, estate=estate@entry=0x138ce48)
>     at execUtils.c:1321
> #3  0x0000000000677cd7 in ExecGetAllUpdatedCols (relinfo=relinfo@entry=0x13b8f50, estate=estate@entry=0x138ce48)
>     at execUtils.c:1362
> #4  0x000000000066b9bf in ExecUpdateLockMode (estate=estate@entry=0x138ce48, relinfo=relinfo@entry=0x13b8f50) at
execMain.c:2385
> #5  0x00007f197fb19a8d in update_entity_tuple (resultRelInfo=<optimized out>, resultRelInfo@entry=0x13b8f50,
>     elemTupleSlot=elemTupleSlot@entry=0x13b9730, estate=estate@entry=0x138ce48, old_tuple=0x13bae80)
>     at src/backend/executor/cypher_set.c:120
> #6  0x00007f197fb1a2ff in process_update_list (node=node@entry=0x138d0c8) at src/backend/executor/cypher_set.c:595
> #7  0x00007f197fb1a348 in process_all_tuples (node=node@entry=0x138d0c8) at src/backend/executor/cypher_set.c:212
> #8  0x00007f197fb1a455 in exec_cypher_set (node=0x138d0c8) at src/backend/executor/cypher_set.c:641

So apparently, what we have here is a result-relation RTE that has
no permissions info associated.  It's not clear to me whether that
is a bug in building the parse tree, or an expectable situation
that GetResultRTEPermissionInfo ought to be coping with.  I'm
inclined to bet on the former though, and to guess that AGE is
missing dealing with the new RTEPermissionInfo structs in someplace
or other.  I'm afraid that allowing GetResultRTEPermissionInfo to
let this pass without comment would mask actual bugs, so fixing
it at that end doesn't seem attractive.

            regards, tom lane

Hello,

On Sat, Jul 15, 2023 at 4:43 AM Farias de Oliveira
<matheusfarias519@gmail.com> wrote:
> I believe I have found something interesting that might be the root of the problem with RTEPermissionInfo. But I do
notknow how to fix it exactly. In AGE's code, the execution of it goes through a function called
analyze_cypher_clause()which does the following: 
>
> It ends up going inside other functions and changing it more a bit, but at the end of one of these functions it
assignsvalues to some members of the query: 
>
> query->targetList = lappend(query->targetList, tle);
> query->rtable = pstate->p_rtable;
> query->jointree = makeFromExpr(pstate->p_joinlist, NULL);
>
> I assume that here is missing the assignment of query->rteperminfos to be the same as pstate->p_rteperminfos, but the
pstatehas the following values: 
>
> {pstate = {parentParseState = 0x0, p_sourcetext = 0x2b06ef0 "MATCH (n) SET n.i = 3", p_rtable = 0x2c6e590,
>     p_rteperminfos = 0x0, p_joinexprs = 0x0, p_nullingrels = 0x0, p_joinlist = 0x2c6e678, p_namespace = 0x2c6e6c8,
>     p_lateral_active = false, p_ctenamespace = 0x0, p_future_ctes = 0x0, p_parent_cte = 0x0, p_target_relation = 0x0,
>     p_target_nsitem = 0x0, p_is_insert = false, p_windowdefs = 0x0, p_expr_kind = EXPR_KIND_NONE, p_next_resno = 3,
>     p_multiassign_exprs = 0x0, p_locking_clause = 0x0, p_locked_from_parent = false, p_resolve_unknowns = true,
>     p_queryEnv = 0x0, p_hasAggs = false, p_hasWindowFuncs = false, p_hasTargetSRFs = false, p_hasSubLinks = false,
>     p_hasModifyingCTE = false, p_last_srf = 0x0, p_pre_columnref_hook = 0x0, p_post_columnref_hook = 0x0,
>     p_paramref_hook = 0x0, p_coerce_param_hook = 0x0, p_ref_hook_state = 0x0}, graph_name = 0x2b06e50 "cypher_set",
>   graph_oid = 16942, params = 0x0, default_alias_num = 0, entities = 0x2c6e228, property_constraint_quals = 0x0,
>   exprHasAgg = false, p_opt_match = false}
>
> So changing that won't solve the issue.

Does p_rtable in this last pstate contain any RTE_RELATION entries?
If it does, p_rteperminfos being NIL looks like a bug in your code.

Actually, given the back trace of the error that you shared, I am
suspecting more of a problem in the code that generates a
ResultRelInfo pointing at the wrong RTE via its ri_RangeTableIndex.
That code should perhaps set the ri_RangeTableIndex to 0 if it doesn't
know the actual existing RTE corresponding to that result relation.
If you set it to some non-0 value, the RTE that it points to should
satisfy invariants such as having the corresponding RTEPermissionInfo
present in the rteperminfos list if necessary.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Hello,

On Fri, Jul 21, 2023 at 5:05 AM Farias de Oliveira
<matheusfarias519@gmail.com> wrote:
>
> Hello,
>
> Thank you for the help guys and I'm so sorry for my late response. Indeed, the error relies on the ResultRelInfo. In
GetResultRTEPermissionInfo()function, it does a checking on the relinfo->ri_RootResultRelInfo variable. I believe that
itshould go inside this scope: 
>
>
>         if (relinfo->ri_RootResultRelInfo)
> {
> /*
> * For inheritance child result relations (a partition routing target
> * of an INSERT or a child UPDATE target), this returns the root
> * parent's RTE to fetch the RTEPermissionInfo because that's the only
> * one that has one assigned.
> */
> rti = relinfo->ri_RootResultRelInfo->ri_RangeTableIndex;
> }
>
> The relinfo contains:
>
> {type = T_ResultRelInfo, ri_RangeTableIndex = 5, ri_RelationDesc = 0x7f44e3308cc8, ri_NumIndices = 0,
ri_IndexRelationDescs= 0x0, ri_IndexRelationInfo = 0x0, ri_RowIdAttNo = 0, 
>   ri_extraUpdatedCols = 0x0, ri_projectNew = 0x0, ri_newTupleSlot = 0x0, ri_oldTupleSlot = 0x0,
ri_projectNewInfoValid= false, ri_TrigDesc = 0x0, ri_TrigFunctions = 0x0, 
>   ri_TrigWhenExprs = 0x0, ri_TrigInstrument = 0x0, ri_ReturningSlot = 0x0, ri_TrigOldSlot = 0x0, ri_TrigNewSlot =
0x0,ri_FdwRoutine = 0x0, ri_FdwState = 0x0, 
>   ri_usesFdwDirectModify = false, ri_NumSlots = 0, ri_NumSlotsInitialized = 0, ri_BatchSize = 0, ri_Slots = 0x0,
ri_PlanSlots= 0x0, ri_WithCheckOptions = 0x0, 
>   ri_WithCheckOptionExprs = 0x0, ri_ConstraintExprs = 0x0, ri_GeneratedExprsI = 0x0, ri_GeneratedExprsU = 0x0,
ri_NumGeneratedNeededI= 0, ri_NumGeneratedNeededU = 0, 
>   ri_returningList = 0x0, ri_projectReturning = 0x0, ri_onConflictArbiterIndexes = 0x0, ri_onConflict = 0x0,
ri_matchedMergeAction= 0x0, ri_notMatchedMergeAction = 0x0, 
>   ri_PartitionCheckExpr = 0x0, ri_ChildToRootMap = 0x0, ri_ChildToRootMapValid = false, ri_RootToChildMap = 0x0,
ri_RootToChildMapValid= false, ri_RootResultRelInfo = 0x0, 
>   ri_PartitionTupleSlot = 0x0, ri_CopyMultiInsertBuffer = 0x0, ri_ancestorResultRels = 0x0}
>
> Since relinfo->ri_RootResultRelInfo = 0x0, the rti will have no value and Postgres will interpret that the
ResultRelInfomust've been created only for filtering triggers and the relation is not being inserted into. 
> The relinfo variable is created with the create_entity_result_rel_info() function:
>
> ResultRelInfo *create_entity_result_rel_info(EState *estate, char *graph_name,
>                                              char *label_name)
> {
>     RangeVar *rv;
>     Relation label_relation;
>     ResultRelInfo *resultRelInfo;
>
>     ParseState *pstate = make_parsestate(NULL);
>
>     resultRelInfo = palloc(sizeof(ResultRelInfo));
>
>     if (strlen(label_name) == 0)
>     {
>         rv = makeRangeVar(graph_name, AG_DEFAULT_LABEL_VERTEX, -1);
>     }
>     else
>     {
>         rv = makeRangeVar(graph_name, label_name, -1);
>     }
>
>     label_relation = parserOpenTable(pstate, rv, RowExclusiveLock);
>
>     // initialize the resultRelInfo
>     InitResultRelInfo(resultRelInfo, label_relation,
>                       list_length(estate->es_range_table), NULL,
>                       estate->es_instrument);
>
>     // open the parse state
>     ExecOpenIndices(resultRelInfo, false);
>
>     free_parsestate(pstate);
>
>     return resultRelInfo;
> }
>
> In this case, how can we get the relinfo->ri_RootResultRelInfo to store the appropriate data?

Your function doesn't seem to have access to the ModifyTableState
node, so setting ri_RootResultRelInfo to the correct ResultRelInfo
node does not seem doable.

As I suggested in my previous reply, please check if passing 0 (not
list_length(estate->es_range_table)) for the 3rd argument
InitResultRelInfo() fixes the problem and gives the correct result.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com