On Mon, May 18, 2026 at 8:22 AM Ashutosh Bapat <[email protected]> wrote: > > On Sun, May 17, 2026 at 5:12 PM Junwang Zhao <[email protected]> wrote: > > > > > > > > > > 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.
+1 for this change, we don't need to re-add the label and property to myshop. > >> > >> 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. > > Thanks for noticing. Here's attached correct patch. The patch LGTM, thanks for taking care of this. > > -- > Best Wishes, > Ashutosh Bapat -- Regards Junwang Zhao
