MIME-Version: 1.0
References: <a855795d-e697-4fa5-8698-d20122126567@eisentraut.org>
 <CAExHW5s3BL_qUwQ=ye8RX7GnVcGC+FeR7h6jFocBZC5JFXATEQ@mail.gmail.com>
 <04c0abe2-e9c5-4ffc-97b2-bedc1e3cd178@eisentraut.org> <CAExHW5vhiawMAEtRwV=X-zQmJeUVY2iyuskWFFKobm_ZvgMf9Q@mail.gmail.com>
 <CAExHW5tf9UDzhNWXHyDNBG4DRoq81-Ty4XqcY8wiixt844B0eA@mail.gmail.com>
 <CAExHW5uF7KMcABuNVB1de6JRY3ne-jYEP3+8ycQZGkFA=1OTLw@mail.gmail.com>
 <CA+UBfakRHrTqTkx8W0cmxGD7zfnZkZ4nczUvp0FGF68LvMSG-A@mail.gmail.com>
 <CAExHW5uykgWS=4fqL8eJqCKOQcHgtK89=Nq7eS_av-28ge=LKQ@mail.gmail.com>
 <CA+UBfamThQm9FQ7Kv=HQZPDy5CfULpn6yuU0bzPy-kX-=-jV4Q@mail.gmail.com>
 <CABRHmytyWxGMaQejiZ28OubtqnOUfzB9nZQSctyvsmxvS_cKCw@mail.gmail.com>
 <CABRHmyuvgFEXNgUsxXiSHh01iKcLrqyvcDWeOP+wuXjbMRYkEw@mail.gmail.com>
 <CABRHmytkwZNiXEqLKx7r6w3xVL8pCiFLaG8_a=BkqfcuumM1vA@mail.gmail.com>
 <CAExHW5vqo7iTcVznspb3HHD87Ps3Q=JF6_gg+h5mhyuwhd3Q4Q@mail.gmail.com>
 <CAExHW5serNyfY4v9oy6us=tM7ZyAd3_gYnMjOmRPk8eWEskSog@mail.gmail.com>
 <3b190de8-910a-4091-82f9-e98cf562d832@postgresfriends.org>
 <CAExHW5tYchoePjt3Z69zSmgMW50_jxNfpUiodRcQABUY6V8oxw@mail.gmail.com>
 <CAExHW5vG8sgT7H=Q6V_+aMF3f3vs+F7Mu_3AFrufhD=BPkBP0A@mail.gmail.com>
 <CAExHW5tDDwUK0dVqFRyUag8UsABmFNWhz8E6AVPDV2BXJLsY5w@mail.gmail.com>
 <CAEG8a3KFWD-a-jRtA__opQQaC9Z=_+rw=Gtydk1zStR-roMwWw@mail.gmail.com>
 <CAExHW5sM+ZGVzR1428FsDHuWc-FU2-6zQr5j91KLh6vZaWY0ow@mail.gmail.com>
 <CAExHW5t9OkLPv5g68C9=kaduyaz2HZQ2bHzOT3d421Wm4_teuA@mail.gmail.com>
 <CAEG8a3KLQ4GLKA7cLNPfMVNMy7-QEGCW4jVz+pa+ErYbgmFh9w@mail.gmail.com>
 <CAExHW5soDmT=yuXOqGLuNbp-_p-0rGD_CbAQ0PqROUKNBtJsBw@mail.gmail.com> <CAEG8a3L_Q8chyLQsOLtfLFfOHRW=sL_akKrtmh334RfjgGvWKg@mail.gmail.com>
In-Reply-To: <CAEG8a3L_Q8chyLQsOLtfLFfOHRW=sL_akKrtmh334RfjgGvWKg@mail.gmail.com>
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Fri, 24 Jan 2025 23:56:06 +0800
Message-ID: <CAEG8a3L-BC85c6fTUK0iU+TEcubRKODqGAf=kkwf=YSynKL2PA@mail.gmail.com>
Subject: Re: SQL Property Graph Queries (SQL/PGQ)
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Vik Fearing <vik@postgresfriends.org>, Ajay Pal <ajay.pal.k@gmail.com>, 
	Imran Zaheer <imran.zhir@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAEG8a3L-BC85c6fTUK0iU%2BTEcubRKODqGAf%3Dkkwf%3DYSynKL2PA%40mail.gmail.com>
Precedence: bulk

