Hi Ashutosh,
On Fri, Dec 6, 2024 at 12:34 AM Ashutosh Bapat
<ashutosh.bapat....@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....@gmail.com> wrote:
> >
> > On Fri, Nov 22, 2024 at 7:29 PM Ashutosh Bapat
> > <ashutosh.bapat....@gmail.com> wrote:
> > >
> > > On Tue, Nov 19, 2024 at 10:08 PM Vik Fearing <v...@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....@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.
> > >
> > > Thanks for looking at the patches.
> > >
> > > >
> > > >
> > > > There are a few things that stick out to me.
> > > >
> > > >
> > > > 1.
> > > > I don't see any way to view the structure of of a property graph. For
> > > > example:
> > > >
> > > >
> > > > postgres=# CREATE TABLE objects (id INTEGER, color VARCHAR, shape
> > > > VARCHAR, size INTEGER);
> > > > CREATE TABLE
> > > > postgres=# CREATE PROPERTY GRAPH propgraph VERTEX TABLES (objects KEY
> > > > (id) PROPERTIES ALL COLUMNS);
> > > > CREATE PROPERTY GRAPH
> > > > postgres=# \dG propgraph
> > > > List of relations
> > > > Schema | Name | Type | Owner
> > > > -------------------+-----------+----------------+-------------
> > > > graph_table_tests | propgraph | property graph | vik.fearing
> > > > (1 row)
> > > >
> > > > postgres=# \d propgraph
> > > > Property graph "graph_table_tests.propgraph"
> > > > Column | Type
> > > > --------+------
> > > >
> > > > I don't really know what to do with that.
> > >
> > > Yes. It is on my TODO list. IMO we should just print the first line
> > > property graph "...". There are no predefined columns in this
> > > relation. And somehow redirect them to use \dG instead.
> >
> > \d+ propgraph prints the definition of property graph. I find \dG
> > similar to \di which doesn't print the structure of an index. Instead
> > \d+ <index name> prints it.
> >
> > In the attached patch series I have added patch 0008 to remove
> > unnecessary header
> > > > Column | Type
> > > > --------+------
> >
> > It still prints an extra line but I haven't found a way to eliminate
> > it. Hence 0008 is WIP. I have listed TODOs in the commit message of
> > that patch.
> >
> >
> > > >
> > > >
> > > > 2.
> > > > There is a missing newline in the \? help of psql.
> > > > HELP0(" \\dFt[+] [PATTERN] list text search templates\n");
> > > > HELP0(" \\dg[S+] [PATTERN] list roles\n");
> > > > HELP0(" \\dG[S+] [PATTERN] list property graphs"); <---
> > > > HELP0(" \\di[S+] [PATTERN] list indexes\n");
> > > > HELP0(" \\dl[+] list large objects, same as
> > > > \\lo_list\n");
> > > >
> > >
> > > Will fix that in the next set.
> >
> > Done. The fix is part of 0001 now.
> >
> >
> >
> > >
> > > >
> > > > I will continue to review this feature from the user's perspective.
> > > > Thank you for working on it, I am very excited to get this in.
> > >
> >
> > here's patchset rebased on 792b2c7e6d926e61e8ff3b33d3e22d7d74e7a437
> > with conflicts in rowsecurity.sql/out fixed.
> >
> > >
> > > 0001 - 0005 are the same as the previous set.
> > > 0007 - has RLS tests. It is the same as 0006 from the previous patch set.
> >
> > This is same as the previous patchset.
> >
> > >
> > > 0006 - is new addressing collation and edge-vertex link qual creation.
> > > This patch adds code to store collation and typmod to
> > > pg_propgraph_property catalog and propagate it to property graph
> > > references in GRAPH_TABLE. Collation is used by
> > > assign_query_collations and assign_expr_collations to propagate
> > > collation up the query and expression tree respectively. typmod is
> > > used to report typmod of property reference from exprTypemod().
> > >
> > > While finishing code to create vertex-edge link quals, I found that we
> > > need to find and store the operator to be used for key matching in
> > > those quals. I think we have to do something similar to what primary
> > > key handling code does - find the equality operator when creating edge
> > > descriptor and store it in pg_propgraph_element. I am not sure whether
> > > we should add a dependency on the operator. I will look into this
> > > next.
> >
> > 0006 in the attached patch series completes this work. Now we find the
> > equality operators to be used for vertex-edge quals and save it in
> > pg_propgraph_element catalog and also add a dependency between the
> > edge element and the equality operators.
> >
> > 0008 - has WIP fix for \d and \d+
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
Here are some review opinions for 0001, I haven't seen the other
patches, forgive me if some of the opinions have already been
addressed.
1. In propgraph_edge_get_ref_keys, when finding a matching foreign key,
the fk pointer will always point to last ForeignKeyCacheInfo of
the edge relation, which is wrong. We should add another pointer
that remembers the matched ForeignKeyCacheInfo to ref_rel. See
attached 0001.
2. Some of the TODOs seem easy to address, attached 0002 does this.
3. Since property name and label name are unique in property graph
scope, some of the wording are not accurate. See attached 0003.
--
Regards
Junwang Zhao
From 57ececd2d76e83709afc01c5112f6747f750ccf6 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjw...@gmail.com>
Date: Sat, 21 Dec 2024 12:10:03 +0000
Subject: [PATCH v1 1/3] fix wrong ref keys
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.
Signed-off-by: Junwang Zhao <zhjw...@gmail.com>
---
src/backend/commands/propgraphcmds.c | 10 +++++++---
src/test/regress/expected/create_property_graph.out | 4 ++--
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/propgraphcmds.c
b/src/backend/commands/propgraphcmds.c
index 8430bfda7b..b9afb3a413 100644
--- a/src/backend/commands/propgraphcmds.c
+++ b/src/backend/commands/propgraphcmds.c
@@ -381,6 +381,7 @@ propgraph_edge_get_ref_keys(ParseState *pstate, const List
*keycols, const List
ListCell *lc;
int count = 0;
ForeignKeyCacheInfo *fk = NULL;
+ ForeignKeyCacheInfo *fk2 = NULL;
fkeys = RelationGetFKeyList(edge_rel);
foreach(lc, fkeys)
@@ -388,7 +389,10 @@ propgraph_edge_get_ref_keys(ParseState *pstate, const List
*keycols, const List
fk = lfirst_node(ForeignKeyCacheInfo, lc);
if (fk->confrelid == RelationGetRelid(ref_rel))
+ {
count++;
+ fk2 = fk;
+ }
}
if (count == 0)
@@ -402,10 +406,10 @@ propgraph_edge_get_ref_keys(ParseState *pstate, const
List *keycols, const List
errmsg("more than one suitable foreign
key exists for %s key of edge \"%s\"", type, aliasname),
parser_errposition(pstate, location));
- Assert(fk);
+ Assert(fk2);
- *outkey = array_from_attnums(fk->nkeys, fk->conkey);
- *outref = array_from_attnums(fk->nkeys, fk->confkey);
+ *outkey = array_from_attnums(fk2->nkeys, fk2->conkey);
+ *outref = array_from_attnums(fk2->nkeys, fk2->confkey);
}
}
diff --git a/src/test/regress/expected/create_property_graph.out
b/src/test/regress/expected/create_property_graph.out
index 43316fbc02..70c4afda52 100644
--- a/src/test/regress/expected/create_property_graph.out
+++ b/src/test/regress/expected/create_property_graph.out
@@ -96,7 +96,7 @@ SELECT pg_get_propgraphdef('g5'::regclass);
t12 KEY (b) PROPERTIES (b)
+
)
+
EDGE TABLES (
+
- t13 KEY (c) SOURCE KEY (e) REFERENCES t11 (a) DESTINATION KEY (e)
REFERENCES t12 (b) PROPERTIES (c, d, e)+
+ t13 KEY (c) SOURCE KEY (d) REFERENCES t11 (a) DESTINATION KEY (e)
REFERENCES t12 (b) PROPERTIES (c, d, e)+
)
(1 row)
@@ -255,7 +255,7 @@ SELECT * FROM information_schema.pg_edge_table_components
ORDER BY property_grap
regression | create_property_graph_tests | g4 |
e2 | t1 | SOURCE | a |
a | 1
regression | create_property_graph_tests | g4 |
e2 | t3 | DESTINATION | x |
x | 1
regression | create_property_graph_tests | g4 |
e2 | t3 | DESTINATION | t |
y | 2
- regression | create_property_graph_tests | g5 |
t13 | t11 | SOURCE | e |
a | 1
+ regression | create_property_graph_tests | g5 |
t13 | t11 | SOURCE | d |
a | 1
regression | create_property_graph_tests | g5 |
t13 | t12 | DESTINATION | e |
b | 1
(12 rows)
--
2.39.5
From 5f226e04412eda4b7f12e64a24618772505c4c04 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjw...@gmail.com>
Date: Sat, 21 Dec 2024 12:13:40 +0000
Subject: [PATCH v1 2/3] reduce several trivial TODOs
Signed-off-by: Junwang Zhao <zhjw...@gmail.com>
---
src/backend/catalog/objectaddress.c | 42 ++++++++++++++++++++++++-----
src/backend/utils/cache/lsyscache.c | 19 +++++++++++++
src/include/utils/lsyscache.h | 1 +
3 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/objectaddress.c
b/src/backend/catalog/objectaddress.c
index d1bce431c7..6f967add87 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -6149,16 +6149,46 @@ getObjectIdentityParts(const ObjectAddress *object,
}
case PropgraphElementRelationId:
- appendStringInfo(&buffer, "%u TODO", object->objectId);
- break;
+ {
+ char *elemname;
+
+ elemname =
get_propgraph_element_alias_name(object->objectId);
+ if (elemname)
+ {
+ appendStringInfoString(&buffer,
quote_identifier(elemname));
+ if (objname)
+ *objname = list_make1(elemname);
+ }
+ break;
+ }
case PropgraphLabelRelationId:
- appendStringInfo(&buffer, "%u TODO", object->objectId);
- break;
+ {
+ char *labelname;
+
+ labelname =
get_propgraph_label_name(object->objectId);
+ if (labelname)
+ {
+ appendStringInfoString(&buffer,
quote_identifier(labelname));
+ if (objname)
+ *objname =
list_make1(labelname);
+ }
+ break;
+ }
case PropgraphPropertyRelationId:
- appendStringInfo(&buffer, "%u TODO", object->objectId);
- break;
+ {
+ char *propname;
+
+ propname =
get_propgraph_property_name(object->objectId);
+ if (propname)
+ {
+ appendStringInfoString(&buffer,
quote_identifier(propname));
+ if (objname)
+ *objname = list_make1(propname);
+ }
+ break;
+ }
case PublicationRelationId:
{
diff --git a/src/backend/utils/cache/lsyscache.c
b/src/backend/utils/cache/lsyscache.c
index 3bf331b21a..6c081ca63f 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -32,6 +32,7 @@
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_proc.h"
+#include "catalog/pg_propgraph_element.h"
#include "catalog/pg_propgraph_label.h"
#include "catalog/pg_propgraph_property.h"
#include "catalog/pg_publication.h"
@@ -3717,6 +3718,24 @@ get_subscription_name(Oid subid, bool missing_ok)
return subname;
}
+char *
+get_propgraph_element_alias_name(Oid elemoid)
+{
+ HeapTuple tuple;
+ char *elemname;
+
+ tuple = SearchSysCache1(PROPGRAPHELOID, elemoid);
+ if (!tuple)
+ {
+ elog(ERROR, "cache lookup failed for element %u", elemoid);
+ return NULL;
+ }
+ elemname = pstrdup(NameStr(((Form_pg_propgraph_element)
GETSTRUCT(tuple))->pgealias));
+ ReleaseSysCache(tuple);
+
+ return elemname;
+}
+
char *
get_propgraph_label_name(Oid labeloid)
{
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 82d0bbb267..7708793364 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -206,6 +206,7 @@ extern char *get_publication_name(Oid pubid, bool
missing_ok);
extern Oid get_subscription_oid(const char *subname, bool missing_ok);
extern char *get_subscription_name(Oid subid, bool missing_ok);
+extern char *get_propgraph_element_alias_name(Oid elemoid);
extern char *get_propgraph_label_name(Oid labeloid);
extern char *get_propgraph_property_name(Oid propoid);
--
2.39.5
From 6a0aadc5687414673ec54e2a1a40ee913051622a Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjw...@gmail.com>
Date: Sat, 21 Dec 2024 12:41:31 +0000
Subject: [PATCH v1 3/3] some typos
Since property name and label name are unique in property graph
scope, some of the wording are not accurate.
Signed-off-by: Junwang Zhao <zhjw...@gmail.com>
---
doc/src/sgml/information_schema.sgml | 18 +++++++++---------
doc/src/sgml/ref/alter_property_graph.sgml | 2 +-
doc/src/sgml/ref/create_property_graph.sgml | 6 +++---
doc/src/sgml/ref/drop_property_graph.sgml | 2 +-
src/backend/commands/propgraphcmds.c | 4 ++--
.../regress/expected/create_property_graph.out | 8 ++++----
6 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/doc/src/sgml/information_schema.sgml
b/doc/src/sgml/information_schema.sgml
index c92b65b03a..dc0257902b 100644
--- a/doc/src/sgml/information_schema.sgml
+++ b/doc/src/sgml/information_schema.sgml
@@ -4227,7 +4227,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4346,7 +4346,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4430,7 +4430,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4505,7 +4505,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4589,7 +4589,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4700,7 +4700,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4774,7 +4774,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -4839,7 +4839,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
@@ -5255,7 +5255,7 @@ ORDER BY c.ordinal_position;
<structfield>property_graph_name</structfield>
<type>sql_identifier</type>
</para>
<para>
- Name of the property_graph
+ Name of the property graph
</para></entry>
</row>
</tbody>
diff --git a/doc/src/sgml/ref/alter_property_graph.sgml
b/doc/src/sgml/ref/alter_property_graph.sgml
index 604c518011..4ae53deb09 100644
--- a/doc/src/sgml/ref/alter_property_graph.sgml
+++ b/doc/src/sgml/ref/alter_property_graph.sgml
@@ -99,7 +99,7 @@ ALTER PROPERTY GRAPH [ IF EXISTS ] <replaceable
class="parameter">name</replacea
<term><literal>ALTER {VERTEX|NODE|EDGE|RELATIONSHIP} TABLE ... DROP
LABEL</literal></term>
<listitem>
<para>
- This form removes a new label from an existing vertex or edge table.
+ This form removes a label from an existing vertex or edge table.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/create_property_graph.sgml
b/doc/src/sgml/ref/create_property_graph.sgml
index f88d1194cb..e953725847 100644
--- a/doc/src/sgml/ref/create_property_graph.sgml
+++ b/doc/src/sgml/ref/create_property_graph.sgml
@@ -151,7 +151,7 @@ CREATE [ TEMP | TEMPORARY ] PROPERTY GRAPH <replaceable
class="parameter">name</
<listitem>
<para>
The vertex tables that the edge table is linked to. These refer to the
- aliases of a the vertex table.
+ aliases of the vertex tables.
</para>
</listitem>
</varlistentry>
@@ -251,8 +251,8 @@ CREATE PROPERTY GRAPH g1
<listitem>
<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
allowed:
<programlisting>
CREATE PROPERTY GRAPH g1
diff --git a/doc/src/sgml/ref/drop_property_graph.sgml
b/doc/src/sgml/ref/drop_property_graph.sgml
index 31cb77a2af..e16de5507b 100644
--- a/doc/src/sgml/ref/drop_property_graph.sgml
+++ b/doc/src/sgml/ref/drop_property_graph.sgml
@@ -96,7 +96,7 @@ DROP PROPERTY GRAPH g1;
<command>DROP PROPERTY GRAPH</command> conforms to ISO/IEC 9075-16
(SQL/PGQ), except that the standard only allows one property graph to be
dropped per command, and apart from the <literal>IF EXISTS</literal>
- option, which is a <productname>PostgreSQL</productname> extension..
+ option, which is a <productname>PostgreSQL</productname> extension.
</para>
</refsect1>
diff --git a/src/backend/commands/propgraphcmds.c
b/src/backend/commands/propgraphcmds.c
index b9afb3a413..93d3bc1bf1 100644
--- a/src/backend/commands/propgraphcmds.c
+++ b/src/backend/commands/propgraphcmds.c
@@ -848,7 +848,7 @@ insert_property_record(Oid graphid, Oid ellabeloid, Oid
pgerelid, const char *pr
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("property \"%s\" data type mismatch: %s
vs. %s",
propname, format_type_be(proptypid),
format_type_be(exprtypid)),
- errdetail("In a property graph, a property of
the same name has to have the same data type in each label."));
+ errdetail("In a property graph, a property of
the same name has to have the same data type."));
}
/*
@@ -995,7 +995,7 @@ check_element_properties(Oid peoid)
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("element
\"%s\" property \"%s\" expression mismatch: %s vs. %s",
NameStr(elform->pgealias), get_propgraph_property_name(propoid), dpa, dpb),
- errdetail("In a
property graph element, a property of the same name has to have the same
expression in each label."));
+ errdetail("In a
property graph, a property of the same name has to have the same expression."));
ReleaseSysCache(tuple3);
}
diff --git a/src/test/regress/expected/create_property_graph.out
b/src/test/regress/expected/create_property_graph.out
index 70c4afda52..475f2b7943 100644
--- a/src/test/regress/expected/create_property_graph.out
+++ b/src/test/regress/expected/create_property_graph.out
@@ -139,24 +139,24 @@ CREATE PROPERTY GRAPH gx
LABEL bar PROPERTIES (1 + a AS aa) -- expression mismatch
);
ERROR: element "t1" property "aa" expression mismatch: (1 + a) vs. (a + 1)
-DETAIL: In a property graph element, a property of the same name has to have
the same expression in each label.
+DETAIL: In a property graph, a property of the same name has to have the same
expression.
ALTER PROPERTY GRAPH g2
ADD VERTEX TABLES (
t1 AS t1x KEY (a) LABEL foo PROPERTIES (a + 1 AS aa)
LABEL bar PROPERTIES (1 + a AS aa) -- expression
mismatch
);
ERROR: element "t1x" property "aa" expression mismatch: (1 + a) vs. (a + 1)
-DETAIL: In a property graph element, a property of the same name has to have
the same expression in each label.
+DETAIL: In a property graph, a property of the same name has to have the same
expression.
CREATE PROPERTY GRAPH gx
VERTEX TABLES (
t1 KEY (a) PROPERTIES (b AS p1),
t2 PROPERTIES (k AS p1) -- type mismatch
);
ERROR: property "p1" data type mismatch: text vs. integer
-DETAIL: In a property graph, a property of the same name has to have the same
data type in each label.
+DETAIL: In a property graph, a property of the same name has to have the same
data type.
ALTER PROPERTY GRAPH g2 ALTER VERTEX TABLE t1 ADD LABEL foo PROPERTIES (b AS
k); -- type mismatch
ERROR: property "k" data type mismatch: integer vs. text
-DETAIL: In a property graph, a property of the same name has to have the same
data type in each label.
+DETAIL: In a property graph, a property of the same name has to have the same
data type.
CREATE PROPERTY GRAPH gx
VERTEX TABLES (
t1 KEY (a) LABEL l1 PROPERTIES (a, a AS aa),
--
2.39.5
