public inbox for [email protected]help / color / mirror / Atom feed
Re: (SQL/PGQ) cache lookup failed for label 3+ messages / 3 participants [nested] [flat]
* Re: (SQL/PGQ) cache lookup failed for label @ 2026-05-18 02:07 Junwang Zhao <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Junwang Zhao @ 2026-05-18 02:07 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Ayush Tiwari <[email protected]>; zengman <[email protected]>; pgsql-hackers <[email protected]>; Peter Eisentraut <[email protected]> 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 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: (SQL/PGQ) cache lookup failed for label @ 2026-05-18 06:26 Ayush Tiwari <[email protected]> parent: Junwang Zhao <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Ayush Tiwari @ 2026-05-18 06:26 UTC (permalink / raw) To: Junwang Zhao <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; zengman <[email protected]>; pgsql-hackers <[email protected]>; Peter Eisentraut <[email protected]> Hi, On Mon, 18 May 2026 at 07:37, Junwang Zhao <[email protected]> wrote: > On Mon, May 18, 2026 at 8:22 AM Ashutosh Bapat > <[email protected]> wrote: > > > >> > > >> 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. +1, LGTM I've changed the status in CF entry status to Ready for Committer. https://commitfest.postgresql.org/patch/6760/ Regards, Ayush ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: (SQL/PGQ) cache lookup failed for label @ 2026-05-21 18:37 Ashutosh Bapat <[email protected]> parent: Ayush Tiwari <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Ashutosh Bapat @ 2026-05-21 18:37 UTC (permalink / raw) To: Ayush Tiwari <[email protected]>; +Cc: Junwang Zhao <[email protected]>; zengman <[email protected]>; pgsql-hackers <[email protected]>; Peter Eisentraut <[email protected]> On Sun, May 17, 2026 at 11:26 PM Ayush Tiwari <[email protected]> wrote: > > Hi, > > On Mon, 18 May 2026 at 07:37, Junwang Zhao <[email protected]> wrote: >> >> On Mon, May 18, 2026 at 8:22 AM Ashutosh Bapat >> <[email protected]> wrote: >> >> > >> >> > >> 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. > > > +1, LGTM > > I've changed the status in CF entry status to Ready for Committer. > https://commitfest.postgresql.org/patch/6760/ Thanks Ayush. While working on this, I found two other problems. Before we dive into those, I think we should commit the current patch. It need not wait for a solution to these problems. I was checking whether we should be expanding an empty label during the transformation stage instead of rewrite phase so that, in case the graph pattern is part of a view definition, the set of labels the empty label expression resolves to is stored in the catalogs. That way, we can create a dependency of view on the set of labels at the time of view. After discussing it with Peter Eisentraut, we feel that doing so will be good for future-proofing pg_dump of views containing graph patterns with empty labels. We will definitely need it before supporting all properties reference since the properties that the all properties reference expands to depends upon the set of labels in the label expression. We need to create dependencies between those properties and the view and hence need dependencies between labels (that the empty label expression resolves to) and the view. I will provide a patch for the same soon. When trying different things which could lead to invalidation of view if we don't add the dependency between labels (that the empty label expression resolves to) and the view, I found another way to invalidate the view. To reproduce it, run the following statements after running graph_table.sql tests. CREATE VIEW v_empty_label AS SELECT * FROM GRAPH_TABLE (g1 MATCH (v WHERE v.vprop1 = 10) COLUMNS (v.elname)); BEGIN; ALTER PROPERTY GRAPH g1 ALTER VERTEX TABLE v1 DROP LABEL l1; ALTER PROPERTY GRAPH g1 ALTER VERTEX TABLE v2 DROP LABEL l1; ALTER PROPERTY GRAPH g1 ALTER VERTEX TABLE v3 DROP LABEL l1; SELECT * FROM v_empty_label; ROLLBACK; The three ALTER TABLE statements leave the label behind since it's associated with the edge tables. The SELECT statement still throws "ERROR: property "elname" for element variable "v" not found". If we expand the empty label reference in the transformation phase and record dependencies between those labels and the view, we get a different error "ERROR: no property graph element of type "vertex" has label "l1" associated with it in property graph "g1"". The first error is a bit obscure compared to the second one. So there's relative improvement. Myself and Peter thought that the second error is an artifact of the term "vertex labels" in the standard; a term which is not properly defined in the standard. There are two solutions 1. We create vertex label and edge label as separate objects and record depdencies accordingly 2. We ignore the term "vertex/edge label" in the standard and handle labels that exist in property graphs but have no associated element of required type in the query. I think this will require some corrections in standard to standardize the outcome of such queries. I will provide patches once we decide which approach to take. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-05-21 18:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-18 02:07 Re: (SQL/PGQ) cache lookup failed for label Junwang Zhao <[email protected]> 2026-05-18 06:26 ` Ayush Tiwari <[email protected]> 2026-05-21 18:37 ` Ashutosh Bapat <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox