Обсуждение: SQL Property Graph Queries (SQL/PGQ)

Here is a prototype implementation of SQL property graph queries
(SQL/PGQ), following SQL:2023.  This was talked about briefly at the
FOSDEM developer meeting, and a few people were interested, so I
wrapped up what I had in progress into a presentable form.

There is some documentation to get started in doc/src/sgml/ddl.sgml
and doc/src/sgml/queries.sgml.

To learn more about this facility, here are some external resources:

* An article about a competing product:
   https://oracle-base.com/articles/23c/sql-property-graphs-and-sql-pgq-23c
   (All the queries in the article work, except the ones using
   vertex_id() and edge_id(), which are non-standard, and the JSON
   examples at the end, which require some of the in-progress JSON
   functionality for PostgreSQL.)

* An academic paper related to another competing product:
   https://www.cidrdb.org/cidr2023/papers/p66-wolde.pdf (The main part
   of this paper discusses advanced functionality that my patch doesn't
   have.)

* A 2019 presentation about graph databases:
   https://www.pgcon.org/2019/schedule/events/1300.en.html (There is
   also a video.)

* (Vik has a recent presentation "Property Graphs: When the Relational
   Model Is Not Enough", but I haven't found the content posted
   online.)

The patch is quite fragile, and treading outside the tested paths will
likely lead to grave misbehavior.  Use with caution.  But I feel that
the general structure is ok, and we just need to fill in the
proverbial few thousand lines of code in the designated areas.

Hi,

On 2024-02-16 15:53:11 +0100, Peter Eisentraut wrote:
> The patch is quite fragile, and treading outside the tested paths will
> likely lead to grave misbehavior.  Use with caution.  But I feel that
> the general structure is ok, and we just need to fill in the
> proverbial few thousand lines of code in the designated areas.

One aspect that I m concerned with structurally is that the transformation,
from property graph queries to something postgres understands, is done via the
rewrite system. I doubt that that is a good idea. For one it bars the planner
from making plans that benefit from the graph query formulation. But more
importantly, we IMO should reduce usage of the rewrite system, not increase
it.

Greetings,

Andres Freund

On 16.02.24 20:23, Andres Freund wrote:
> One aspect that I m concerned with structurally is that the transformation,
> from property graph queries to something postgres understands, is done via the
> rewrite system. I doubt that that is a good idea. For one it bars the planner
> from making plans that benefit from the graph query formulation. But more
> importantly, we IMO should reduce usage of the rewrite system, not increase
> it.

PGQ is meant to be implemented like that, like views expanding to joins 
and unions.  This is what I have gathered during the specification 
process, and from other implementations, and from academics.  There are 
certainly other ways to combine relational and graph database stuff, 
like with native graph storage and specialized execution support, but 
this is not that, and to some extent PGQ was created to supplant those 
other approaches.

Many people will agree that the rewriter is sort of weird and archaic at 
this point.  But I'm not aware of any plans or proposals to do anything 
about it.  As long as the view expansion takes place there, it makes 
sense to align with that.  For example, all the view security stuff 
(privileges, security barriers, etc.) will eventually need to be 
considered, and it would make sense to do that in a consistent way.  So 
for now, I'm working with what we have, but let's see where it goes.

(Note to self: Check that graph inside view inside graph inside view ... 
works.)

On 2/23/24 17:15, Peter Eisentraut wrote:
> On 16.02.24 20:23, Andres Freund wrote:
>> One aspect that I m concerned with structurally is that the
>> transformation,
>> from property graph queries to something postgres understands, is done
>> via the
>> rewrite system. I doubt that that is a good idea. For one it bars the
>> planner
>> from making plans that benefit from the graph query formulation. But more
>> importantly, we IMO should reduce usage of the rewrite system, not
>> increase
>> it.
> 
> PGQ is meant to be implemented like that, like views expanding to joins
> and unions.  This is what I have gathered during the specification
> process, and from other implementations, and from academics.  There are
> certainly other ways to combine relational and graph database stuff,
> like with native graph storage and specialized execution support, but
> this is not that, and to some extent PGQ was created to supplant those
> other approaches.
> 

I understand PGQ was meant to be implemented as a bit of a "syntactic
sugar" on top of relations, instead of inventing some completely new
ways to store/query graph data.

But does that really mean it needs to be translated to relations this
early / in rewriter? I haven't thought about it very deeply, but won't
that discard useful information about semantics of the query, which
might be useful when planning/executing the query?

I've somehow imagined we'd be able to invent some new index types, or
utilize some other type of auxiliary structure, maybe some special
executor node, but it seems harder without this extra info ...

> Many people will agree that the rewriter is sort of weird and archaic at
> this point.  But I'm not aware of any plans or proposals to do anything
> about it.  As long as the view expansion takes place there, it makes
> sense to align with that.  For example, all the view security stuff
> (privileges, security barriers, etc.) will eventually need to be
> considered, and it would make sense to do that in a consistent way.  So
> for now, I'm working with what we have, but let's see where it goes.
> 
> (Note to self: Check that graph inside view inside graph inside view ...
> works.)
> 

AFAIK the "policy" regarding rewriter was that we don't want to use it
for user stuff (e.g. people using it for partitioning), but I'm not sure
about internal stuff.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Feb 23, 2024 at 11:08 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 2/23/24 17:15, Peter Eisentraut wrote:
> > On 16.02.24 20:23, Andres Freund wrote:
> >> One aspect that I m concerned with structurally is that the
> >> transformation,
> >> from property graph queries to something postgres understands, is done
> >> via the
> >> rewrite system. I doubt that that is a good idea. For one it bars the
> >> planner
> >> from making plans that benefit from the graph query formulation. But more
> >> importantly, we IMO should reduce usage of the rewrite system, not
> >> increase
> >> it.
> >
> > PGQ is meant to be implemented like that, like views expanding to joins
> > and unions.  This is what I have gathered during the specification
> > process, and from other implementations, and from academics.  There are
> > certainly other ways to combine relational and graph database stuff,
> > like with native graph storage and specialized execution support, but
> > this is not that, and to some extent PGQ was created to supplant those
> > other approaches.
> >
>
> I understand PGQ was meant to be implemented as a bit of a "syntactic
> sugar" on top of relations, instead of inventing some completely new
> ways to store/query graph data.
>
> But does that really mean it needs to be translated to relations this
> early / in rewriter? I haven't thought about it very deeply, but won't
> that discard useful information about semantics of the query, which
> might be useful when planning/executing the query?
>
> I've somehow imagined we'd be able to invent some new index types, or
> utilize some other type of auxiliary structure, maybe some special
> executor node, but it seems harder without this extra info ...

I am yet to look at the implementation but ...
1. If there are optimizations that improve performance of some path
patterns, they are likely to improve the performance of joins used to
implement those. In such cases, loosing some information might be ok.
2. Explicit graph annotatiion might help to automate some things like
creating indexes automatically on columns that appear in specific
patterns OR create extended statistics automatically on the columns
participating in specific patterns. OR interpreting statistics/costing
in differently than normal query execution. Those kind of things will
require retaining annotations in views, planner/execution trees etc.
3. There are some things like aggregates/operations on paths which
might require stuff like new execution nodes. But I am not sure we
have reached that stage yet.

There might be things we may not see right now in the standard e.g.
indexes on graph properties. For those mapping the graph objects unto
database objects might prove useful. That goes back to Peter's comment
--- quote
As long as the view expansion takes place there, it makes
sense to align with that.  For example, all the view security stuff
(privileges, security barriers, etc.) will eventually need to be
considered, and it would make sense to do that in a consistent way.
--- unquote

--
Best Wishes,
Ashutosh Bapat

Here is a new version of this patch.  I have been working together with 
Ashutosh on this.  While the version 0 was more of a fragile demo, this 
version 1 has a fairly complete minimal feature set and should be useful 
for playing around with.  We do have a long list of various internal 
bits that still need to be fixed or revised or looked at again, so there 
is by no means a claim that everything is completed.

Documentation to get started is included (ddl.sgml and queries.sgml). 
(Of course, feedback on the getting-started documentation would be most 
welcome.)

In the ddl.sgml, I’d swap the first two paragraphs.
I find the first one a bit confusing as-is. As far as I can tell, it’s an implementation detail.
The first paragraph should answer, “I have some data modeled as a graph G=(V, E). Can Postgres help me?”.

Then, introducing property graphs makes more sense.

I'd also use the examples and fake data in `graph_table.sql` in ddl/queries.sgml).
I was bummed that that copy-pasting didn't work as is.
I’d keep explaining how a graph query translates to a relational one later in the page.

As for the implementation, I can’t have an opinion yet,
but for those not familiar, Apache Age uses a slightly different approach
that mimics jsonpath (parses a sublanguage expression into an internal execution engine etc.).
However, the standard requires mapping this to the relational model, which makes sense for core Postgres.


> On 27 Jun 2024, at 3:31 PM, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> Here is a new version of this patch.  I have been working together with Ashutosh on this.  While the version 0 was
moreof a fragile demo, this version 1 has a fairly complete minimal feature set and should be useful for playing around
with. We do have a long list of various internal bits that still need to be fixed or revised or looked at again, so
thereis by no means a claim that everything is completed. 
>
> Documentation to get started is included (ddl.sgml and queries.sgml). (Of course, feedback on the getting-started
documentationwould be most welcome.) 
> <v1-0001-WIP-SQL-Property-Graph-Queries-SQL-PGQ.patch>

On Mon, Jul 8, 2024 at 7:07 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
>
>
> On Thu, Jun 27, 2024 at 6:01 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> Here is a new version of this patch.  I have been working together with
>> Ashutosh on this.  While the version 0 was more of a fragile demo, this
>> version 1 has a fairly complete minimal feature set and should be useful
>> for playing around with.  We do have a long list of various internal
>> bits that still need to be fixed or revised or looked at again, so there
>> is by no means a claim that everything is completed.
>
>
> PFA the patchset fixing compilation error reported by CI bot.
> 0001 - same as previous one
> 0002 - fixes compilation error
> 0003 - adds support for WHERE clause in graph pattern missing in the first patch.
>

There's a test failure reported by CI. Property graph related tests
are failing when regression is run from perl tests. The failure is
reported only on Free BSD. I have added one patch in the series which
will help narrow the failure. The patch changes the code to report the
location of an error reported when handling implicit properties or
labels.
0001 - same as previous one
0002 - fixes pgperltidy complaints
0003 - fixes compilation failure
0004 - same as 0003 in previous set
0005 - patch to report parse location of error



--
Best Wishes,
Ashutosh Bapat

On Wed, Jul 17, 2024 at 11:04 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Mon, Jul 8, 2024 at 7:07 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> >
> >
> > On Thu, Jun 27, 2024 at 6:01 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >>
> >> Here is a new version of this patch.  I have been working together with
> >> Ashutosh on this.  While the version 0 was more of a fragile demo, this
> >> version 1 has a fairly complete minimal feature set and should be useful
> >> for playing around with.  We do have a long list of various internal
> >> bits that still need to be fixed or revised or looked at again, so there
> >> is by no means a claim that everything is completed.
> >
> >
> > PFA the patchset fixing compilation error reported by CI bot.
> > 0001 - same as previous one
> > 0002 - fixes compilation error
> > 0003 - adds support for WHERE clause in graph pattern missing in the first patch.
> >
>
> There's a test failure reported by CI. Property graph related tests
> are failing when regression is run from perl tests. The failure is
> reported only on Free BSD.

I thought it's related to FreeBSD but the bug could be observed
anywhere with -DRELCACHE_FORCE_RELEASE. It's also reported indirectly
by valgrind.

When infering properties of an element from the underlying table's
attributes, the attribute name pointed to the memory in the heap tuple
of pg_attribute row. Thus when the tuple was released, it pointed to a
garbage instead of actual column name resulting in column not found
error.

Attached set of patches with an additional patch to fix the bug.

0001 - same as previous one
0002 - fixes pgperltidy complaints
0003 - fixes compilation failure
0004 - fixes issue seen on CI
0005 - adds support for WHERE clause in graph pattern missing in the
first patch.

Once reviewed, patches 0002 to 0005 should be merged into 0001.

--
Best Wishes,
Ashutosh Bapat

Hi
I am attaching a new patch for a minor feature addition.

- Adding support for 'Labels and properties: EXCEPT list'

Please let me know if something is missing.

Thanks and Regards
Imran Zaheer

On Mon, Jul 22, 2024 at 9:02 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Jul 17, 2024 at 11:04 AM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Mon, Jul 8, 2024 at 7:07 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > >
> > >
> > > On Thu, Jun 27, 2024 at 6:01 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> > >>
> > >> Here is a new version of this patch.  I have been working together with
> > >> Ashutosh on this.  While the version 0 was more of a fragile demo, this
> > >> version 1 has a fairly complete minimal feature set and should be useful
> > >> for playing around with.  We do have a long list of various internal
> > >> bits that still need to be fixed or revised or looked at again, so there
> > >> is by no means a claim that everything is completed.
> > >
> > >
> > > PFA the patchset fixing compilation error reported by CI bot.
> > > 0001 - same as previous one
> > > 0002 - fixes compilation error
> > > 0003 - adds support for WHERE clause in graph pattern missing in the first patch.
> > >
> >
> > There's a test failure reported by CI. Property graph related tests
> > are failing when regression is run from perl tests. The failure is
> > reported only on Free BSD.
>
> I thought it's related to FreeBSD but the bug could be observed
> anywhere with -DRELCACHE_FORCE_RELEASE. It's also reported indirectly
> by valgrind.
>
> When infering properties of an element from the underlying table's
> attributes, the attribute name pointed to the memory in the heap tuple
> of pg_attribute row. Thus when the tuple was released, it pointed to a
> garbage instead of actual column name resulting in column not found
> error.
>
> Attached set of patches with an additional patch to fix the bug.
>
> 0001 - same as previous one
> 0002 - fixes pgperltidy complaints
> 0003 - fixes compilation failure
> 0004 - fixes issue seen on CI
> 0005 - adds support for WHERE clause in graph pattern missing in the
> first patch.
>
> Once reviewed, patches 0002 to 0005 should be merged into 0001.
>
> --
> Best Wishes,
> Ashutosh Bapat

On Mon, Jul 22, 2024 at 5:31 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>

