> Repro: > > CREATE TABLE v(id int PRIMARY KEY, vname text); > CREATE PROPERTY GRAPH g VERTEX TABLES (v); > SELECT * FROM GRAPH_TABLE(g MATCH (a) COLUMNS (a.vname)) gt > FOR UPDATE OF gt; > -- ERROR: unrecognized RTE type: 8 > > Attached a patch that returns ERRCODE_FEATURE_NOT_SUPPORTED "FOR ... cannot be > applied to a GRAPH_TABLE" with a position pointer, matching the convention used by > the function/tablefunc etc.
In a fully matured SQL/PGQ support, a row mark on GRAPH_TABLE should be pushed down to the resulting subqueries in rewriteGraphTable(). I see this similar to how the row mark is pushed down into views during the rewrite phase. I don't think the code changes will be massive, it will be a matter of invoking markQueryForLocking() on the query that replaces the graph pattern in rewriteGraphTable(). I am not sure whether we want to do that now when stabilizing the feature. Peter, what do you think?
I wanted to implement this by trying to push FOR UPDATE/SHARE down into the relational tables underlying the GRAPH_TABLE (the "fully matured SQL/PGQ" direction mentioned). But had some doubts around the implementation.
I'd like input on a few design questions from you/Peter/hackers:
Q1. Is the right place to do the pushdown rewriteGraphTable(), mirroring what ApplyRetrieveRule() does for views (i.e. call markQueryForLocking() on the freshly built subquery)? That would require exposing markQueryForLocking() outside rewriteHandler.c.
Q2. How should FOR UPDATE behave when the MATCH expands to more than one path and rewriteGraphTable() produces a UNION? markQueryForLocking() refuses to descend into set-operations, same as it does for views. Options: (a) reject FOR UPDATE on multi-path MATCH with a clear "cannot lock rows from a set-operation result" error, matching the view behaviour; (b) silently fall back to the current "raise relation lock only" behaviour and document it. I think it should be (a).
Q3. SQL/PGQ does not really define UPDATE/DELETE semantics for graph paths. Is the intent here that FOR UPDATE/SHARE on GRAPH_TABLE is purely a concurrency hint rather than a precursor to UPDATE WHERE CURRENT OF? If so, that should probably be documented next to the GRAPH_TABLE syntax in the SQL reference.
If we decide to not to support it, the code changes look on the right path. We need to update the following comment which appears earlier in the function to mention GRAPH_TABLE, /* ignore JOIN, SPECIAL, FUNCTION, VALUES, CTE RTEs */. I think it will be better to specify that this restriction is temporary in the comments at both the places; but that's arguable.
> Patch includes tests for all four locking strengths. > Since the code path looks simple we can just keep one of them as well and trim other > tests. Thoughts?
I don't think we need all the four tests, but we need a test for locking clause without relation since that takes a different code path.
Attached is v2-0001, rebased on master and addressing
the comments you added in case we plan on parking the feature
