Re: SQL Property Graph Queries (SQL/PGQ)

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