Re: SQL Property Graph Queries (SQL/PGQ)

Hi Ashutosh,

On Fri, Dec 6, 2024 at 12:34 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Sorry, I forgot to attach patches. PFA the patches.
>
> On Thu, Dec 5, 2024 at 4:38 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Fri, Nov 22, 2024 at 7:29 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Nov 19, 2024 at 10:08 PM Vik Fearing <vik(at)postgresfriends(dot)org> wrote:
> > > >
> > > >
> > > > On 05/11/2024 16:41, Ashutosh Bapat wrote:
> > > >
> > > > On Wed, Aug 28, 2024 at 3:48 PM Ashutosh Bapat
> > > > <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > > >
> > > > Patches 0001 - 0006 are same as the previous set.
> > > > 0007 - fixes all the problems you reported till now and also the one I
> > > > found. The commit message describes the fixes in detail.
> > > >
> > > > Here's an updated patchset based on the latest HEAD.
> > > >
> > > >
> > > >
> > > > I have been looking at this patchset from a user's and standards' perspective and I am quite pleased with what I am seeing for the most part. I have not been looking much at the code itself, although I do plan on reviewing some of the code in the future.
> > >
> > > Thanks for looking at the patches.
> > >
> > > >
> > > >
> > > > There are a few things that stick out to me.
> > > >
> > > >
> > > > 1.
> > > > I don't see any way to view the structure of of a property graph. For example:
> > > >
> > > >
> > > > postgres=# CREATE TABLE objects (id INTEGER, color VARCHAR, shape VARCHAR, size INTEGER);
> > > > CREATE TABLE
> > > > postgres=# CREATE PROPERTY GRAPH propgraph VERTEX TABLES (objects KEY (id) PROPERTIES ALL COLUMNS);
> > > > CREATE PROPERTY GRAPH
> > > > postgres=# \dG propgraph
> > > > List of relations
> > > > Schema | Name | Type | Owner
> > > > -------------------+-----------+----------------+-------------
> > > > graph_table_tests | propgraph | property graph | vik.fearing
> > > > (1 row)
> > > >
> > > > postgres=# \d propgraph
> > > > Property graph "graph_table_tests.propgraph"
> > > > Column | Type
> > > > --------+------
> > > >
> > > > I don't really know what to do with that.
> > >
> > > Yes. It is on my TODO list. IMO we should just print the first line
> > > property graph "...". There are no predefined columns in this
> > > relation. And somehow redirect them to use \dG instead.
> >
> > \d+ propgraph prints the definition of property graph. I find \dG
> > similar to \di which doesn't print the structure of an index. Instead
> > \d+ <index name> prints it.
> >
> > In the attached patch series I have added patch 0008 to remove
> > unnecessary header
> > > > Column | Type
> > > > --------+------
> >
> > It still prints an extra line but I haven't found a way to eliminate
> > it. Hence 0008 is WIP. I have listed TODOs in the commit message of
> > that patch.
> >
> >
> > > >
> > > >
> > > > 2.
> > > > There is a missing newline in the \? help of psql.
> > > > HELP0(" \\dFt[+] [PATTERN] list text search templates\n");
> > > > HELP0(" \\dg[S+] [PATTERN] list roles\n");
> > > > HELP0(" \\dG[S+] [PATTERN] list property graphs"); <---
> > > > HELP0(" \\di[S+] [PATTERN] list indexes\n");
> > > > HELP0(" \\dl[+] list large objects, same as \\lo_list\n");
> > > >
> > >
> > > Will fix that in the next set.
> >
> > Done. The fix is part of 0001 now.
> >
> >
> >
> > >
> > > >
> > > > I will continue to review this feature from the user's perspective. Thank you for working on it, I am very excited to get this in.
> > >
> >
> > here's patchset rebased on 792b2c7e6d926e61e8ff3b33d3e22d7d74e7a437
> > with conflicts in rowsecurity.sql/out fixed.
> >
> > >
> > > 0001 - 0005 are the same as the previous set.
> > > 0007 - has RLS tests. It is the same as 0006 from the previous patch set.
> >
> > This is same as the previous patchset.
> >
> > >
> > > 0006 - is new addressing collation and edge-vertex link qual creation.
> > > This patch adds code to store collation and typmod to
> > > pg_propgraph_property catalog and propagate it to property graph
> > > references in GRAPH_TABLE. Collation is used by
> > > assign_query_collations and assign_expr_collations to propagate
> > > collation up the query and expression tree respectively. typmod is
> > > used to report typmod of property reference from exprTypemod().
> > >
> > > While finishing code to create vertex-edge link quals, I found that we
> > > need to find and store the operator to be used for key matching in
> > > those quals. I think we have to do something similar to what primary
> > > key handling code does - find the equality operator when creating edge
> > > descriptor and store it in pg_propgraph_element. I am not sure whether
> > > we should add a dependency on the operator. I will look into this
> > > next.
> >
> > 0006 in the attached patch series completes this work. Now we find the
> > equality operators to be used for vertex-edge quals and save it in
> > pg_propgraph_element catalog and also add a dependency between the
> > edge element and the equality operators.
> >
> > 0008 - has WIP fix for \d and \d+
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat

Here are some review opinions for 0001, I haven't seen the other
patches, forgive me if some of the opinions have already been
addressed.

1. In propgraph_edge_get_ref_keys, when finding a matching foreign key,
the fk pointer will always point to last ForeignKeyCacheInfo of
the edge relation, which is wrong. We should add another pointer
that remembers the matched ForeignKeyCacheInfo to ref_rel. See
attached 0001.

2. Some of the TODOs seem easy to address, attached 0002 does this.

3. Since property name and label name are unique in property graph
scope, some of the wording are not accurate. See attached 0003.

--
Regards
Junwang Zhao