Thread: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Farias de Oliveira

Date:

13 July 2023, 22:14:32

Hello, my name is Matheus Farias and this is the first time that I'm sending an email to the pgsql-hackers list. I'm a software developer intern at Bitnine Global Inc. and, along with other interns, we've been working on updating Apache AGE with the latest version of Postgres, the REL_16_BETA version. One of the main problems that we are facing is that the code was reworked to update the permission checking and now some of the queries return ERROR: invalid perminfoindex <rte->perminfoindex> in RTE with relid <rte->relid>. This occurs due to one of the RTEs having perminfoindex = 0 and the relid containing a value.

AGE is a Postgres extension which allows us to execute openCypher commands to create a graph with nodes and edges. There are two main tables that are created: _ag_label_vertex and _ag_label_edge. Both of them will be the parent label tables of every other vertex/edge label we create.

When we do a simple MATCH query to find all nodes with the v label:

SELECT * FROM cypher('cypher_set', $$
MATCH (n:v)
RETURN n
$$) AS (node agtype);


inside the add_rtes_to_flat_rtable() function, it goes inside a loop where we can see the stored RTEs in root->parse->rtable:

// I've simplified what every RTE shows.

root->parse->rtable
[    (rtekind = RTE_SUBQUERY, relid =     0, perminfoindex = 0),    (rtekind = RTE_SUBQUERY, relid =     0, perminfoindex = 0),    (rtekind = RTE_SUBQUERY, relid =     0, perminfoindex = 0),    (rtekind = RTE_RELATION, relid = 16991, perminfoindex = 1)
]

But executing the query with a simple SET clause:

SELECT * FROM cypher('cypher_set', $$
MATCH (n) 
SET n.i = 3
$$) AS (a agtype);

One of the RTEs of the RTE_RELATION type and relid with a not null value has perminfoindex = 0

root->parse->rtable
[    (rtekind = RTE_SUBQUERY, relid =     0, perminfoindex = 0),    (rtekind = RTE_RELATION, relid = 16971, perminfoindex = 1),    (rtekind = RTE_RELATION, relid = 16971, perminfoindex = 1),    (rtekind = RTE_RELATION, relid = 16991, perminfoindex = 0)
]

the relid = 16991 is related to the child vertex label and the relid = 16971 related to the parent vertex label:

SELECT to_regclass('cypher_set._ag_label_vertex')::oid; to_regclass 
-------------       16971

SELECT to_regclass('cypher_set.v')::oid; to_regclass 
-------------       16991


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}

And the pnsi->p_rte has:

{type = T_RangeTblEntry, rtekind = RTE_SUBQUERY, relid = 0, relkind = 0 '\000', rellockmode = 0,   tablesample = 0x0, perminfoindex = 0, subquery = 0x11ed710, security_barrier = false,   jointype = JOIN_INNER, joinmergedcols = 0, joinaliasvars = 0x0, joinleftcols = 0x0, joinrightcols = 0x0,   join_using_alias = 0x0, functions = 0x0, funcordinality = false, tablefunc = 0x0, values_lists = 0x0,   ctename = 0x0, ctelevelsup = 0, self_reference = false, coltypes = 0x0, coltypmods = 0x0,   colcollations = 0x0, enrname = 0x0, enrtuples = 0, alias = 0x12055f0, eref = 0x1205638, lateral = false,   inh = false, inFromCl = true, securityQuals = 0x0}

Then it calls addNSItemToQuery(pstate, pnsi, true, false, true);. This function adds the given nsitem/RTE as a top-level entry in the pstate's join list and/or namespace list. I've been thinking if adding the nsitem/RTE like this won't cause this error?

Also in handle_prev_clause it has the following line, which is going to add all the rte's attributes to the current queries targetlist which, again, I'm not sure if that's what causing the problem because the relid of the rte is 0:

query->targetList = expandNSItemAttrs(pstate, pnsi, 0, true, -1);

If someone knows more about it, I would be grateful for any kind of answer or help. AGE's source code can be found here: https://github.com/apache/age

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Tom Lane

Date:

14 July 2023, 01:12:53

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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Amit Langote

Date:

14 July 2023, 06:05:11

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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Farias de Oliveira

Date:

14 July 2023, 17:30:38