On Fri, Jan 24, 2025 at 11:46=E2=80=AFPM Junwang Zhao <zhjwpku@gmail.com> w=
rote:
>
> On Fri, Jan 24, 2025 at 9:31=E2=80=AFPM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Sun, Jan 19, 2025 at 6:45=E2=80=AFPM Junwang Zhao <zhjwpku@gmail.com=
> wrote:
> > >
> > > Hi Ashutosh,
> > >
> > > On Wed, Jan 1, 2025 at 5:02=E2=80=AFPM Ashutosh Bapat
> > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 1, 2025 at 2:22=E2=80=AFPM Ashutosh Bapat
> > > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > > >
> > > > > On Sat, Dec 21, 2024 at 6:21=E2=80=AFPM Junwang Zhao <zhjwpku@gma=
il.com> wrote:
> > > > > Thanks Junwang for your review.
> > > > >
> > > > > > Here are some review opinions for 0001, I haven't seen the othe=
r
> > > > > > patches, forgive me if some of the opinions have already been
> > > > > > addressed.
> > > > > >
> > > > > > 1. In propgraph_edge_get_ref_keys, when finding a matching fore=
ign key,
> > > > > > the fk pointer will always point to last ForeignKeyCacheInfo of
> > > > > > the edge relation, which is wrong. We should add another pointe=
r
> > > > > > that remembers the matched ForeignKeyCacheInfo to ref_rel. See
> > > > > > attached 0001.
> > > > >
> > > > > Nice catch. I fixed it in a slightly different way reducing overa=
ll
> > > > > code by using foreach_node(). I have merged this as part of 0004 =
which
> > > > > has fixes for several other issues. Interestingly there was a SQL=
 that
> > > > > had revealed the problem in create_property_graph.sql. But we had
> > > > > accepted a wrong output. Corrected that as well.
> > > > >
> > > > > >
> > > > > > 2. Some of the TODOs seem easy to address, attached 0002 does t=
his.
> > > > >
> > > > > From a cursory glance those changes look useful and mostly correc=
t. It
> > > > > will be good if you can provide a SQL test for those, covering th=
at
> > > > > part of the code. Please post the whole patch-set with your chang=
es as
> > > > > a separate commit/patch.
> > > > >
> > > > > >
> > > > > > 3. Since property name and label name are unique in property gr=
aph
> > > > > > scope, some of the wording are not accurate. See attached 0003.
> > > > >
> > > > >      <para>
> > > > > -      For each property graph element, all properties with the s=
ame name must
> > > > > -      have the same expression for each label.  For example, thi=
s would be
> > > > > +      For each property graph, all properties with the same name=
 must
> > > > > +      have the same expression.  For example, this would be
> > > > >
> > > > > Each property graph element may have a property with the same nam=
e
> > > > > associated with multiple labels. But each of those properties sho=
uld
> > > > > have the same expression. You may see that as the same property b=
eing
> > > > > defined multiple times once in each label it is associated with O=
R
> > > > > multiple properties with the same name. Current wording uses the
> > > > > latter notion, so it looks fine to me. The changes made to error
> > > > > messages are not needed with this notion.
> > > >
> > > > My last email is held for moderation. It will arrive once moderator=
s
> > > > release it. In the meantime trying to send the patches as a zip fil=
e
> > > > in a hope that it won't require moderation.
> > > >
> > > > --
> > > > Best Wishes,
> > > > Ashutosh Bapat
> > >
> > > 0007-RLS-tests-20250101.patch introduces regression test failure:
> > >
> > > +WARNING:  outfuncs/readfuncs failed to produce an equal rewritten pa=
rse tree
> > >
> > > P.S. It seems the patch set doesn't apply to master.
> >
> > Thanks for noticing it.
> >
> > Here's rebased patch set on the current head with some minor conflicts
> > in various files fixed. I did not see the said failure on my laptop.
>
> I figured out it's because I have `-DWRITE_READ_PARSE_PLAN_TREES` this op=
tion,
> removing this option clears the warning, but I'm not sure if this is a
> hidden issue.

Cirrus meson FreeBSD build has this setting, the regression test
rowsecurity failed
the same as my local machine, see [0], so I think this is an issue we
have to resolve.

Cirrus meson Bookworm build failed 002_pg_upgrade.pl and 027_stream_regress=
.pl,
see [1]

[0] https://cirrus-ci.com/task/5702052891852800
[1] https://cirrus-ci.com/task/6265002845274112

>
> >
> > 0001-0009 patches are the same as the previous set sans mergeconflict f=
ixes.
> > 0010 - is test for \dGx - to make sure that the extended format output
> > works with \dG. We may decide not to have this patch in the final
> > commit. But no harm in being there til that point.
>
> I have some changes based on the latest patch set. I attached two patches=
 with
> the following summary.
>
> 0001:
> - doc: some of the query in ddl.sgml can not be executed
> - after path factor was introduced, some comments doesn't apply
>
> 0002:
> - add a get_propgraph_element_alias_name function and do some trivial ref=
actor
>
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Regards
> Junwang Zhao


--=20
Regards
Junwang Zhao