I found that the patches do not support cyclic paths correctly. A
cyclic path pattern is a path patterns where an element pattern
variable repeats e.g. (a)->(b)->(a). In such a path pattern the
element patterns with the same variable indicate the same element in
the path. In the given example (a) specifies that the path should
start and end with the same vertex. Patch 0006 supports cyclic path
patterns.

Elements which share the variable name should have the same element
type. The element patterns sharing the same variable name should have
same label expression. They may be constrained by different conditions
which are finally ANDed since they all represent the same element. The
patch creates a separate abstraction "path_factor" which combines all
the GraphElementPatterns into one element pattern. SQL/PGQ standard
uses path_factor for such an entity, so I chose that as the structure
name. But suggestions are welcome.

A path_factor is further expanded into a list of path_element objects
each representing a vertex or edge table that satisfies the label
expression in GraphElementPattern. In the previous patch set, the
consecutive elements were considered to be connected to each other.
Cyclic paths change that. For example, in path pattern (a)->(b)->(a),
(b) is connected to the first element on both sides (forming a cycle)
instead of first and third element. Patch 0006 has code changes to
appropriately link the elements. As a side effect, I have eliminated
the confusion between variables with name gep and gpe.

While it's easy to imagine a repeated vertex pattern, a repeated edge
pattern is slightly complex. An edge connects only two vertices, and
thus a repeated edge pattern constrains the adjacent vertex patterns
even if they have different variable names. Such patterns are not
supported. E.g. (a)-[b]->(c)-[b]->(d) would mean that (d) and (a)
represent the same vertex even if the variable names are different.
Such patterns are not supported for now. But (a)-[b]->(a)-[b]->(a) OR
(a)-[b]->(c)<-[b]-(a) are supported since the vertices adjacent to
repeated edges are constrained by the variable name anyway.

The patch also changes many foreach() to use foreach_* macros as appropriate.

> 0001 - same as previous one
> 0002 - fixes pgperltidy complaints
> 0003 - fixes compilation failure
> 0004 - fixes issue seen on CI
> 0005 - adds support for WHERE clause in graph pattern missing in the
> first patch.
0006 - adds full support for cyclic path patterns

Once reviewed, patches 0002 to 0006 should be merged into 0001.

--
Best Wishes,
Ashutosh Bapat

Hi Imran,

On Sun, Aug 4, 2024 at 12:32 PM Imran Zaheer <imran.zhir@gmail.com> wrote:
>
> Hi
> I am attaching a new patch for a minor feature addition.
>
> - Adding support for 'Labels and properties: EXCEPT list'

Do you intend to support EXCEPT in the label expression as well or
just properties?

>
> Please let me know if something is missing.

I think the code changes are in the right place. I didn't review the
patch thoroughly. But here are some comments and some advice.

Please do not top-post on hackers.

Always sent the whole patchset. Otherwise, CI bot gets confused. It
doesn't pick up patchset from the previous emails.

About the functionality: It's not clear to me whether an EXCEPT should
be applicable only at the time of property graph creation or it should
be applicable always. I.e. when a property graph is dumped, should it
have EXCEPT in it or have a list of columns surviving except list?
What if a column in except list is dropped after creating a property
graph?

Some comments on the code
1. You could use list_member() in insert_property_records() to check
whether a given column is in the list of exceptions after you have
enveloped in String node.
2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql.
We don't include those in create_property_graph.sql
3. Instead of creating a new property graph in the test, you may
modify one of the existing property graphs to have a label with except
list and then query it.

We are aiming a minimal set of features in the first version. I will
let Peter E. decide whether to consider this as minimal set feature or
not. The feature looks useful to me.

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

Thanks for the feedback.

> Do you intend to support EXCEPT in the label expression as well or
> just properties?
>

I only implemented it for the properties because I couldn't find any
example for Label expression using EXCEPT clause. So I thought it was
only meant to be for the properties.
But if you can confirm that we do use EXCEPT clauses with label
expressions as well then I can try supporting that too.

>
> Please do not top-post on hackers.
>
> Always sent the whole patchset. Otherwise, CI bot gets confused. It
> doesn't pick up patchset from the previous emails.
>
Okay, I will take care of that.

> About the functionality: It's not clear to me whether an EXCEPT should
> be applicable only at the time of property graph creation or it should
> be applicable always. I.e. when a property graph is dumped, should it
> have EXCEPT in it or have a list of columns surviving except list?
> What if a column in except list is dropped after creating a property
> graph?
>

I did some testing on that,  for now we are just dumping the columns
surviving the except list.
If an exceptional table column is deleted afterwards it doesn't show
any effect on the graph. I also tested this scenario with duckdb pgq
extension [1], deleting the col doesn't affect the graph.

> Some comments on the code

I am attaching a new patch after trying to fix according to you comments

> 1. You could use list_member() in insert_property_records() to check
> whether a given column is in the list of exceptions after you have
> enveloped in String node.

* I have changed to code to use list_member(), but I have to make
ResTarget->name from `pstrdup(NameStr(att->attname));` to `NULL`
We are using `xml_attribute_list` for our columns list and while
making this list in gram.y we are assigning `rt->name` as NULL [2],
this causes list_member() func to fail while comparing except_list
nodes. That's why I am changing rt->name from string value to NULL in
propgraphcmds.c in this patch.

* Also, in order to use list_member() func I have to add a separate
for loop to iterate through the exceptional columns to generate the
error message if col is not valid. My question is, is it ok to use two
separate for loops (one to check except cols validity &
other(list_memeber) to check existence of scanned col in except list).
In the previous patch I was using single for loop to validate both
things.

> 2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql.
> We don't include those in create_property_graph.sql

* I have moved the graph_table queries from create_property_graph.sql
to graph_table.sql.
* But in graph_table.sql I didn't use the existing graphs because
those graphs and tables look like there for some specific test
scenario, so I created my separate graph and table for my test
scenario. I didn't drop the graph and the table as we will be dropping
the schema at the end but Peter E has this comment "-- leave for
pg_upgrade/pg_dump tests".

> 3. Instead of creating a new property graph in the test, you may
> modify one of the existing property graphs to have a label with except
> list and then query it.
>

* I have modified the graphs in create_property_graph.sql in order to
test except list cols in the alter command and create graph command.

> We are aiming a minimal set of features in the first version. I will
> let Peter E. decide whether to consider this as minimal set feature or
> not. The feature looks useful to me.

Thanks if you find this patch useful. I am attaching the modified patch.

> 0001 - same as previous one
> 0002 - fixes pgperltidy complaints
> 0003 - fixes compilation failure
> 0004 - fixes issue seen on CI
> 0005 - adds support for WHERE clause in graph pattern missing in the
> first patch.
> 0006 - adds full support for cyclic path patterns

0007 - adds support for except cols list in graph properties

Thanks
Imran Zaheer

[1]: https://github.com/cwida/duckpgq-extension
[2]:
https://github.com/postgres/postgres/blob/f5a1311fccd2ed24a9fb42aa47a17d1df7126039/src/backend/parser/gram.y#L16166

Hello,

With the attached patch found below error when try to use "Any
directed edge" syntax.