Thanks Amit and Tom for the quick response. I have attached a file that contains the execution of the code via GDB and also what the backtrace command shows when it gets the error. If I forgot to add something or if it is necessary to add anything else, please let me know.

Thank you,

Matheus Farias

Em sex., 14 de jul. de 2023 às 00:05, Amit Langote <amitlangote09@gmail.com> escreveu:

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

Attachment

debugging_PG16_gdb.txt

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Tom Lane

Date:

14 July 2023, 18:16:50

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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Farias de Oliveira

Date:

14 July 2023, 22:43:01

I believe I have found something interesting that might be the root of the problem with RTEPermissionInfo. But I do not know 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:

static Query *analyze_cypher_clause(transform_method transform,
cypher_clause *clause,
cypher_parsestate *parent_cpstate)
{
cypher_parsestate *cpstate;
Query *query;
ParseState *parent_pstate = (ParseState*)parent_cpstate;
ParseState *pstate;

cpstate = make_cypher_parsestate(parent_cpstate);
pstate = (ParseState*)cpstate;

/* copy the expr_kind down to the child */
pstate->p_expr_kind = parent_pstate->p_expr_kind;

query = transform(cpstate, clause);

advance_transform_entities_to_next_clause(cpstate->entities);

parent_cpstate->entities = list_concat(parent_cpstate->entities,
cpstate->entities);

free_cypher_parsestate(cpstate);

return query;

}

the free_cypher_parsestate() function calls the free_parsestate() function:

void free_cypher_parsestate(cypher_parsestate *cpstate)
{
free_parsestate((ParseState *)cpstate);
}

So, before that happens the cpstate struct contains the following data:

{pstate = {parentParseState = 0x2b06ab0, p_sourcetext = 0x2b06ef0 "MATCH (n) SET n.i = 3", p_rtable = 0x2bdb370,
p_rteperminfos = 0x2bdb320, p_joinexprs = 0x0, p_nullingrels = 0x0, p_joinlist = 0x2bdb478, p_namespace = 0x2bdb4c8,
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_FROM_SUBSELECT, p_next_resno = 2,
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 = 0x2c6e158, property_constraint_quals = 0x0,
exprHasAgg = false, p_opt_match = false}

And then after that the pstate gets all wiped out:

{pstate = {parentParseState = 0x0, p_sourcetext = 0x2b06ef0 "MATCH (n) SET n.i = 3", p_rtable = 0x0,
p_rteperminfos = 0x0, p_joinexprs = 0x0, p_nullingrels = 0x0, p_joinlist = 0x0, p_namespace = 0x0,
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_FROM_SUBSELECT, p_next_resno = 1,
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}

But in transform_cypher_clause_as_subquery(), we use the same pstate. And when we assign

pnsi = addRangeTableEntryForSubquery(pstate, query, alias, lateral, true);

The pstate changes, adding a value to p_rtable but nothing in p_rteperminfos.

Then after that addNSItemToQuery(pstate, pnsi, true, false, true) is called, changing the pstate to add values to p_joinlist and p_namespace.

It ends up going inside other functions and changing it more a bit, but at the end of one of these functions it assigns values 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 pstate has 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.

Em sex., 14 de jul. de 2023 às 12:16, Tom Lane <tgl@sss.pgh.pa.us> escreveu:

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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Amit Langote

Date:

18 July 2023, 12:58:20

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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Farias de Oliveira

Date:

20 July 2023, 23:05:29

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 it should 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 ResultRelInfo must'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?

Thank you,

Matheus Farias

Em ter., 18 de jul. de 2023 às 06:58, Amit Langote <amitlangote09@gmail.com> escreveu:

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 not know 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 assigns values 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 pstate has 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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Amit Langote

Date:

26 July 2023, 14:30:01

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

Re: In Postgres 16 BETA, should the ParseNamespaceItem have the same index as it's RangeTableEntry?

From

Farias de Oliveira

Date:

28 July 2023, 21:37:39

Hello,

Thank you Amit, changing the 3rd argument to 0 fixes some of the errors (there are 6 out of 24 errors still failing) but it throws a new one "ERROR: bad buffer ID: 0". We will need to take a more in depth look here on why this is occuring, but thank you so much for the help.

Thank you,

Matheus Farias

Em qua., 26 de jul. de 2023 às 08:30, Amit Langote <amitlangote09@gmail.com> escreveu:

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 it should 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 ResultRelInfo must'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