Re: SQL Property Graph Queries (SQL/PGQ) - Mailing list pgsql-hackers

Hi Amit,

On Tue, Mar 11, 2025 at 5:06 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Ashutosh, Junwang,
>
> On Tue, Mar 11, 2025 at 4:22 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> > On Wed, Feb 26, 2025 at 8:04 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > I added a trivial fix(v12-0014) that called table_open/table_close in
> > > rewriteGraphTable, it now passed the regression test and cirrus ci test,
> > > but I'm not sure it's the correct fix.
> > >
> > > I hope Ashutosh can chime in and take a look at this problem.
> >
> > 2. Following Assertion is failing, the assertion was added recently.
> > TRAP: failed Assert("IsParallelWorker() ||
> > CheckRelationOidLockedByMe(rte->relid, AccessShareLock, true)"), File:
> > "../../coderoot/pg/src/backend/executor/execMain.c", Line: 695, PID:
> > 303994
> > postgres: ashutosh regression [local]
> > SELECT(ExceptionalCondition+0xbe)[0x5c838c5d7114]
> > postgres: ashutosh regression [local]
> > SELECT(ExecCheckPermissions+0xf8)[0x5c838c11fb9c]
> > postgres: ashutosh regression [local] SELECT(+0x38223f)[0x5c838c12023f]
> > postgres: ashutosh regression [local]
> > SELECT(standard_ExecutorStart+0x2f8)[0x5c838c11f223]
> > postgres: ashutosh regression [local] SELECT(ExecutorStart+0x69)[0x5c838c11ef22]
> > postgres: ashutosh regression [local] SELECT(PortalStart+0x368)[0x5c838c3d991a]
> > postgres: ashutosh regression [local] SELECT(+0x63458e)[0x5c838c3d258e]
> > postgres: ashutosh regression [local] SELECT(PostgresMain+0x9eb)[0x5c838c3d7cf0]
> > postgres: ashutosh regression [local] SELECT(+0x630178)[0x5c838c3ce178]
> > postgres: ashutosh regression [local]
> > SELECT(postmaster_child_launch+0x137)[0x5c838c2da677]
> > postgres: ashutosh regression [local] SELECT(+0x5431b4)[0x5c838c2e11b4]
> > postgres: ashutosh regression [local] SELECT(+0x54076a)[0x5c838c2de76a]
> > postgres: ashutosh regression [local]
> > SELECT(PostmasterMain+0x15f8)[0x5c838c2de04d]
> > postgres: ashutosh regression [local] SELECT(main+0x3a1)[0x5c838c1b12be]
> > /lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7eda9ea29d90]
> > /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7eda9ea29e40]
> > postgres: ashutosh regression [local] SELECT(_start+0x25)[0x5c838be7c025]
> > 2025-03-11 11:40:01.696 IST postmaster[303081] LOG: client backend
> > (PID 303994) was terminated by signal 6: Aborted
> > 2025-03-11 11:40:01.696 IST postmaster[303081] DETAIL: Failed process
> > was running: select * from atpgv1;
> > I tried to investigate the Assertion, it's failing for property graph
> > RTE which is turned into subquery RTE. It has the right lock mode set,
> > but I haven't been able to figure out where the lock is supposed to be
> > taken or where it's released. If we just prepare the failing query and
> > execute the prepared statement, it does not trip the assertion. Will
> > investigate it more.
>
> I reproduced the crash using the example Junwang gave.
>
> The problem seems to be that RTEs of rtekind RTE_GRAPH_TABLE are not
> handled in AcquireRewriteLocks().  You'll need to add a case for
> RTE_GRAPH_TABLE similar to RTE_RELATION in the following switch of
> that function:
>
>     /*
>      * First, process RTEs of the current query level.
>      */
>     rt_index = 0;
>     foreach(l, parsetree->rtable)
>     {
>         RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
>         Relation    rel;
>         LOCKMODE    lockmode;
>         List       *newaliasvars;
>         Index       curinputvarno;
>         RangeTblEntry *curinputrte;
>         ListCell   *ll;
>
>         ++rt_index;
>         switch (rte->rtekind)
>         {
>             case RTE_RELATION:
>
> which could be as simple as the following (fixes the crash!) or
> something that's specific to RTE_GRAPH_TABLE:
>
> diff --git a/src/backend/rewrite/rewriteHandler.c
> b/src/backend/rewrite/rewriteHandler.c
> index c51dd3d2ee4..8fa6edb90eb 100644
> --- a/src/backend/rewrite/rewriteHandler.c
> +++ b/src/backend/rewrite/rewriteHandler.c
> @@ -174,6 +174,7 @@ AcquireRewriteLocks(Query *parsetree,
>          switch (rte->rtekind)
>          {
>              case RTE_RELATION:
> +            case RTE_GRAPH_TABLE:
>
> --
> Thanks, Amit Langote

I’ve tested the fix, it works and is better than my previous solution.
I will send a new patch set with this improvement, thanks.

--
Regards
Junwang Zhao