postgres=# SELECT * FROM GRAPH_TABLE (students_graph
postgres(#   MATCH
postgres(#   (a IS person )  - [] - (b IS person)
postgres(#   COLUMNS (a.name AS person_a, b.name AS person_b)
postgres(# );
ERROR:  unsupported element pattern kind: undirected edge

If this syntax is supported then should behave as below,

PERSON_A   PERSON_B
---------- ----------
Bob    John
John    Mary
Alice    Mary
Mary    Bob
Mary    John
Bob    Mary
John    Bob
Mary    Alice

8 rows selected.

Attaching the sql file for reference.

Thanks
Ajay

On Sat, Aug 10, 2024 at 2:52 PM Imran Zaheer <imran.zhir@gmail.com> wrote:
>
> Hi Ashutosh,
>
> Thanks for the feedback.
>
> > Do you intend to support EXCEPT in the label expression as well or
> > just properties?
> >
>
> I only implemented it for the properties because I couldn't find any
> example for Label expression using EXCEPT clause. So I thought it was
> only meant to be for the properties.
> But if you can confirm that we do use EXCEPT clauses with label
> expressions as well then I can try supporting that too.
>
> >
> > Please do not top-post on hackers.
> >
> > Always sent the whole patchset. Otherwise, CI bot gets confused. It
> > doesn't pick up patchset from the previous emails.
> >
> Okay, I will take care of that.
>
> > About the functionality: It's not clear to me whether an EXCEPT should
> > be applicable only at the time of property graph creation or it should
> > be applicable always. I.e. when a property graph is dumped, should it
> > have EXCEPT in it or have a list of columns surviving except list?
> > What if a column in except list is dropped after creating a property
> > graph?
> >
>
> I did some testing on that,  for now we are just dumping the columns
> surviving the except list.
> If an exceptional table column is deleted afterwards it doesn't show
> any effect on the graph. I also tested this scenario with duckdb pgq
> extension [1], deleting the col doesn't affect the graph.
>
> > Some comments on the code
>
> I am attaching a new patch after trying to fix according to you comments
>
> > 1. You could use list_member() in insert_property_records() to check
> > whether a given column is in the list of exceptions after you have
> > enveloped in String node.
>
> * I have changed to code to use list_member(), but I have to make
> ResTarget->name from `pstrdup(NameStr(att->attname));` to `NULL`
> We are using `xml_attribute_list` for our columns list and while
> making this list in gram.y we are assigning `rt->name` as NULL [2],
> this causes list_member() func to fail while comparing except_list
> nodes. That's why I am changing rt->name from string value to NULL in
> propgraphcmds.c in this patch.
>
> * Also, in order to use list_member() func I have to add a separate
> for loop to iterate through the exceptional columns to generate the
> error message if col is not valid. My question is, is it ok to use two
> separate for loops (one to check except cols validity &
> other(list_memeber) to check existence of scanned col in except list).
> In the previous patch I was using single for loop to validate both
> things.
>
> > 2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql.
> > We don't include those in create_property_graph.sql
>
> * I have moved the graph_table queries from create_property_graph.sql
> to graph_table.sql.
> * But in graph_table.sql I didn't use the existing graphs because
> those graphs and tables look like there for some specific test
> scenario, so I created my separate graph and table for my test
> scenario. I didn't drop the graph and the table as we will be dropping
> the schema at the end but Peter E has this comment "-- leave for
> pg_upgrade/pg_dump tests".
>
> > 3. Instead of creating a new property graph in the test, you may
> > modify one of the existing property graphs to have a label with except
> > list and then query it.
> >
>
> * I have modified the graphs in create_property_graph.sql in order to
> test except list cols in the alter command and create graph command.
>
> > We are aiming a minimal set of features in the first version. I will
> > let Peter E. decide whether to consider this as minimal set feature or
> > not. The feature looks useful to me.
>
> Thanks if you find this patch useful. I am attaching the modified patch.
>
> > 0001 - same as previous one
> > 0002 - fixes pgperltidy complaints
> > 0003 - fixes compilation failure
> > 0004 - fixes issue seen on CI
> > 0005 - adds support for WHERE clause in graph pattern missing in the
> > first patch.
> > 0006 - adds full support for cyclic path patterns
>
> 0007 - adds support for except cols list in graph properties
>
> Thanks
> Imran Zaheer
>
> [1]: https://github.com/cwida/duckpgq-extension
> [2]:
https://github.com/postgres/postgres/blob/f5a1311fccd2ed24a9fb42aa47a17d1df7126039/src/backend/parser/gram.y#L16166

Hello,

Further testing found that using a property graph with the plpgsql
function crashed the server. Please take a look at the attached SQL
file for reference tables.

postgres=# create or replace function func() returns int as
postgres-# $$
postgres$# declare person_av varchar;
postgres$# begin
postgres$#
postgres$#         SELECT person_a into person_av FROM GRAPH_TABLE
(students_graph
postgres$#           MATCH
postgres$#           (a IS person) -[e IS friends]-> (b IS person
WHERE b.name = 'Bob')
postgres$#           WHERE a.name='John'
postgres$#           COLUMNS (a.name AS person_a, b.name AS person_b)
postgres$#         );
postgres$#
postgres$#         return person_av;
postgres$# end
postgres$# $$ language plpgsql;
CREATE FUNCTION
postgres=# select func();
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>

Please let me know if you need more details.

Thanks
Ajay

On Tue, Aug 13, 2024 at 3:22 PM Ajay Pal <ajay.pal.k@gmail.com> wrote:
>
> Hello,
>
> With the attached patch found below error when try to use "Any
> directed edge" syntax.
>
> postgres=# SELECT * FROM GRAPH_TABLE (students_graph
> postgres(#   MATCH
> postgres(#   (a IS person )  - [] - (b IS person)
> postgres(#   COLUMNS (a.name AS person_a, b.name AS person_b)
> postgres(# );
> ERROR:  unsupported element pattern kind: undirected edge
>
> If this syntax is supported then should behave as below,
>
> PERSON_A   PERSON_B
> ---------- ----------
> Bob    John
> John    Mary
> Alice    Mary
> Mary    Bob
> Mary    John
> Bob    Mary
> John    Bob
> Mary    Alice
>
> 8 rows selected.
>
> Attaching the sql file for reference.
>
> Thanks
> Ajay
>
> On Sat, Aug 10, 2024 at 2:52 PM Imran Zaheer <imran.zhir@gmail.com> wrote:
> >
> > Hi Ashutosh,
> >
> > Thanks for the feedback.
> >
> > > Do you intend to support EXCEPT in the label expression as well or
> > > just properties?
> > >
> >
> > I only implemented it for the properties because I couldn't find any
> > example for Label expression using EXCEPT clause. So I thought it was
> > only meant to be for the properties.
> > But if you can confirm that we do use EXCEPT clauses with label
> > expressions as well then I can try supporting that too.
> >
> > >
> > > Please do not top-post on hackers.
> > >
> > > Always sent the whole patchset. Otherwise, CI bot gets confused. It
> > > doesn't pick up patchset from the previous emails.
> > >
> > Okay, I will take care of that.
> >
> > > About the functionality: It's not clear to me whether an EXCEPT should
> > > be applicable only at the time of property graph creation or it should
> > > be applicable always. I.e. when a property graph is dumped, should it
> > > have EXCEPT in it or have a list of columns surviving except list?
> > > What if a column in except list is dropped after creating a property
> > > graph?
> > >
> >
> > I did some testing on that,  for now we are just dumping the columns
> > surviving the except list.
> > If an exceptional table column is deleted afterwards it doesn't show
> > any effect on the graph. I also tested this scenario with duckdb pgq
> > extension [1], deleting the col doesn't affect the graph.
> >
> > > Some comments on the code
> >
> > I am attaching a new patch after trying to fix according to you comments
> >
> > > 1. You could use list_member() in insert_property_records() to check
> > > whether a given column is in the list of exceptions after you have
> > > enveloped in String node.
> >
> > * I have changed to code to use list_member(), but I have to make
> > ResTarget->name from `pstrdup(NameStr(att->attname));` to `NULL`
> > We are using `xml_attribute_list` for our columns list and while
> > making this list in gram.y we are assigning `rt->name` as NULL [2],
> > this causes list_member() func to fail while comparing except_list
> > nodes. That's why I am changing rt->name from string value to NULL in
> > propgraphcmds.c in this patch.
> >
> > * Also, in order to use list_member() func I have to add a separate
> > for loop to iterate through the exceptional columns to generate the
> > error message if col is not valid. My question is, is it ok to use two
> > separate for loops (one to check except cols validity &
> > other(list_memeber) to check existence of scanned col in except list).
> > In the previous patch I was using single for loop to validate both
> > things.
> >
> > > 2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql.
> > > We don't include those in create_property_graph.sql
> >
> > * I have moved the graph_table queries from create_property_graph.sql
> > to graph_table.sql.
> > * But in graph_table.sql I didn't use the existing graphs because
> > those graphs and tables look like there for some specific test
> > scenario, so I created my separate graph and table for my test
> > scenario. I didn't drop the graph and the table as we will be dropping
> > the schema at the end but Peter E has this comment "-- leave for
> > pg_upgrade/pg_dump tests".
> >
> > > 3. Instead of creating a new property graph in the test, you may
> > > modify one of the existing property graphs to have a label with except
> > > list and then query it.
> > >
> >
> > * I have modified the graphs in create_property_graph.sql in order to
> > test except list cols in the alter command and create graph command.
> >
> > > We are aiming a minimal set of features in the first version. I will
> > > let Peter E. decide whether to consider this as minimal set feature or
> > > not. The feature looks useful to me.
> >
> > Thanks if you find this patch useful. I am attaching the modified patch.
> >
> > > 0001 - same as previous one
> > > 0002 - fixes pgperltidy complaints
> > > 0003 - fixes compilation failure
> > > 0004 - fixes issue seen on CI
> > > 0005 - adds support for WHERE clause in graph pattern missing in the
> > > first patch.
> > > 0006 - adds full support for cyclic path patterns
> >
> > 0007 - adds support for except cols list in graph properties
> >
> > Thanks
> > Imran Zaheer
> >
> > [1]: https://github.com/cwida/duckpgq-extension
> > [2]:
https://github.com/postgres/postgres/blob/f5a1311fccd2ed24a9fb42aa47a17d1df7126039/src/backend/parser/gram.y#L16166

Hi All,

When we use a graph table and any local table, the server crashes.
Please note, It is happening when using the where clause for the local
table only.

postgres=# SELECT * FROM customers a, GRAPH_TABLE (myshop2 MATCH (c IS
customers WHERE c.address = 'US')-[IS customer_orders]->(o IS orders)
COLUMNS (c.name_redacted AS customer_name_redacted));
 customer_id |   name    | address | customer_name_redacted
-------------+-----------+---------+------------------------
           1 | customer1 | US      | redacted1
           2 | customer2 | CA      | redacted1
           3 | customer3 | GL      | redacted1
(3 rows)

postgres=# SELECT * FROM customers a, GRAPH_TABLE (myshop2 MATCH (c IS
customers WHERE c.address = 'US')-[IS customer_orders]->(o IS orders)
COLUMNS (c.name_redacted AS customer_name_redacted)) where
a.customer_id=1;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?> \q

Note:- I have referred to graph_table.sql to get the table structure
used in the above query.

Thanks
Ajay


On Tue, Aug 13, 2024 at 4:08 PM Ajay Pal <ajay.pal.k@gmail.com> wrote:
>
> Hello,
>
> Further testing found that using a property graph with the plpgsql
> function crashed the server. Please take a look at the attached SQL
> file for reference tables.
>
> postgres=# create or replace function func() returns int as
> postgres-# $$
> postgres$# declare person_av varchar;
> postgres$# begin
> postgres$#
> postgres$#         SELECT person_a into person_av FROM GRAPH_TABLE
> (students_graph
> postgres$#           MATCH
> postgres$#           (a IS person) -[e IS friends]-> (b IS person
> WHERE b.name = 'Bob')
> postgres$#           WHERE a.name='John'
> postgres$#           COLUMNS (a.name AS person_a, b.name AS person_b)
> postgres$#         );
> postgres$#
> postgres$#         return person_av;
> postgres$# end
> postgres$# $$ language plpgsql;
> CREATE FUNCTION
> postgres=# select func();
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> The connection to the server was lost. Attempting reset: Failed.
> !?>
>
> Please let me know if you need more details.
>
> Thanks
> Ajay
>
> On Tue, Aug 13, 2024 at 3:22 PM Ajay Pal <ajay.pal.k@gmail.com> wrote:
> >
> > Hello,
> >
> > With the attached patch found below error when try to use "Any
> > directed edge" syntax.
> >
> > postgres=# SELECT * FROM GRAPH_TABLE (students_graph
> > postgres(#   MATCH
> > postgres(#   (a IS person )  - [] - (b IS person)
> > postgres(#   COLUMNS (a.name AS person_a, b.name AS person_b)
> > postgres(# );
> > ERROR:  unsupported element pattern kind: undirected edge
> >
> > If this syntax is supported then should behave as below,
> >
> > PERSON_A   PERSON_B
> > ---------- ----------
> > Bob    John
> > John    Mary
> > Alice    Mary
> > Mary    Bob
> > Mary    John
> > Bob    Mary
> > John    Bob
> > Mary    Alice
> >
> > 8 rows selected.
> >
> > Attaching the sql file for reference.
> >
> > Thanks
> > Ajay
> >
> > On Sat, Aug 10, 2024 at 2:52 PM Imran Zaheer <imran.zhir@gmail.com> wrote:
> > >
> > > Hi Ashutosh,
> > >
> > > Thanks for the feedback.
> > >
> > > > Do you intend to support EXCEPT in the label expression as well or
> > > > just properties?
> > > >
> > >
> > > I only implemented it for the properties because I couldn't find any
> > > example for Label expression using EXCEPT clause. So I thought it was
> > > only meant to be for the properties.
> > > But if you can confirm that we do use EXCEPT clauses with label
> > > expressions as well then I can try supporting that too.
> > >
> > > >
> > > > Please do not top-post on hackers.
> > > >
> > > > Always sent the whole patchset. Otherwise, CI bot gets confused. It
> > > > doesn't pick up patchset from the previous emails.
> > > >
> > > Okay, I will take care of that.
> > >
> > > > About the functionality: It's not clear to me whether an EXCEPT should
> > > > be applicable only at the time of property graph creation or it should
> > > > be applicable always. I.e. when a property graph is dumped, should it
> > > > have EXCEPT in it or have a list of columns surviving except list?
> > > > What if a column in except list is dropped after creating a property
> > > > graph?
> > > >
> > >
> > > I did some testing on that,  for now we are just dumping the columns
> > > surviving the except list.
> > > If an exceptional table column is deleted afterwards it doesn't show
> > > any effect on the graph. I also tested this scenario with duckdb pgq
> > > extension [1], deleting the col doesn't affect the graph.
> > >
> > > > Some comments on the code
> > >
> > > I am attaching a new patch after trying to fix according to you comments
> > >
> > > > 1. You could use list_member() in insert_property_records() to check
> > > > whether a given column is in the list of exceptions after you have
> > > > enveloped in String node.
> > >
> > > * I have changed to code to use list_member(), but I have to make
> > > ResTarget->name from `pstrdup(NameStr(att->attname));` to `NULL`
> > > We are using `xml_attribute_list` for our columns list and while
> > > making this list in gram.y we are assigning `rt->name` as NULL [2],
> > > this causes list_member() func to fail while comparing except_list
> > > nodes. That's why I am changing rt->name from string value to NULL in
> > > propgraphcmds.c in this patch.
> > >
> > > * Also, in order to use list_member() func I have to add a separate
> > > for loop to iterate through the exceptional columns to generate the
> > > error message if col is not valid. My question is, is it ok to use two
> > > separate for loops (one to check except cols validity &
> > > other(list_memeber) to check existence of scanned col in except list).
> > > In the previous patch I was using single for loop to validate both
> > > things.
> > >
> > > > 2. The SELECT with GRAPH_TABLE queries are tested in graph_table.sql.
> > > > We don't include those in create_property_graph.sql
> > >
> > > * I have moved the graph_table queries from create_property_graph.sql
> > > to graph_table.sql.
> > > * But in graph_table.sql I didn't use the existing graphs because
> > > those graphs and tables look like there for some specific test
> > > scenario, so I created my separate graph and table for my test
> > > scenario. I didn't drop the graph and the table as we will be dropping
> > > the schema at the end but Peter E has this comment "-- leave for
> > > pg_upgrade/pg_dump tests".
> > >
> > > > 3. Instead of creating a new property graph in the test, you may
> > > > modify one of the existing property graphs to have a label with except
> > > > list and then query it.
> > > >
> > >
> > > * I have modified the graphs in create_property_graph.sql in order to
> > > test except list cols in the alter command and create graph command.
> > >
> > > > We are aiming a minimal set of features in the first version. I will
> > > > let Peter E. decide whether to consider this as minimal set feature or
> > > > not. The feature looks useful to me.
> > >
> > > Thanks if you find this patch useful. I am attaching the modified patch.
> > >
> > > > 0001 - same as previous one
> > > > 0002 - fixes pgperltidy complaints
> > > > 0003 - fixes compilation failure
> > > > 0004 - fixes issue seen on CI
> > > > 0005 - adds support for WHERE clause in graph pattern missing in the
> > > > first patch.
> > > > 0006 - adds full support for cyclic path patterns
> > >
> > > 0007 - adds support for except cols list in graph properties
> > >
> > > Thanks
> > > Imran Zaheer
> > >
> > > [1]: https://github.com/cwida/duckpgq-extension
> > > [2]:
https://github.com/postgres/postgres/blob/f5a1311fccd2ed24a9fb42aa47a17d1df7126039/src/backend/parser/gram.y#L16166

On Wed, Aug 28, 2024 at 3:48 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.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.


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.


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");



3.
The noise word "ARE" is missing from the <element table properties alternatives> clause.
There is also no support for the EXCEPT clause, but I imagine that can be added at a later time.


4.
I notice that tables in pg_catalog are not allowed in a property graph.  What are the reasons for this?  It is true that this might cause some problems with upgrades if a column is removed, but it shouldn't cause trouble for columns being added.  That case works with user tables.

5.

The ascii art characters (I am loathe to call them operators) allow junk in between them.  For example:

    MATCH (c) -[:lbl]-> (d)

can be written as

    MATCH (c) -
        [:lbl] -
        /* a comment here */
        > (d)

Is that intentional?

----

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.

--

Vik Fearing

On Fri, Nov 22, 2024 at 7:29 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Nov 19, 2024 at 10:08 PM Vik Fearing <vik@postgresfriends.org> wrote:
> >
> >
> > On 05/11/2024 16:41, Ashutosh Bapat wrote:
> >
> > On Wed, Aug 28, 2024 at 3:48 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.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
seeingfor the most part.  I have not been looking much at the code itself, although I do plan on reviewing some of the
codein 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
toget 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

Hi Ashutosh,

On Fri, Dec 6, 2024 at 12:34 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Sorry, I forgot to attach patches. PFA the patches.
>
> On Thu, Dec 5, 2024 at 4:38 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Nov 22, 2024 at 7:29 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Tue, Nov 19, 2024 at 10:08 PM Vik Fearing <vik@postgresfriends.org> wrote:
> > > >
> > > >
> > > > On 05/11/2024 16:41, Ashutosh Bapat wrote:
> > > >
> > > > On Wed, Aug 28, 2024 at 3:48 PM Ashutosh Bapat
> > > > <ashutosh.bapat.oss@gmail.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
Iam seeing for the most part.  I have not been looking much at the code itself, although I do plan on reviewing some of
thecode 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
excitedto 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

I'm looking at the catalog definition, I have some questions which
might be silly.

Each pg element can have multiple labels(whose properties belong
to the same pg element), can we have multiple elements share the
same label?

If we have a 1..* relation between element and label, I think maybe
we don't need _pg_propgraph_label_.

From the name _pg_propgraph_label_property_, I tend to think it is
referencing _pg_propgraph_label_, but actually it is referencing
_pg_propgraph_element_label_.

--
Regards
Junwang Zhao

Hi Junwang,

>
> I'm looking at the catalog definition, I have some questions which
> might be silly.

Thanks for your interest in SQL/PGQ.

>
> Each pg element can have multiple labels(whose properties belong
> to the same pg element), can we have multiple elements share the
> same label?

Yes. A label can be shared between edges and vertices as well.

>
> If we have a 1..* relation between element and label, I think maybe
> we don't need _pg_propgraph_label_.

pg_propgraph_label is used to map a label name in a given property
graph to its OID. pg_propgraph_element_label - can be used to find all
the elements with a given label and all the labels of a given element.
The relationship is many to many.

>
> From the name _pg_propgraph_label_property_, I tend to think it is
> referencing _pg_propgraph_label_, but actually it is referencing
> _pg_propgraph_element_label_.
>

Right.

-- 
Best Wishes,
Ashutosh Bapat

On Mon, Dec 16, 2024 at 6:14 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Junwang,
>
> >
> > I'm looking at the catalog definition, I have some questions which
> > might be silly.
>
> Thanks for your interest in SQL/PGQ.
>
> >
> > Each pg element can have multiple labels(whose properties belong
> > to the same pg element), can we have multiple elements share the
> > same label?
>
> Yes. A label can be shared between edges and vertices as well.
>
> >
> > If we have a 1..* relation between element and label, I think maybe
> > we don't need _pg_propgraph_label_.
>
> pg_propgraph_label is used to map a label name in a given property
> graph to its OID. pg_propgraph_element_label - can be used to find all
> the elements with a given label and all the labels of a given element.
> The relationship is many to many.

Ok, then it makes sense to me. Thanks for the explanation.

>
> >
> > From the name _pg_propgraph_label_property_, I tend to think it is
> > referencing _pg_propgraph_label_, but actually it is referencing
> > _pg_propgraph_element_label_.
> >
>
> Right.
>
> --
> Best Wishes,
> Ashutosh Bapat



--
Regards
Junwang Zhao

Hi Ashutosh,

On Wed, Jan 1, 2025 at 5:02 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Jan 1, 2025 at 2:22 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Sat, Dec 21, 2024 at 6:21 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > Thanks Junwang for your review.
> >
> > > 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.
> >
> > Nice catch. I fixed it in a slightly different way reducing overall
> > 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 this.
> >
> > From a cursory glance those changes look useful and mostly correct. It
> > will be good if you can provide a SQL test for those, covering that
> > part of the code. Please post the whole patch-set with your changes as
> > a separate commit/patch.
> >
> > >
> > > 3. Since property name and label name are unique in property graph
> > > scope, some of the wording are not accurate. See attached 0003.
> >
> >      <para>
> > -      For each property graph element, all properties with the same name must
> > -      have the same expression for each label.  For example, this 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 name
> > associated with multiple labels. But each of those properties should
> > have the same expression. You may see that as the same property being
> > defined multiple times once in each label it is associated with OR
> > 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 moderators
> release it. In the meantime trying to send the patches as a zip file
> 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 parse tree

P.S. It seems the patch set doesn't apply to master.

--
Regards
Junwang Zhao

On Fri, Jan 24, 2025 at 11:46 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Fri, Jan 24, 2025 at 9:31 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Sun, Jan 19, 2025 at 6:45 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > >
> > > Hi Ashutosh,
> > >
> > > On Wed, Jan 1, 2025 at 5:02 PM Ashutosh Bapat
> > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 1, 2025 at 2:22 PM Ashutosh Bapat
> > > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > > >
> > > > > On Sat, Dec 21, 2024 at 6:21 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > > > Thanks Junwang for your review.
> > > > >
> > > > > > 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.
> > > > >
> > > > > Nice catch. I fixed it in a slightly different way reducing overall
> > > > > 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 this.
> > > > >
> > > > > From a cursory glance those changes look useful and mostly correct. It
> > > > > will be good if you can provide a SQL test for those, covering that
> > > > > part of the code. Please post the whole patch-set with your changes as
> > > > > a separate commit/patch.
> > > > >
> > > > > >
> > > > > > 3. Since property name and label name are unique in property graph
> > > > > > scope, some of the wording are not accurate. See attached 0003.
> > > > >
> > > > >      <para>
> > > > > -      For each property graph element, all properties with the same name must
> > > > > -      have the same expression for each label.  For example, this 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 name
> > > > > associated with multiple labels. But each of those properties should
> > > > > have the same expression. You may see that as the same property being
> > > > > defined multiple times once in each label it is associated with OR
> > > > > 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 moderators
> > > > release it. In the meantime trying to send the patches as a zip file
> > > > 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 parse 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 option,
> 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 fixes.
> > 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 refactor
>
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Regards
> Junwang Zhao



--
Regards
Junwang Zhao

On Tue, Jan 28, 2025 at 6:17 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Jan 24, 2025 at 9:16 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > I figured out it's because I have `-DWRITE_READ_PARSE_PLAN_TREES` this option,
> > removing this option clears the warning, but I'm not sure if this is a
> > hidden issue.
> >
>
> Thanks for the hint. I found the issue and the fix appears in 0004.
> Some members of RangeTblEntry were not being handled in read/out
> functions.

Thanks for the fix, I will apply and test again.

>
> > >
> > > 0001-0009 patches are the same as the previous set sans mergeconflict fixes.
> > > 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
>
> The standard seems to require a property reference to be qualified by
> element pattern variable, even if the property is referenced within
> the same element pattern bound to the variable. Hence I accepted your
> document fixes which qualify property reference with element pattern
> variable.
>
> However, I didn't understand why you changed a LABEL name from order to order_.

Ah, that's because `order` is a keyword of SQL(order by), so the alias
is a conflict.
I'm not saying `order_` is a good name though.

>
> > - after path factor was introduced, some comments doesn't apply
>
> Thanks pointing those out. Accepted after rephrasing those and also
> correcting some related comments.
>
> >
> > 0002:
> > - add a get_propgraph_element_alias_name function and do some trivial refactor
> >
>
> I see you have added some negative tests - object not found tests, but
> I didn't see positive tests. Hence I haven't added those changes in
> the attached patchset. But we certainly need those changes. You may
> want to submit a patch with positive tests. That code needs to be
> fixed before committing anyway.

Ok, I'll add positive tests.

>
> --
> Best Wishes,
> Ashutosh Bapat



--
Regards
Junwang Zhao

On Tue, Jan 28, 2025 at 4:31 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Tue, Jan 28, 2025 at 6:17 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Jan 24, 2025 at 9:16 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > >
> > > I figured out it's because I have `-DWRITE_READ_PARSE_PLAN_TREES` this option,
> > > removing this option clears the warning, but I'm not sure if this is a
> > > hidden issue.
> > >
> >
> > Thanks for the hint. I found the issue and the fix appears in 0004.
> > Some members of RangeTblEntry were not being handled in read/out
> > functions.
>
> Thanks for the fix, I will apply and test again.
>
> >
> > > >
> > > > 0001-0009 patches are the same as the previous set sans mergeconflict fixes.
> > > > 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
> >
> > The standard seems to require a property reference to be qualified by
> > element pattern variable, even if the property is referenced within
> > the same element pattern bound to the variable. Hence I accepted your
> > document fixes which qualify property reference with element pattern
> > variable.
> >
> > However, I didn't understand why you changed a LABEL name from order to order_.
>
> Ah, that's because `order` is a keyword of SQL(order by), so the alias
> is a conflict.
> I'm not saying `order_` is a good name though.

I didn't realize that. Thanks for catching. In that case just orders
or cust_orders?

>
> >
> > > - after path factor was introduced, some comments doesn't apply
> >
> > Thanks pointing those out. Accepted after rephrasing those and also
> > correcting some related comments.
> >
> > >
> > > 0002:
> > > - add a get_propgraph_element_alias_name function and do some trivial refactor
> > >
> >
> > I see you have added some negative tests - object not found tests, but
> > I didn't see positive tests. Hence I haven't added those changes in
> > the attached patchset. But we certainly need those changes. You may
> > want to submit a patch with positive tests. That code needs to be
> > fixed before committing anyway.
>
> Ok, I'll add positive tests.

Thanks.

--
Best Wishes,
Ashutosh Bapat

On Tue, Jan 28, 2025 at 7:03 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Jan 28, 2025 at 4:31 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > On Tue, Jan 28, 2025 at 6:17 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 9:16 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > >
> > > > I figured out it's because I have `-DWRITE_READ_PARSE_PLAN_TREES` this option,
> > > > removing this option clears the warning, but I'm not sure if this is a
> > > > hidden issue.
> > > >
> > >
> > > Thanks for the hint. I found the issue and the fix appears in 0004.
> > > Some members of RangeTblEntry were not being handled in read/out
> > > functions.
> >
> > Thanks for the fix, I will apply and test again.
> >
> > >
> > > > >
> > > > > 0001-0009 patches are the same as the previous set sans mergeconflict fixes.
> > > > > 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
> > >
> > > The standard seems to require a property reference to be qualified by
> > > element pattern variable, even if the property is referenced within
> > > the same element pattern bound to the variable. Hence I accepted your
> > > document fixes which qualify property reference with element pattern
> > > variable.
> > >
> > > However, I didn't understand why you changed a LABEL name from order to order_.
> >
> > Ah, that's because `order` is a keyword of SQL(order by), so the alias
> > is a conflict.
> > I'm not saying `order_` is a good name though.
>
> I didn't realize that. Thanks for catching. In that case just orders
> or cust_orders?

Yeah, I think explicitly LABELed as `orders` makes sense.

>
> >
> > >
> > > > - after path factor was introduced, some comments doesn't apply
> > >
> > > Thanks pointing those out. Accepted after rephrasing those and also
> > > correcting some related comments.
> > >
> > > >
> > > > 0002:
> > > > - add a get_propgraph_element_alias_name function and do some trivial refactor
> > > >
> > >
> > > I see you have added some negative tests - object not found tests, but
> > > I didn't see positive tests. Hence I haven't added those changes in
> > > the attached patchset. But we certainly need those changes. You may
> > > want to submit a patch with positive tests. That code needs to be
> > > fixed before committing anyway.
> >
> > Ok, I'll add positive tests.
>
> Thanks.
>
> --
> Best Wishes,
> Ashutosh Bapat



--
Regards
Junwang Zhao

On Thu, Feb 6, 2025 at 8:22 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > >
> >
> > I see you have added some negative tests - object not found tests, but
> > I didn't see positive tests. Hence I haven't added those changes in
> > the attached patchset. But we certainly need those changes. You may
> > want to submit a patch with positive tests. That code needs to be
> > fixed before committing anyway.
>
> The attached file adds the positive tests.

Hi Junwang,
Thanks for the patch, but please post it as a separate patch in the
full patch-set, otherwise CFBot gets confused.

--
Best Wishes,
Ashutosh Bapat

On Mon, Feb 10, 2025 at 11:00 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Ashutosh,
>
> On Mon, Feb 10, 2025 at 2:14 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Thu, Feb 6, 2025 at 8:22 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > > >
> > > >
> > > > I see you have added some negative tests - object not found tests, but
> > > > I didn't see positive tests. Hence I haven't added those changes in
> > > > the attached patchset. But we certainly need those changes. You may
> > > > want to submit a patch with positive tests. That code needs to be
> > > > fixed before committing anyway.
> > >
> > > The attached file adds the positive tests.
> >
> > Hi Junwang,
> > Thanks for the patch, but please post it as a separate patch in the
> > full patch-set, otherwise CFBot gets confused.
>
> Ok, see the attached.
>
> 0001-0009 are the original patches of 20250127 with rebase of master
> 0010 fix ci error due to `Show more-intuitive titles for psql
> commands`, see a14707da564e
> 0011 fix ci error due to unstable collation tests, we should not
> depend on pg_catalog."default", I observed 002_pg_upgrade.pl failure
> caused by this.
> 0012 trivial refactor of property graph object address
>
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Regards
> Junwang Zhao

Here is v11 with another trivial refactor, I add a seperate patch file 0013, in
insert_property_record, there is no need to check `type`, `typmode` and
`collation` if the property doesn't exists before, in AlterPropGraph, there is
no need to call `check_all_labels_properties` for each added vertex or edge.

Other files remain unchanged except I’ve added some missing document and
typo fix we discussed in the list but not included in the previous
patch, I included
them in 0008.

--
Regards
Junwang Zhao

Hi!

  I would ask about CREATE GRAPH PROPERTY. From my point of view it makes 
very strong
check of types including check of typmod.

  Example:

CREATE TABLE userslist (
   user_id INT primary key,
   name   VARCHAR(40)
);

CREATE TABLE groupslist (
   group_id INT primary key,
   name   VARCHAR(30)
);

CREATE TABLE memberslist (
  member_id INT primary key,
  user_id INT,
  group_id INT,
  CONSTRAINT members_users_fk1 FOREIGN KEY (user_id) REFERENCES userslist 
(user_id),
  CONSTRAINT members_groups_fk1 FOREIGN KEY (group_id) REFERENCES 
groupslist (group_id)
);

CREATE PROPERTY GRAPH members_pg
   VERTEX TABLES (
     userslist
       KEY (user_id)
       LABEL luser
       PROPERTIES ALL COLUMNS,
     groupslist
       KEY (group_id)
       LABEL lgroup
       PROPERTIES ALL COLUMNS
  )
   edge TABLES (
     memberslist
       KEY (member_id)
       SOURCE KEY (user_id) REFERENCES userslist (user_id)
       DESTINATION KEY (group_id) REFERENCES groupslist (group_id)
       LABEL lmember
       PROPERTIES ALL COLUMNS
  );

Last command fails with error:
ERROR:  property "name" data type modifier mismatch: 19 vs. 14
DETAIL:  In a property graph, a property of the same name has to have
the same type modifier in each label.

Labels luser and lgroup both have property "name" originated from tables 
userslist and groupslist
with types VARCHAR(40) and VARCHAR(30). Current implementation treats 
them as fields with
different types. I think, they should not be treated as different types. 
Typmod is additional
constrain, it does not create another type.
Documentation includes:"modifiers, that is optional constraints attached 
to a type declaration,
such as char(5) or numeric(30,2)"

Operations with values using different typmod do not require cast, they  
treated as the
same type.

Also I would add, that Oracle executes the code above without error.

I propose to exclude the check of typmod from the equivalence of types 
check in
src/backend/commands/propgraphcmds.c .

I suppose there is other reason to make this check (not the Standart 
SQL-2023 requirement,
but internal dependency), but I have not realised this reason.

-- 
Best regards,

Vladlen Popolitov.

On Tue, Feb 25, 2025 at 3:17 PM Vladlen Popolitov
<v.popolitov@postgrespro.ru> wrote:
>
> Hi!
>
>   I would ask about CREATE GRAPH PROPERTY. From my point of view it makes
> very strong
> check of types including check of typmod.
>
>   Example:
>
> CREATE TABLE userslist (
>    user_id INT primary key,
>    name   VARCHAR(40)
> );
>
> CREATE TABLE groupslist (
>    group_id INT primary key,
>    name   VARCHAR(30)
> );
>
> CREATE TABLE memberslist (
>   member_id INT primary key,
>   user_id INT,
>   group_id INT,
>   CONSTRAINT members_users_fk1 FOREIGN KEY (user_id) REFERENCES userslist
> (user_id),
>   CONSTRAINT members_groups_fk1 FOREIGN KEY (group_id) REFERENCES
> groupslist (group_id)
> );
>
> CREATE PROPERTY GRAPH members_pg
>    VERTEX TABLES (
>      userslist
>        KEY (user_id)
>        LABEL luser
>        PROPERTIES ALL COLUMNS,
>      groupslist
>        KEY (group_id)
>        LABEL lgroup
>        PROPERTIES ALL COLUMNS
>   )
>    edge TABLES (
>      memberslist
>        KEY (member_id)
>        SOURCE KEY (user_id) REFERENCES userslist (user_id)
>        DESTINATION KEY (group_id) REFERENCES groupslist (group_id)
>        LABEL lmember
>        PROPERTIES ALL COLUMNS
>   );
>
> Last command fails with error:
> ERROR:  property "name" data type modifier mismatch: 19 vs. 14
> DETAIL:  In a property graph, a property of the same name has to have
> the same type modifier in each label.
>
> Labels luser and lgroup both have property "name" originated from tables
> userslist and groupslist
> with types VARCHAR(40) and VARCHAR(30). Current implementation treats
> them as fields with
> different types. I think, they should not be treated as different types.
> Typmod is additional
> constrain, it does not create another type.
> Documentation includes:"modifiers, that is optional constraints attached
> to a type declaration,
> such as char(5) or numeric(30,2)"
>
> Operations with values using different typmod do not require cast, they
> treated as the
> same type.

When a property is projected in a COLUMNS clause, it will be added to
the SELECT list of the rewritten query, which in turn could be a UNION
query. Having the same typmod for all the properties with the same
name makes it easy to write the SELECT list of UNION. Otherwise, we
have to find a common typmod for all the properties. This restriction
is not expected to be permanent but makes the first set of patches
easy, which are quite large as is. I think in some next version, we
will lift this restriction as well as lift the restriction on all
properties with the same name needing the same datatype. According to
the SQL standard, they can be of different data types as long as
there's a common type to which they all can be cast to. It may be
possible to lift this restriction in this version itself, if we find
enough developer and reviewer bandwidth.


--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat писал(а) 2025-02-25 17:20:
> On Tue, Feb 25, 2025 at 3:17 PM Vladlen Popolitov
> <v.popolitov@postgrespro.ru> wrote:
>> 
>> Hi!
>> 
>>   I would ask about CREATE GRAPH PROPERTY. From my point of view it 
>> makes
>> very strong
>> check of types including check of typmod.
>> 
>>   Example:
>> 
>> CREATE TABLE userslist (
>>    user_id INT primary key,
>>    name   VARCHAR(40)
>> );
>> 
>> CREATE TABLE groupslist (
>>    group_id INT primary key,
>>    name   VARCHAR(30)
>> );
>> 
>> CREATE TABLE memberslist (
>>   member_id INT primary key,
>>   user_id INT,
>>   group_id INT,
>>   CONSTRAINT members_users_fk1 FOREIGN KEY (user_id) REFERENCES 
>> userslist
>> (user_id),
>>   CONSTRAINT members_groups_fk1 FOREIGN KEY (group_id) REFERENCES
>> groupslist (group_id)
>> );
>> 
>> CREATE PROPERTY GRAPH members_pg
>>    VERTEX TABLES (
>>      userslist
>>        KEY (user_id)
>>        LABEL luser
>>        PROPERTIES ALL COLUMNS,
>>      groupslist
>>        KEY (group_id)
>>        LABEL lgroup
>>        PROPERTIES ALL COLUMNS
>>   )
>>    edge TABLES (
>>      memberslist
>>        KEY (member_id)
>>        SOURCE KEY (user_id) REFERENCES userslist (user_id)
>>        DESTINATION KEY (group_id) REFERENCES groupslist (group_id)
>>        LABEL lmember
>>        PROPERTIES ALL COLUMNS
>>   );
>> 
>> Last command fails with error:
>> ERROR:  property "name" data type modifier mismatch: 19 vs. 14
>> DETAIL:  In a property graph, a property of the same name has to have
>> the same type modifier in each label.
>> 
>> Labels luser and lgroup both have property "name" originated from 
>> tables
>> userslist and groupslist
>> with types VARCHAR(40) and VARCHAR(30). Current implementation treats
>> them as fields with
>> different types. I think, they should not be treated as different 
>> types.
>> Typmod is additional
>> constrain, it does not create another type.
>> Documentation includes:"modifiers, that is optional constraints 
>> attached
>> to a type declaration,
>> such as char(5) or numeric(30,2)"
>> 
>> Operations with values using different typmod do not require cast, 
>> they
>> treated as the
>> same type.
> 
> When a property is projected in a COLUMNS clause, it will be added to
> the SELECT list of the rewritten query, which in turn could be a UNION
> query. Having the same typmod for all the properties with the same
> name makes it easy to write the SELECT list of UNION. Otherwise, we
> have to find a common typmod for all the properties. This restriction
> is not expected to be permanent but makes the first set of patches
> easy, which are quite large as is. I think in some next version, we
> will lift this restriction as well as lift the restriction on all
> properties with the same name needing the same datatype. According to
> the SQL standard, they can be of different data types as long as
> there's a common type to which they all can be cast to. It may be
> possible to lift this restriction in this version itself, if we find
> enough developer and reviewer bandwidth.
Hi!

  Thank you for explanation. I expected, that it has some internal
dependency, but did not find it.
I am running graph queries now, and I will see how they interact with
typmod.

-- 
Best regards,

Vladlen Popolitov.

On Sun, Feb 23, 2025 at 8:54 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Mon, Feb 10, 2025 at 11:00 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > Hi Ashutosh,
> >
> > On Mon, Feb 10, 2025 at 2:14 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Thu, Feb 6, 2025 at 8:22 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > > > >
> > > > >
> > > > > I see you have added some negative tests - object not found tests, but
> > > > > I didn't see positive tests. Hence I haven't added those changes in
> > > > > the attached patchset. But we certainly need those changes. You may
> > > > > want to submit a patch with positive tests. That code needs to be
> > > > > fixed before committing anyway.
> > > >
> > > > The attached file adds the positive tests.
> > >
> > > Hi Junwang,
> > > Thanks for the patch, but please post it as a separate patch in the
> > > full patch-set, otherwise CFBot gets confused.
> >
> > Ok, see the attached.
> >
> > 0001-0009 are the original patches of 20250127 with rebase of master
> > 0010 fix ci error due to `Show more-intuitive titles for psql
> > commands`, see a14707da564e
> > 0011 fix ci error due to unstable collation tests, we should not
> > depend on pg_catalog."default", I observed 002_pg_upgrade.pl failure
> > caused by this.
> > 0012 trivial refactor of property graph object address
> >
> > >
> > > --
> > > Best Wishes,
> > > Ashutosh Bapat
> >
> >
> >
> > --
> > Regards
> > Junwang Zhao
>
> Here is v11 with another trivial refactor, I add a seperate patch file 0013, in
> insert_property_record, there is no need to check `type`, `typmode` and
> `collation` if the property doesn't exists before, in AlterPropGraph, there is
> no need to call `check_all_labels_properties` for each added vertex or edge.
>
> Other files remain unchanged except I’ve added some missing document and
> typo fix we discussed in the list but not included in the previous
> patch, I included
> them in 0008.
>
> --
> Regards
> Junwang Zhao

Current patch set failed the cfbot, I did some analysis on this, the reason for
the failure is that backend has a core dump, I extract some queries from
graph_table.sql to reproduce the issue:

----------------- queries begin -----------------
CREATE SCHEMA graph_table_tests;
GRANT USAGE ON SCHEMA graph_table_tests TO PUBLIC;
SET search_path = graph_table_tests;

CREATE TABLE products (
    product_no integer PRIMARY KEY,
    name varchar,
    price numeric
);

CREATE TABLE customers (
    customer_id integer PRIMARY KEY,
    name varchar,
    address varchar
);

CREATE TABLE orders (
    order_id integer PRIMARY KEY,
    ordered_when date
);

CREATE TABLE order_items (
    order_items_id integer PRIMARY KEY,
    order_id integer REFERENCES orders (order_id),
    product_no integer REFERENCES products (product_no),
    quantity integer
);

CREATE TABLE customer_orders (
    customer_orders_id integer PRIMARY KEY,
    customer_id integer REFERENCES customers (customer_id),
    order_id integer REFERENCES orders (order_id)
);


CREATE VIEW customers_view AS SELECT customer_id, 'redacted' ||
customer_id AS name_redacted, address FROM customers;

CREATE PROPERTY GRAPH myshop2
    VERTEX TABLES (
        products,
        customers_view KEY (customer_id) LABEL customers,
        orders
    )
    EDGE TABLES (
        order_items KEY (order_items_id)
            SOURCE KEY (order_id) REFERENCES orders (order_id)
            DESTINATION KEY (product_no) REFERENCES products (product_no),
        customer_orders KEY (customer_orders_id)
            SOURCE KEY (customer_id) REFERENCES customers_view (customer_id)
            DESTINATION KEY (order_id) REFERENCES orders (order_id)
    );

CREATE VIEW customers_us_redacted AS SELECT * FROM GRAPH_TABLE
(myshop2 MATCH (c IS customers WHERE c.address = 'US')-[IS
customer_orders]->(o IS orders) COLUMNS (c.name_redacted AS
customer_name_redacted));

SELECT * FROM customers_us_redacted;         <----- abort on this query
----------------- queries end -----------------

The backend aborted in ExecCheckPermissions, a recent commit 525392d57
add the following check:

----------------- code begin -----------------
/*
* Ensure that we have at least an AccessShareLock on relations
* whose permissions need to be checked.
*
* Skip this check in a parallel worker because locks won't be
* taken until ExecInitNode() performs plan initialization.
*
* XXX: ExecCheckPermissions() in a parallel worker may be
* redundant with the checks done in the leader process, so this
* should be reviewed to ensure it’s necessary.
*/
Assert(IsParallelWorker() ||
CheckRelationOidLockedByMe(rte->relid, AccessShareLock,
true));
----------------- code end -----------------

The query causing the core dump doesn't call transformRangeGraphTable
because the graph table is not in the `from` lists, so the graph table
is not locked.

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.




--
Regards
Junwang Zhao

Hi Amit and Ashutosh,

On Tue, Mar 11, 2025 at 8:19 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> 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

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

Hi Junwang,

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.

Looks like the first patch has grown some conflicts. Here's set of
rebased patches. There are two main things missing from this patchset

1. Junwang's patches - Extremely sorry, didn't get time to include
your patches. Junwang, you may want to post whole patchset rebased on
attached patch set. I hope to review and incorporate your patches some
time soon.

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.



--
Best Wishes,
Ashutosh Bapat

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

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

Thanks

On Tue, Mar 11, 2025 at 7:51 PM Junwang Zhao <zhjwpku@gmail.com> wrote:

>
> 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

I think that fix is correct and it fixes the crash. I need to think
more about it - especially whether there are more places where we are
missing RTE_GRAPH_TABLE. But will do that as time permits.

> 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
>

Thanks Junwang. I have added your patches to my local repository. Next
time I post an updated set, I will post it along with your patches.
Will merge them into the original patches as time permits.

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh and Peter,

On Wed, Mar 12, 2025 at 12:31 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Thanks
>
> On Tue, Mar 11, 2025 at 7:51 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> >
> > 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
>
> I think that fix is correct and it fixes the crash. I need to think
> more about it - especially whether there are more places where we are
> missing RTE_GRAPH_TABLE. But will do that as time permits.
>
> > 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
> >
>
> Thanks Junwang. I have added your patches to my local repository. Next
> time I post an updated set, I will post it along with your patches.
> Will merge them into the original patches as time permits.
>
> --
> Best Wishes,
> Ashutosh Bapat

Since this PGQ feature won't be in PG 18, I'd like to raise a discussion of
the possibility of implementing the quantifier feature, which I think is a
quite useful feature in the graph database area.

I'll start with a graph definition first.

`Person(id, name, age, sex)` with id as PK
`Knows(id, start_id, end_id, since)` with id as PK, start_id and
end_id FK referencing Person's id

insert into Person values(1, 'A', 31, 'M'), (2, 'B', 30, 'F'), (3,
'C', 33, 'M'), (4, 'D', 31, 'F'), (5, 'E', 32, 'M'), (6, 'F', 33,
'M');
insert into Knows values (1, 1, 2, '2020');  -- A knows B since 2020
insert into Knows values (2, 1, 3, '2021');  -- A knows C since 2021
insert into Knows values (3, 1, 4, '2020');  -- A knows D since 2020
insert into Knows values (4, 2, 4, '2023');  -- B knows D since 2023
insert into Knows values (5, 3, 5, '2022');  -- C knows E since 2022
insert into Knows values (6, 2, 6, '2021');  -- B knows F since 2021
insert into Knows values (7, 4, 6, '2020');  -- D knows F since 2020

Then we create a property graph:

CREATE property graph new_graph
VERTEX TABLES (Person)
EDGE TABLES (Knows);

If we want to find A's non-directly known friends within 3 hops, we can query:

select name from graph_table (new_graph match (a:Person WHERE a.name =
'A') --> (b:Person) --> (c:Person) COLUMNS (c.name))
union
select name from graph_table (new_graph match (a:Person WHERE a.name =
'A') --> (b:Person) -->(c:Person)-->(d:Person) COLUMNS (d.name));

Or if we support quantifier, we can simply the query as:

select name from graph_table (new_graph match (a:Person WHERE a.name =
'A') -->{2,3} (b:Person) COLUMNS (b.name));

In the current design of PostgreSQL, we can rewrite this pattern with
quantifiers to
the union form with some effort.

But what if the pattern is more complicated, for example:

1. select name, since from graph_table (new_graph match (a:Person
WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
r.since));
Can we support the r.since column? I guess not, in this case r is a
variable length edge.

2. select name, count from graph_table (new_graph match (a:Person
WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
count(r)));
Can we support this count aggregation(this is called horizontal
aggregation in Oracle's pgql)? How can the executor know the length of
the variable length edge?

3. What if the query doesn't specify the Label of edge, and there can
be different edge labels of r, can we easily do the rewrite?

I did some study of the apache age, they have fixed columns for node
labels(id, agtype)
and edge labels(id, source_id, end_id, agtype), agtype is kind of
json. So they can
resolve the above question easily.

Above are just my random thoughts of the quantifier feature, I don't have a copy
of the PGQ standard, so I'd like to hear your opinion about this.

[1] https://pgql-lang.org/spec/1.2/#horizontal-aggregation-using-group-variables

--
Regards
Junwang Zhao

On Sat, Apr 5, 2025 at 6:20 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Ashutosh and Peter,
>
> Since this PGQ feature won't be in PG 18, I'd like to raise a discussion of
> the possibility of implementing the quantifier feature, which I think is a
> quite useful feature in the graph database area.

I agree that quantifiers feature is very useful; it's being used in
many usecases. However, it's a bit of a complex feature. IMO, we
should keep that discussion as well as the patch in a separate thread,
so that this patchset doesn't grow too large to review and also
discussion in this thread can remain focused. Once we get the current
patch set reviewed and committed we can tackle the quantifier problem
in a separate discussion. Of course that doesn't mean that we can not
start discussion, try POC and even a working patch for quantifier
support.

Peter may think otherwise.

>
> I'll start with a graph definition first.
>
> `Person(id, name, age, sex)` with id as PK
> `Knows(id, start_id, end_id, since)` with id as PK, start_id and
> end_id FK referencing Person's id
>
> insert into Person values(1, 'A', 31, 'M'), (2, 'B', 30, 'F'), (3,
> 'C', 33, 'M'), (4, 'D', 31, 'F'), (5, 'E', 32, 'M'), (6, 'F', 33,
> 'M');
> insert into Knows values (1, 1, 2, '2020');  -- A knows B since 2020
> insert into Knows values (2, 1, 3, '2021');  -- A knows C since 2021
> insert into Knows values (3, 1, 4, '2020');  -- A knows D since 2020
> insert into Knows values (4, 2, 4, '2023');  -- B knows D since 2023
> insert into Knows values (5, 3, 5, '2022');  -- C knows E since 2022
> insert into Knows values (6, 2, 6, '2021');  -- B knows F since 2021
> insert into Knows values (7, 4, 6, '2020');  -- D knows F since 2020
>
> Then we create a property graph:
>
> CREATE property graph new_graph
> VERTEX TABLES (Person)
> EDGE TABLES (Knows);
>
> If we want to find A's non-directly known friends within 3 hops, we can query:
>
> select name from graph_table (new_graph match (a:Person WHERE a.name =
> 'A') --> (b:Person) --> (c:Person) COLUMNS (c.name))
> union
> select name from graph_table (new_graph match (a:Person WHERE a.name =
> 'A') --> (b:Person) -->(c:Person)-->(d:Person) COLUMNS (d.name));
>
> Or if we support quantifier, we can simply the query as:
>
> select name from graph_table (new_graph match (a:Person WHERE a.name =
> 'A') -->{2,3} (b:Person) COLUMNS (b.name));
>
> In the current design of PostgreSQL, we can rewrite this pattern with
> quantifiers to
> the union form with some effort.
>
> But what if the pattern is more complicated, for example:
>
> 1. select name, since from graph_table (new_graph match (a:Person
> WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
> r.since));
> Can we support the r.since column? I guess not, in this case r is a
> variable length edge.
>
> 2. select name, count from graph_table (new_graph match (a:Person
> WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
> count(r)));
> Can we support this count aggregation(this is called horizontal
> aggregation in Oracle's pgql)? How can the executor know the length of
> the variable length edge?
>
> 3. What if the query doesn't specify the Label of edge, and there can
> be different edge labels of r, can we easily do the rewrite?
>
> I did some study of the apache age, they have fixed columns for node
> labels(id, agtype)
> and edge labels(id, source_id, end_id, agtype), agtype is kind of
> json. So they can
> resolve the above question easily.
>
> Above are just my random thoughts of the quantifier feature, I don't have a copy
> of the PGQ standard, so I'd like to hear your opinion about this.
>

I think the questions you have raised are valid. If we decide to
discuss this in a separate thread, I will start that thread just by
responding to these questions and design I have in mind.

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

On Mon, Apr 7, 2025 at 1:19 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Sat, Apr 5, 2025 at 6:20 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > Hi Ashutosh and Peter,
> >
> > Since this PGQ feature won't be in PG 18, I'd like to raise a discussion of
> > the possibility of implementing the quantifier feature, which I think is a
> > quite useful feature in the graph database area.
>
> I agree that quantifiers feature is very useful; it's being used in
> many usecases. However, it's a bit of a complex feature. IMO, we
> should keep that discussion as well as the patch in a separate thread,
> so that this patchset doesn't grow too large to review and also
> discussion in this thread can remain focused. Once we get the current
> patch set reviewed and committed we can tackle the quantifier problem
> in a separate discussion. Of course that doesn't mean that we can not
> start discussion, try POC and even a working patch for quantifier
> support.
>
> Peter may think otherwise.
>
> >
> > I'll start with a graph definition first.
> >
> > `Person(id, name, age, sex)` with id as PK
> > `Knows(id, start_id, end_id, since)` with id as PK, start_id and
> > end_id FK referencing Person's id
> >
> > insert into Person values(1, 'A', 31, 'M'), (2, 'B', 30, 'F'), (3,
> > 'C', 33, 'M'), (4, 'D', 31, 'F'), (5, 'E', 32, 'M'), (6, 'F', 33,
> > 'M');
> > insert into Knows values (1, 1, 2, '2020');  -- A knows B since 2020
> > insert into Knows values (2, 1, 3, '2021');  -- A knows C since 2021
> > insert into Knows values (3, 1, 4, '2020');  -- A knows D since 2020
> > insert into Knows values (4, 2, 4, '2023');  -- B knows D since 2023
> > insert into Knows values (5, 3, 5, '2022');  -- C knows E since 2022
> > insert into Knows values (6, 2, 6, '2021');  -- B knows F since 2021
> > insert into Knows values (7, 4, 6, '2020');  -- D knows F since 2020
> >
> > Then we create a property graph:
> >
> > CREATE property graph new_graph
> > VERTEX TABLES (Person)
> > EDGE TABLES (Knows);
> >
> > If we want to find A's non-directly known friends within 3 hops, we can query:
> >
> > select name from graph_table (new_graph match (a:Person WHERE a.name =
> > 'A') --> (b:Person) --> (c:Person) COLUMNS (c.name))
> > union
> > select name from graph_table (new_graph match (a:Person WHERE a.name =
> > 'A') --> (b:Person) -->(c:Person)-->(d:Person) COLUMNS (d.name));
> >
> > Or if we support quantifier, we can simply the query as:
> >
> > select name from graph_table (new_graph match (a:Person WHERE a.name =
> > 'A') -->{2,3} (b:Person) COLUMNS (b.name));
> >
> > In the current design of PostgreSQL, we can rewrite this pattern with
> > quantifiers to
> > the union form with some effort.
> >
> > But what if the pattern is more complicated, for example:
> >
> > 1. select name, since from graph_table (new_graph match (a:Person
> > WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
> > r.since));
> > Can we support the r.since column? I guess not, in this case r is a
> > variable length edge.
> >
> > 2. select name, count from graph_table (new_graph match (a:Person
> > WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
> > count(r)));
> > Can we support this count aggregation(this is called horizontal
> > aggregation in Oracle's pgql)? How can the executor know the length of
> > the variable length edge?
> >
> > 3. What if the query doesn't specify the Label of edge, and there can
> > be different edge labels of r, can we easily do the rewrite?
> >
> > I did some study of the apache age, they have fixed columns for node
> > labels(id, agtype)
> > and edge labels(id, source_id, end_id, agtype), agtype is kind of
> > json. So they can
> > resolve the above question easily.
> >
> > Above are just my random thoughts of the quantifier feature, I don't have a copy
> > of the PGQ standard, so I'd like to hear your opinion about this.
> >
>
> I think the questions you have raised are valid. If we decide to
> discuss this in a separate thread, I will start that thread just by
> responding to these questions and design I have in mind.

I'm ok with starting a new thread for quantifier discussion, and I'd
really happy to know your design on this.

>
> --
> Best Wishes,
> Ashutosh Bapat



--
Regards
Junwang Zhao

 What is the current thinking about getting PGQ committed ?

The attached fixes *ONLY* the duplicate OIDs error to get it build again in CI

On Mon, Apr 7, 2025 at 10:13 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Ashutosh,
>
> On Mon, Apr 7, 2025 at 1:19 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Sat, Apr 5, 2025 at 6:20 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > >
> > > Hi Ashutosh and Peter,
> > >
> > > Since this PGQ feature won't be in PG 18, I'd like to raise a discussion of
> > > the possibility of implementing the quantifier feature, which I think is a
> > > quite useful feature in the graph database area.
> >
> > I agree that quantifiers feature is very useful; it's being used in
> > many usecases. However, it's a bit of a complex feature. IMO, we
> > should keep that discussion as well as the patch in a separate thread,
> > so that this patchset doesn't grow too large to review and also
> > discussion in this thread can remain focused. Once we get the current
> > patch set reviewed and committed we can tackle the quantifier problem
> > in a separate discussion. Of course that doesn't mean that we can not
> > start discussion, try POC and even a working patch for quantifier
> > support.
> >
> > Peter may think otherwise.
> >
> > >
> > > I'll start with a graph definition first.
> > >
> > > `Person(id, name, age, sex)` with id as PK
> > > `Knows(id, start_id, end_id, since)` with id as PK, start_id and
> > > end_id FK referencing Person's id
> > >
> > > insert into Person values(1, 'A', 31, 'M'), (2, 'B', 30, 'F'), (3,
> > > 'C', 33, 'M'), (4, 'D', 31, 'F'), (5, 'E', 32, 'M'), (6, 'F', 33,
> > > 'M');
> > > insert into Knows values (1, 1, 2, '2020');  -- A knows B since 2020
> > > insert into Knows values (2, 1, 3, '2021');  -- A knows C since 2021
> > > insert into Knows values (3, 1, 4, '2020');  -- A knows D since 2020
> > > insert into Knows values (4, 2, 4, '2023');  -- B knows D since 2023
> > > insert into Knows values (5, 3, 5, '2022');  -- C knows E since 2022
> > > insert into Knows values (6, 2, 6, '2021');  -- B knows F since 2021
> > > insert into Knows values (7, 4, 6, '2020');  -- D knows F since 2020
> > >
> > > Then we create a property graph:
> > >
> > > CREATE property graph new_graph
> > > VERTEX TABLES (Person)
> > > EDGE TABLES (Knows);
> > >
> > > If we want to find A's non-directly known friends within 3 hops, we can query:
> > >
> > > select name from graph_table (new_graph match (a:Person WHERE a.name =
> > > 'A') --> (b:Person) --> (c:Person) COLUMNS (c.name))
> > > union
> > > select name from graph_table (new_graph match (a:Person WHERE a.name =
> > > 'A') --> (b:Person) -->(c:Person)-->(d:Person) COLUMNS (d.name));
> > >
> > > Or if we support quantifier, we can simply the query as:
> > >
> > > select name from graph_table (new_graph match (a:Person WHERE a.name =
> > > 'A') -->{2,3} (b:Person) COLUMNS (b.name));
> > >
> > > In the current design of PostgreSQL, we can rewrite this pattern with
> > > quantifiers to
> > > the union form with some effort.
> > >
> > > But what if the pattern is more complicated, for example:
> > >
> > > 1. select name, since from graph_table (new_graph match (a:Person
> > > WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
> > > r.since));
> > > Can we support the r.since column? I guess not, in this case r is a
> > > variable length edge.
> > >
> > > 2. select name, count from graph_table (new_graph match (a:Person
> > > WHERE a.name = 'A') -[r:Knows]->{2,3} (b:Person) COLUMNS (b.name,
> > > count(r)));
> > > Can we support this count aggregation(this is called horizontal
> > > aggregation in Oracle's pgql)? How can the executor know the length of
> > > the variable length edge?
> > >
> > > 3. What if the query doesn't specify the Label of edge, and there can
> > > be different edge labels of r, can we easily do the rewrite?
> > >
> > > I did some study of the apache age, they have fixed columns for node
> > > labels(id, agtype)
> > > and edge labels(id, source_id, end_id, agtype), agtype is kind of
> > > json. So they can
> > > resolve the above question easily.
> > >
> > > Above are just my random thoughts of the quantifier feature, I don't have a copy
> > > of the PGQ standard, so I'd like to hear your opinion about this.
> > >
> >
> > I think the questions you have raised are valid. If we decide to
> > discuss this in a separate thread, I will start that thread just by
> > responding to these questions and design I have in mind.
>
> I'm ok with starting a new thread for quantifier discussion, and I'd
> really happy to know your design on this.
>
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Regards
> Junwang Zhao
>
>

On Wed, Mar 12, 2025 at 10:01 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> >
>
> Thanks Junwang. I have added your patches to my local repository. Next
> time I post an updated set, I will post it along with your patches.
> Will merge them into the original patches as time permits.

I have reviewed your patches. adjusted them as described below.

0001 - is original patch by Peter but it has now accumulated a lot of
conflict resolution fixes, compilation fixes and bug fixes. The commit
message has a list of all the fixes.

0002 - WHERE clause support in graph pattern

0003 - supports cyclic path patterns

0004 - contains a variety of fixes, described in the commit message

0005 - Fixes access permission checks on property graph

0006 - support collation specification for property and handles
collation of vertex - edge quals, this includes Junwang's 0012 and
some part of his 0015. The edit related to reducing number of call
sites didn't look like a net improvement. Having two callsites each
surrounded by the caller's context is better than one.

0007 - pg_overexplain support for property graph (description of
GRAPH_TABLE RTE)

0008 - documentation fixes

0009 - support \d variants for property graph, includes Junwang's 0013

0010 - handles RTE_GRAPH_TABLE at some more places. This includes
Amit's fix for crash in ExecCheckPermissions.

0011 - deals with functions related to object addresses. It's clear
what should be the output of pg_describe_object(),
pg_identify_object() and pg_identify_object_as_address() for property
graph elements, property graph labels and property graph properties.
Those changes are included in the patch. But in order to support those
objects in pg_get_object_address() we need to add ObjectTypes
corresponding to those objects. Every object that is supported by
pg_describe_object(), pg_identify_object() and
pg_identify_object_as_address() is not supported by
pg_get_object_address(). The objects which are not supported do not
have their object types defined in the ObjectTypes enum. E.g. "index
column" or "view column" etc. So I have not implemented that support
for now. If required, we will need to add the corresponding types in
ObjectTypes and then handle them in all the places where ObjectType
enum is used. This contains Junwang's 0014.

0012 - A change to in the properties of labels or expression
associated with an element property requires the cached plans for
queries referring to the corresponding property graph. For that
CacheInvalidateHeapTupleCommon() needs to lookup OID of property graph
given a Form_pg_propgraph_label_property or
Form_pg_propgraph_element_label tuple. These catalogs do not store the
property graph OID directly. Instead they store label or property or
element oid. From those OIDs it is possible to get the property graph
ID from the corresponding label or property or element tuples.
However, when tuples in those catalogs are added along with the label,
element or property tuples, the latter are not visible. So either we
have to add property graph id to pg_propgraph_label_property and
pg_propgraph_element_label catalogs or add a CommandCounterIncrement()
after inserting element, property or label tuples. The latter may
cause some unwanted changes to be visible other than the desired ones,
so seems risky. Hence going with the first option. However, that
option consumes more space in the catalog. Once the catalog changes
are visible, the property graph id in the tuple won't be useful while
consuming space.

0013 and 0014 - minimum RLS related tests. In previous patchsets this
patch was too large and probably not all tests in that patch were
necessary. Now it's broken into two patches  .0013 which has extra
tests for the combinations. 0014 which I think are tested indirectly
in our regression suite. We may include some of those tests in the
main patchset.

I think two more changes are necessary but not included in the patchset
1. graph_table.sql uses mixed cases in SQL queries (e.g. it uses both
SELECT and select). I think we should use a uniform.
2. We have enhanced 002_pg_upgrade.pl to test dump and restore using
regression database. I think we should use that facility instead of
property graph specific dump/restore tests.

Here's the patchset rebased on the latest master.
--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

On Tue, Jul 22, 2025 at 8:40 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> Here's the patchset rebased on the latest master.

Thanks for the update. I was going through the patch series today and
had a few comments on the structure that might help with reviewing and
eventually committing, whoever ends up doing that. :)

I noticed that some functions, like graph_table_property_reference(),
are introduced in one patch and then renamed or removed in a later
one. It’s easier to review if such refactoring is avoided across
patches -- ideally, each patch should introduce code in its final
intended form. That also helps reduce the number of patches and makes
each one more self-contained.

One of the later patches (0004) collects several follow-up changes.
While it fixes a real bug -- a crash when GRAPH_TABLE is used inside
plpgsql due to a conflict with the columnref hook -- it also includes
incidental cleanups like switching to foreach_node(), updating
comments, and adjusting function signatures. Those would be better
folded into the patches that introduced the relevant code, rather than
grouped into a catch-all at the end. That keeps each patch focused and
easier to review -- and easier to merge when committing.

A general principle that might help: if you're refactoring existing
code, a standalone preliminary patch makes sense. But if you're
tweaking something just added in the same series, it’s usually better
to squash that into the original patch. The rename from
graph_table_property_reference() to transformGraphTablePropertyRef()
may be a fair exception since it reflects a shift prompted by the bug
fix -- but most other adjustments could be folded in without loss of
clarity.

I understand the intent to spell out each change, but if the details
are incidental to the overall design, they don’t necessarily need to
be split out. Explaining the reasoning in the thread is always
helpful, but consolidating the patches once the design has settled
makes things easier for both reviewers and committers.

Finally, attaching the patches directly, with versioned names like
v8-000x, instead of zipping them helps.  Many folks (myself included)
will casually skip a zip file because of the small hassle of unzipping
just to read a patch. I once postponed reading such a patch and didn’t
get back to it for quite a while. :)

--
Thanks, Amit Langote

Hi Andreas,

On Sun, Dec 22, 2024 at 3:15 AM Andreas Karlsson <andreas@proxel.se> wrote:
>
> On 10/29/24 8:55 PM, Andreas Karlsson wrote:
> > I especially dislike the static variable in our patch. And as far as I
> > understand it you can avoid the static by changing the lexer to use the
> > push parser so it can emit multiple terminal tokens from one parsed
> > token, but I have not looked into push parsers and have no idea how this
> > would affect performance.
>
> Updated the patch to remove the static variable. No clue why I thought
> that one was necessary.

I have not included this patch in the latest patchset. Given that
Peter E has written the grammar, it best be handled by him. Please
feel free to post it again as an additional patch in the latest
patchset. A standalone patch like this confused CI Bot.

--
Best Wishes,
Ashutosh Bapat

Hi Amit,
Thanks for your comments

On Wed, Jul 23, 2025 at 2:08 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Ashutosh,
>
> On Tue, Jul 22, 2025 at 8:40 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> > Here's the patchset rebased on the latest master.
>
> Thanks for the update. I was going through the patch series today and
> had a few comments on the structure that might help with reviewing and
> eventually committing, whoever ends up doing that. :)
>
> I noticed that some functions, like graph_table_property_reference(),
> are introduced in one patch and then renamed or removed in a later
> one. It’s easier to review if such refactoring is avoided across
> patches -- ideally, each patch should introduce code in its final
> intended form. That also helps reduce the number of patches and makes
> each one more self-contained.
>
> One of the later patches (0004) collects several follow-up changes.
> While it fixes a real bug -- a crash when GRAPH_TABLE is used inside
> plpgsql due to a conflict with the columnref hook -- it also includes
> incidental cleanups like switching to foreach_node(), updating
> comments, and adjusting function signatures. Those would be better
> folded into the patches that introduced the relevant code, rather than
> grouped into a catch-all at the end. That keeps each patch focused and
> easier to review -- and easier to merge when committing.
>
> A general principle that might help: if you're refactoring existing
> code, a standalone preliminary patch makes sense. But if you're
> tweaking something just added in the same series, it’s usually better
> to squash that into the original patch. The rename from
> graph_table_property_reference() to transformGraphTablePropertyRef()
> may be a fair exception since it reflects a shift prompted by the bug
> fix -- but most other adjustments could be folded in without loss of
> clarity.
>
> I understand the intent to spell out each change, but if the details
> are incidental to the overall design, they don’t necessarily need to
> be split out. Explaining the reasoning in the thread is always
> helpful, but consolidating the patches once the design has settled
> makes things easier for both reviewers and committers.

0001 is almost the same patch that Peter posted almost a year ago.
Each of 0002 onwards patches contains logically grouped changes. I
have changed 0001 minimally (so that it continues to apply, compile
and pass regression). I think this arrangement will make it easy for
Peter to review the changes. It will help him understand what all
changed since 0001 and each change in its own context. That has led to
a lot of overlap between 0002 and 0001 which I think should be
squashed together even now. But I would like to know what Peter thinks
- what would make his review easier. This arrangement might also help
Junwang, who has reviewed some patches, to continue his incremental
review.

I have rearranged the patches 0002 to 0014 so that the same change
isn't revised in multiple patches. Hope that helps.

I am also attaching a single patch as well to this email, so that
newer reviewers can review the changes as a whole.

SQL_PGQ_20250807.zip is separate patches zipped together.
SQL_PGQ_one_patch_20250807.zip is a single diff with changes from all
the patches squashed together. It's still zipped to avoid the email
being held for moderation.

>
> Finally, attaching the patches directly, with versioned names like
> v8-000x, instead of zipping them helps.  Many folks (myself included)
> will casually skip a zip file because of the small hassle of unzipping
> just to read a patch. I once postponed reading such a patch and didn’t
> get back to it for quite a while. :)

I used to attach those as separate patches till [1], but after that
the total size of the patchset grew so much that my emails used to get
held back for moderation. So I started attaching zip files. The
increase in size is mostly due to the 0014 patch. If we think that
it's not needed or can be trimmed down, we will be able to attach the
patchset as separate attachments.

Here's what changed between the last patchset and this

0001 has some TODOs, FIXMEs and XXXs. I and Junwang had fixed some of
them earlier. This patchset fixes some more. Below is my analysis of
each of them.
+
+ <!-- TODO: multiple path patterns in a graph pattern (comma-separated) -->
+
+ <!-- TODO: quantifiers -->

I think we should remove these TODOs since we are not going to support
these cases in this patchset. We will add the documentation when we
implement these features.

+/*
+ * 15.2
+ * PG_DEFINED_LABEL_SETS view
+ */
+
+-- TODO
+
+
+/*
+ * 15.3
+ * PG_DEFINED_LABEL_SET_LABELS view
+ */
+
+-- TODO
+
+
+/*
+ * 15.4
+ * PG_EDGE_DEFINED_LABEL_SETS view
+ */
+
+-- TODO
+
+/*
+ * 15.6
+ * PG_EDGE_TRIPLETS view
+ */
+
+-- TODO
+
+/*
+ * 15.15
+ * PG_VERTEX_DEFINED_LABEL_SETS view
+ */
+
+-- TODO

All these views are related to the defined label set. IIUC, defined
label set is a set of labels which is shared by multiple edges or
vertices of a property graph. Please note it's the whole set shared;
not just individual labels in the set. In that sense, I think, the
labels associated with each edge or vertex table in a property graph
form a defined label set. That way the property graph element's OID
becomes the defined label set's identifier. I am not sure if this view
has any practical use for tabular property graphs. It's not clear to
me what these views will contain. Since they are not essential for
SQL/PGQ functionality, I have not implemented them.

+ /*
+ * Now check number and names.
+ *
+ * XXX We could provide more detail in the error messages, but that would
+ * probably only be useful for some ALTER commands, because otherwise it's
+ * not really clear which label definition is the wrong one, and so you'd
+ * have to construct a rather verbose report to be of any use. Let's keep
+ * it simple for now.
+ */

Agree with XXX. I think the code is good enough as is. We can leave
XXX there in case we expect someone to improve it in future. Otherwise
we should remove XXX as well.

+
+ rel = table_open(PropgraphLabelPropertyRelationId, RowShareLock);
+ ScanKeyInit(&key[0],
+ Anum_pg_propgraph_label_property_plppropid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(propoid));
+ scan = systable_beginscan(rel, InvalidOid /* FIXME */ ,
+ true, NULL, 1, key);

I think the FIXME is to add the correct index OID.
pg_propgraph_label_property_label_prop_index contains plppropid as a
key but it's not the first column. So probably some other, yet not
defined, index is expected here?

+ALTER PROPERTY GRAPH g3 ALTER VERTEX TABLE t3 DROP LABEL t3l3;
+ALTER PROPERTY GRAPH g3 DROP VERTEX TABLES (t2); -- fail (TODO:
dubious error message)
+ERROR: cannot drop vertex t2 of property graph g3 because other
objects depend on it
+DETAIL: edge e1 of property graph g3 depends on vertex t2 of property graph g3
+HINT: Use DROP ... CASCADE to drop the dependent objects too.

I don't see what's dubious about this error message. edge e1 connects
t2 and t1. Dropping t2 would leave e1 dangling; error is preventing
it. Looks similar to how we prevent a referenced table being dropped.
Or is it expected that e1 be dropped without specifying cascade when
t2 is dropped since e1 has no existence without t2?

Also the patchset fixes the CI failure from the previous patchset.

The patches are reordered slightly. Each patch contains a commit
message that explains the changes in that patch.

[1] https://www.postgresql.org/message-id/CAExHW5sM+ZGVzR1428FsDHuWc-FU2-6zQr5j91KLh6vZaWY0ow@mail.gmail.com





--
Best Wishes,
Ashutosh Bapat

I have browsed this patch set a bit to get myself up to date.

General thoughts:

This is a large patch set, and it's not going to be perfect at the first 
try.  Especially, some of the parsing rules, query semantics, that kind 
of thing.  So I'm thinking that we should aim at integrating this with 
an understanding that it would be somewhat experimental, and then 
iterate on some of the details in the tree.  Obviously, that would still 
require that the overall architecture is ok, that it doesn't crash, that 
it satisfies security requirements.  Also, it should as much as possible 
follow the "if you don't use it, it doesn't affect you" rule.  So I'm 
specifically looking at where the patch intersects with existing code in 
critical ways, especially in parse analysis.  I want to spend more time 
reviewing that in particular.  Much of the rest of patch is 
almost-boilerplate: New DDL commands, new catalogs, associated tests and 
documentation.  It looks a lot, but most of it is not very surprising.

I found two areas where a bit more work could be done to separate the 
new code from existing code:

src/backend/utils/cache/inval.c: This looks suspicious.  The existing 
code only deals with very fundamental catalogs such as pg_class and 
pg_index.  It doesn't feel right to hardcode additional arguably very 
high-level PGQ-related catalogs there.  We should think of a different 
approach here.

src/test/regress/sql/rowsecurity.sql: I think it would be better to 
separate the new test cases into a separate file.  This test file is 
already large and complicated, and sprinkling a bunch of more stuff all 
over the place is going to make review and maintenance more complicated.

We also need to make sure that property graphs have security features 
equivalent to views.  For example, I suspect we need to integrate them 
with restrict_nonsystem_relation_kind.  There might be other similar 
things, to be checked.

Hi Peter,

On Mon, Aug 11, 2025 at 7:12 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> I have browsed this patch set a bit to get myself up to date.
>
> General thoughts:
>
> This is a large patch set, and it's not going to be perfect at the first
> try.  Especially, some of the parsing rules, query semantics, that kind
> of thing.  So I'm thinking that we should aim at integrating this with
> an understanding that it would be somewhat experimental, and then
> iterate on some of the details in the tree.

FWIW, I agree with this plan.

> Obviously, that would still
> require that the overall architecture is ok, that it doesn't crash, that
> it satisfies security requirements.  Also, it should as much as possible
> follow the "if you don't use it, it doesn't affect you" rule.

+1.

>
> I found two areas where a bit more work could be done to separate the
> new code from existing code:
>
> src/backend/utils/cache/inval.c: This looks suspicious.  The existing
> code only deals with very fundamental catalogs such as pg_class and
> pg_index.  It doesn't feel right to hardcode additional arguably very
> high-level PGQ-related catalogs there.  We should think of a different
> approach here.

I implemented it that way considering similarity between properties
and attributes. But I agree with you. I think it's a matter of calling
CacheInvalidateRelcacheByRelid with OID of property graph at
appropriate places in AlterPropGraph(), which is the only place where
components of the property graph are changed without changing property
graph's pg_class tuple. Creating and dropping a property graph will
touch the pg_class tuple which will trigger the current invalidation
mechanism.

>
> We also need to make sure that property graphs have security features
> equivalent to views.  For example, I suspect we need to integrate them
> with restrict_nonsystem_relation_kind.  There might be other similar
> things, to be checked.
>

Thanks for pointing me to restrict_nonsystem_relation_kind. I can take
care of that. Apart from that views have two security related settings
- security_invoker and security_barrier.

As mentioned in the commit message of 0003, property graphs behave
like views with security_invoker set to true. This will avoid any
privilege escalations through property graphs. This should be the
safest MVP. But it's different from the default security_invoker
setting for views. I was thinking that we will integrate the patches
with this behaviour and add security_invoker semantics in the next
iteration. Please let me know if you think that security_invoker and
security_definer semantics should both be supported before the first
commit itself.

Property graphs do not have security_barrier setting right now.
Property graphs do not have any conditions of their own. But
GRAPH_TABLE() may have. WHERE conditions in the GRAPH_TABLE are
executed like other conditions on the underlying table (if our
optimizer can push the conditions below UNION). I guess we need to
support security_barrier semantics on property graph so that WHERE
conditions in GRAPH_TABLE are executed before other conditions, but it
doesn't seem to be critical to me for the first cut as users can
leverage security_barrier views for the same, if required. But please
let me know if you think that security_barrier semantics should also
be supported before the first commit.

RLS is already covered.

Is there anything that is missing?

>
> src/test/regress/sql/rowsecurity.sql: I think it would be better to
> separate the new test cases into a separate file.  This test file is
> already large and complicated, and sprinkling a bunch of more stuff all
> over the place is going to make review and maintenance more complicated.

I sprinkled the queries in that file since the tables, RLS rules,
scenarios to test were readily available there. But I agree with you
that a file would be better. In fact, it's better to create a separate
file graph_table_security.sql for tests related to security aspects of
property graph covering RLS, restrict_nonsystem_relation_kind,
security_invoker and security_barrier. What do you think? I will work
on that.

From the first sentence in this email, it seems that you are planning
a thorough review of this patch set. Are you waiting for the above
issues to be addressed before continuing the review?

--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

Thanks for the new patch set.

On Thu, Aug 7, 2025 at 6:47 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Amit,
> Thanks for your comments
>
> On Wed, Jul 23, 2025 at 2:08 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > Hi Ashutosh,
> >
> > On Tue, Jul 22, 2025 at 8:40 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > > Here's the patchset rebased on the latest master.
> >
> > Thanks for the update. I was going through the patch series today and
> > had a few comments on the structure that might help with reviewing and
> > eventually committing, whoever ends up doing that. :)
> >
> > I noticed that some functions, like graph_table_property_reference(),
> > are introduced in one patch and then renamed or removed in a later
> > one. It’s easier to review if such refactoring is avoided across
> > patches -- ideally, each patch should introduce code in its final
> > intended form. That also helps reduce the number of patches and makes
> > each one more self-contained.
> >
> > One of the later patches (0004) collects several follow-up changes.
> > While it fixes a real bug -- a crash when GRAPH_TABLE is used inside
> > plpgsql due to a conflict with the columnref hook -- it also includes
> > incidental cleanups like switching to foreach_node(), updating
> > comments, and adjusting function signatures. Those would be better
> > folded into the patches that introduced the relevant code, rather than
> > grouped into a catch-all at the end. That keeps each patch focused and
> > easier to review -- and easier to merge when committing.
> >
> > A general principle that might help: if you're refactoring existing
> > code, a standalone preliminary patch makes sense. But if you're
> > tweaking something just added in the same series, it’s usually better
> > to squash that into the original patch. The rename from
> > graph_table_property_reference() to transformGraphTablePropertyRef()
> > may be a fair exception since it reflects a shift prompted by the bug
> > fix -- but most other adjustments could be folded in without loss of
> > clarity.
> >
> > I understand the intent to spell out each change, but if the details
> > are incidental to the overall design, they don’t necessarily need to
> > be split out. Explaining the reasoning in the thread is always
> > helpful, but consolidating the patches once the design has settled
> > makes things easier for both reviewers and committers.
>
> 0001 is almost the same patch that Peter posted almost a year ago.
> Each of 0002 onwards patches contains logically grouped changes. I
> have changed 0001 minimally (so that it continues to apply, compile
> and pass regression). I think this arrangement will make it easy for
> Peter to review the changes. It will help him understand what all
> changed since 0001 and each change in its own context. That has led to
> a lot of overlap between 0002 and 0001 which I think should be
> squashed together even now. But I would like to know what Peter thinks
> - what would make his review easier. This arrangement might also help
> Junwang, who has reviewed some patches, to continue his incremental
> review.
>
> I have rearranged the patches 0002 to 0014 so that the same change
> isn't revised in multiple patches. Hope that helps.
>
> I am also attaching a single patch as well to this email, so that
> newer reviewers can review the changes as a whole.
>
> SQL_PGQ_20250807.zip is separate patches zipped together.
> SQL_PGQ_one_patch_20250807.zip is a single diff with changes from all
> the patches squashed together. It's still zipped to avoid the email
> being held for moderation.
>
> >
> > Finally, attaching the patches directly, with versioned names like
> > v8-000x, instead of zipping them helps.  Many folks (myself included)
> > will casually skip a zip file because of the small hassle of unzipping
> > just to read a patch. I once postponed reading such a patch and didn’t
> > get back to it for quite a while. :)
>
> I used to attach those as separate patches till [1], but after that
> the total size of the patchset grew so much that my emails used to get
> held back for moderation. So I started attaching zip files. The
> increase in size is mostly due to the 0014 patch. If we think that
> it's not needed or can be trimmed down, we will be able to attach the
> patchset as separate attachments.
>
> Here's what changed between the last patchset and this
>
> 0001 has some TODOs, FIXMEs and XXXs. I and Junwang had fixed some of
> them earlier. This patchset fixes some more. Below is my analysis of
> each of them.
> +
> + <!-- TODO: multiple path patterns in a graph pattern (comma-separated) -->
> +
> + <!-- TODO: quantifiers -->
>
> I think we should remove these TODOs since we are not going to support
> these cases in this patchset. We will add the documentation when we
> implement these features.
>
> +/*
> + * 15.2
> + * PG_DEFINED_LABEL_SETS view
> + */
> +
> +-- TODO
> +
> +
> +/*
> + * 15.3
> + * PG_DEFINED_LABEL_SET_LABELS view
> + */
> +
> +-- TODO
> +
> +
> +/*
> + * 15.4
> + * PG_EDGE_DEFINED_LABEL_SETS view
> + */
> +
> +-- TODO
> +
> +/*
> + * 15.6
> + * PG_EDGE_TRIPLETS view
> + */
> +
> +-- TODO
> +
> +/*
> + * 15.15
> + * PG_VERTEX_DEFINED_LABEL_SETS view
> + */
> +
> +-- TODO
>
> All these views are related to the defined label set. IIUC, defined
> label set is a set of labels which is shared by multiple edges or
> vertices of a property graph. Please note it's the whole set shared;
> not just individual labels in the set. In that sense, I think, the
> labels associated with each edge or vertex table in a property graph
> form a defined label set. That way the property graph element's OID
> becomes the defined label set's identifier. I am not sure if this view
> has any practical use for tabular property graphs. It's not clear to
> me what these views will contain. Since they are not essential for
> SQL/PGQ functionality, I have not implemented them.
>
> + /*
> + * Now check number and names.
> + *
> + * XXX We could provide more detail in the error messages, but that would
> + * probably only be useful for some ALTER commands, because otherwise it's
> + * not really clear which label definition is the wrong one, and so you'd
> + * have to construct a rather verbose report to be of any use. Let's keep
> + * it simple for now.
> + */
>
> Agree with XXX. I think the code is good enough as is. We can leave
> XXX there in case we expect someone to improve it in future. Otherwise
> we should remove XXX as well.
>
> +
> + rel = table_open(PropgraphLabelPropertyRelationId, RowShareLock);
> + ScanKeyInit(&key[0],
> + Anum_pg_propgraph_label_property_plppropid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(propoid));
> + scan = systable_beginscan(rel, InvalidOid /* FIXME */ ,
> + true, NULL, 1, key);
>
> I think the FIXME is to add the correct index OID.
> pg_propgraph_label_property_label_prop_index contains plppropid as a
> key but it's not the first column. So probably some other, yet not
> defined, index is expected here?
>
> +ALTER PROPERTY GRAPH g3 ALTER VERTEX TABLE t3 DROP LABEL t3l3;
> +ALTER PROPERTY GRAPH g3 DROP VERTEX TABLES (t2); -- fail (TODO:
> dubious error message)
> +ERROR: cannot drop vertex t2 of property graph g3 because other
> objects depend on it
> +DETAIL: edge e1 of property graph g3 depends on vertex t2 of property graph g3
> +HINT: Use DROP ... CASCADE to drop the dependent objects too.
>
> I don't see what's dubious about this error message. edge e1 connects
> t2 and t1. Dropping t2 would leave e1 dangling; error is preventing
> it. Looks similar to how we prevent a referenced table being dropped.
> Or is it expected that e1 be dropped without specifying cascade when
> t2 is dropped since e1 has no existence without t2?
>
> Also the patchset fixes the CI failure from the previous patchset.
>
> The patches are reordered slightly. Each patch contains a commit
> message that explains the changes in that patch.
>
> [1] https://www.postgresql.org/message-id/CAExHW5sM+ZGVzR1428FsDHuWc-FU2-6zQr5j91KLh6vZaWY0ow@mail.gmail.com
>
>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat

I have some review comments, and hope some of them are helpful.

1.

doc/src/sgml/ddl.sgml

+<programlisting>
+CREATE PROPERTY GRAPH myshop
+    VERTEX TABLES (
+        products LABEL product,
+        customers LABEL customer,
+        orders LABEL order
+    )
+    EDGE TABLES (
+        order_items SOURCE orders DESTINATION products LABEL contains,
+        customer_orders SOURCE customers DESTINATION orders LABEL has
+    );
+</programlisting>

order is a reserved keyword, so the following example will fail, we
should not give a wrong example in our document.

2.

doc/src/sgml/information_schema.sgml

+  <title><literal>pg_element_table_key_columns</literal></title>
+
+  <para>
+   The view <literal>pg_element_key_columns</literal> identifies which columns
+   are part of the keys of the element tables of property graphs defined in
+   the current database.  Only those property graphs are shown that the
+   current user has access to (by way of being the owner or having some
+   privilege).
+  </para>


+  <title><literal>pg_element_table_properties</literal></title>
+
+  <para>
+   The view <literal>pg_element_table_labels</literal> shows the definitions
+   of the properties for the element tables of property graphs defined in the
+   current database.  Only those property graphs are shown that the current
+   user has access to (by way of being the owner or having some privilege).
+  </para>

the <title> and the <para> doesn't match.

3.

doc/src/sgml/queries.sgml

+   <para>
+    A path pattern thus matches a sequence of vertices and edges.  The
+    simplest possible path pattern is
+<programlisting>
+()
+</programlisting>
+    which matches a single vertex.  The next simplest pattern would be
+<programlisting>
+()-[]->()
+</programlisting>
+    which matches a vertex followed by an edge followed by a vertex.  The
+    characters <literal>()</literal> are a vertex pattern and the characters
+    <literal>-[]-></literal> are an edge pattern.
+   </para>

the description seems wrong, when a user writes (), it should match
all vertices in a property graph, for example:

explain SELECT customer_name FROM GRAPH_TABLE (myshop MATCH () COLUMNS
(1 AS customer_name));

[local] zjw@postgres:5432-73666=# explain SELECT customer_name FROM
GRAPH_TABLE (myshop MATCH () COLUMNS (1 AS customer_name));
                            QUERY PLAN
------------------------------------------------------------------
 Append  (cost=0.00..89.40 rows=3960 width=4)
   ->  Seq Scan on customers  (cost=0.00..18.50 rows=850 width=4)
   ->  Seq Scan on orders  (cost=0.00..32.60 rows=2260 width=4)
   ->  Seq Scan on products  (cost=0.00..18.50 rows=850 width=4)
(4 rows)

4.

doc/src/sgml/ref/create_property_graph.sgml

+<programlisting>
+CREATE PROPERTY GRAPH g1
+    VERTEX TABLES (
+        v1 LABEL foo PROPERTIES (x AS a * 2) LABEL bar PROPERTIES (x AS a * 2)
+    ) ...
+</programlisting>
+      but this would not:
+<programlisting>
+CREATE PROPERTY GRAPH g1
+    VERTEX TABLES (
+        v1 LABEL foo PROPERTIES (x AS a * 2) LABEL bar PROPERTIES (x AS a * 10)
+    ) ...
+</programlisting></para>

The expr after  PROPERTIES should be (a * 2 AS x)

5.

src/backend/catalog/sql_features.txt

+G034 Path concatenation YES SQL/PGQ required

Do we support path concatenation?

+G070 Label expression: label disjunction NO SQL/PGQ required

I think this should be a YES?

6.

src/backend/commands/tablecmds.c

The description RenameRelation and RemoveRelations should adapt
property graph, below are diffs I suggest:

 /*
- * Execute ALTER TABLE/INDEX/SEQUENCE/VIEW/MATERIALIZED VIEW/FOREIGN TABLE
+ * Execute ALTER TABLE/INDEX/SEQUENCE/VIEW/MATERIALIZED VIEW/FOREIGN
TABLE/PROPERTY GRAPH
  * RENAME
  */
 ObjectAddress
@@ -4210,8 +4210,8 @@ RenameRelation(RenameStmt *stmt)

        /*
         * Grab an exclusive lock on the target table, index, sequence, view,
-        * materialized view, or foreign table, which we will NOT release until
-        * end of transaction.
+        * materialized view, foreign table or property graph, which we will NOT
+        * release until end of transaction.


 /*
  * RemoveRelations
  *             Implements DROP TABLE, DROP INDEX, DROP SEQUENCE, DROP VIEW,
- *             DROP MATERIALIZED VIEW, DROP FOREIGN TABLE
+ *             DROP MATERIALIZED VIEW, DROP FOREIGN TABLE, DROP PROPERTY GRAPH

7.

src/backend/nodes/nodeFuncs.c

exprLocation should have an arm for T_GraphPropertyRef? I suggest:

@@ -1811,6 +1811,9 @@ exprLocation(const Node *expr)
                case T_PartitionRangeDatum:
                        loc = ((const PartitionRangeDatum *) expr)->location;
                        break;
+               case T_GraphPropertyRef:
+                       loc = ((const GraphPropertyRef *) expr)->location;
+                       break;

8.

src/backend/rewrite/rewriteGraphTable.c

patch 0002

+ /*
+ * Label expressions from two element patterns need to be
+ * conjucted. Label conjuction is not supported right now

typo, should be conjuncted and conjunction.

9.

The line length of some code is quite long, which isn't ideal, but
it's not a major issue. It can be formatted before being committed.

BTW, the patch set seems not to apply to the current master.

I will go another round of review of the test cases, thanks.

--
Regards
Junwang Zhao