Regards Junwang Zhao On Mon, May 18, 2026 at 07:29 Ashutosh Bapat <[email protected]> wrote:
> On Fri, May 15, 2026 at 8:43 PM Ayush Tiwari > <[email protected]> wrote: > > > > Hi, > > > > > > On Fri, 15 May 2026 at 20:31, Junwang Zhao <[email protected]> wrote: > >> > >> Hi Ayush, > >> > >> >> > >> >> I also added regression coverage for both cases: > >> >> > >> >> DROP LABEL of a label used by a GRAPH_TABLE view > >> >> DROP PROPERTIES of a property used by a GRAPH_TABLE view > >> >> > >> >> Both now fail with the normal dependency error until the view is > dropped. > >> >> > >> >> Thoughts? > >> > >> I'd suggest adding two stmts to the regression that can cover that walk > of > >> graph_table_columns is also working. > >> > >> [local] zhjwpku@postgres:5432-52789=# ALTER PROPERTY GRAPH myshop > >> ALTER VERTEX TABLE customers ALTER LABEL customers DROP PROPERTIES > >> (name); > >> ALTER PROPERTY GRAPH > >> Time: 1.312 ms > >> > >> [local] zhjwpku@postgres:5432-52789=# ALTER PROPERTY GRAPH myshop > >> ALTER VERTEX TABLE products ALTER LABEL products DROP PROPERTIES > >> (name); > >> ERROR: cannot drop property name of property graph myshop because > >> other objects depend on it > >> DETAIL: view customers_us depends on property name of property graph > myshop > >> HINT: Use DROP ... CASCADE to drop the dependent objects too. > >> Time: 2.231 ms > >> > > > > Good point, thanks. I added that coverage in the attached v3. > > > > The test now also drops customers.name first, which is allowed because > the > > graph-level property still exists via products.name, and then verifies > that > > dropping products.name is rejected with the dependency error from > > customers_us. That should cover GraphPropertyRef nodes reached through > the > > GRAPH_TABLE COLUMNS list, in addition to the existing label and > graph-pattern > > property cases. > > > > I re-added customers.name afterward so the existing myshop graph remains > > unchanged for the following tests. > > Thanks Ayush for working on this and providing the patch. Thanks > Junwang for reviewing it. > > I have some more comments. > > } > + else if (IsA(node, GraphLabelRef)) > + { > + GraphLabelRef *glr = (GraphLabelRef *) node; > + > + /* > + * GRAPH_TABLE label reference: depend on the label catalog entry. > + * No expression substructure to recurse into. > > That comment is correct, however, the case doesn't return false, > giving an impression that we are recursing into the substructure. > expression_tree_walker() then returns false. But I am seeing an > inconsistency in when to "return false" and when not to. For example, > for some primitive nodes in expression_tree_walker() like Var, this > function returns false. But for other primitive nodes like Param it > doesn't. And there's not comment explaining this difference. I guess > newer additions to this function are relying on expression_tree_walker > to return false. So I just removed the misleading comment and let the > two new nodes rely on expression_tree_walker(). > > + */ > + add_object_address(PropgraphLabelRelationId, glr->labelid, 0, > + context->addrs); > + } > + else if (IsA(node, GraphPropertyRef)) > + { > + GraphPropertyRef *gpr = (GraphPropertyRef *) node; > + > + /* GRAPH_TABLE property reference: depend on the property entry. */ > + add_object_address(PropgraphPropertyRelationId, gpr->propid, 0, > + context->addrs); > + } > > @@ -536,6 +536,22 @@ SELECT g.* FROM x1, > ORDER BY customer_name, product_name; > SELECT pg_get_viewdef('customers_us'::regclass); > +-- A view defined over GRAPH_TABLE should record dependencies on the > labels > +-- and properties it references, so they cannot be dropped from under it. > +ALTER PROPERTY GRAPH myshop ALTER EDGE TABLE order_items DROP LABEL > list_items; > +ALTER PROPERTY GRAPH myshop ALTER EDGE TABLE wishlist_items > + DROP LABEL list_items; -- error > +ALTER PROPERTY GRAPH myshop ALTER EDGE TABLE order_items > + ADD LABEL list_items PROPERTIES (order_id AS link_id, product_no); > +ALTER PROPERTY GRAPH myshop ALTER VERTEX TABLE customers > + ALTER LABEL customers DROP PROPERTIES (address); -- error > +ALTER PROPERTY GRAPH myshop ALTER VERTEX TABLE customers > + ALTER LABEL customers DROP PROPERTIES (name); > +ALTER PROPERTY GRAPH myshop ALTER VERTEX TABLE products > + ALTER LABEL products DROP PROPERTIES (name); -- error > +ALTER PROPERTY GRAPH myshop ALTER VERTEX TABLE customers > + ALTER LABEL customers ADD PROPERTIES (name); > + > > Without the code changes, we do not see "cache lookup failed for label > " error, because there is nothing that uses this view after the drop. > In the attached patch, I have moved the DDL statements before > pg_get_viewdef() which throws "cache lookup failed" error without code > changes and for every DDL statement when we try it separately. > > My earlier comment or the test by Man might have misled you into > thinking that we need to drop properties or labels which are defined > multiple times so that we test that the dependency error does not > trigger when a property or a label is not orphaned. Sorry if that's > the case. I don't think that's the goal here. Further, such tests > require additional DDL to restore property graph state and also change > the view definition produced by pg_get_viewdef(). So I used DDLs that > drop properties or labels which are defined only once. > > I shortened the commit message by taking essential elements from your > commit message. > > Please review the attached patch. The attached patch seems not for this thread. > > > -- > Best Wishes, > Ashutosh Bapat >
