Re: SQL Property Graph Queries (SQL/PGQ)

Hi Amit and Ashutosh,

On Tue, Mar 11, 2025 at 8:19 PM Junwang Zhao <zhjwpku(at)gmail(dot)com> wrote:
>
> Hi Amit,
>
> On Tue, Mar 11, 2025 at 5:06 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >
> > Hi Ashutosh, Junwang,
> >
> > On Tue, Mar 11, 2025 at 4:22 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > > On Wed, Feb 26, 2025 at 8:04 PM Junwang Zhao <zhjwpku(at)gmail(dot)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

Here is a new version with Amit's fix and my trivial refactors.

0001-0010 is the same as Ashutosh's last email
0011 is Amit's fix of the crash in ExecCheckPermissions
0012 is a trivial fix that remove the test with default collation, or
it will fail CI, see[1]
0013-0015 are some trivial fix and refactor, feel free to incorporate
or drop when you review them.

[1]: https://api.cirrus-ci.com/v1/artifact/task/5290818690351104/testrun/build/testrun/regress/regress/regression.diffs

--
Regards
Junwang Zhao