Hi Andres, Robert, I am looking at all the issues reported in this thread to summarize the things we need take care of in the next round. I will send a summary at the end and start discussion on topics that need further discussion. I am replying to individual discussions here.
On Fri, Sep 4, 2026 at 1:51 AM Andres Freund <[email protected]> wrote: > > Hi, > > On 2026-09-03 15:47:32 -0400, Robert Haas wrote: > > On Wed, Sep 2, 2026 at 5:33 PM Andres Freund <[email protected]> wrote: > > > This isn't just bad when the graph table query is in a view (where the > > > parser wouldn't have acquired locks), afaict parse-analysis doesn't > > > acquire > > > any locks on the element tables? > > > > I'm having trouble parsing this sentence. Are there some extra > > negatives in here someplace, or some missing ones? > > Something indeed isn't quite right, I think I was mainly just being too terse > (IIRC I had a longer explanation there that felt redundant, that I shortened > ... too much). > > What I wanted to draw attention to is that there are two different "locking > schemes" related to rewrite/ code. Rewrites on plain queries can rely on > locks being acquired during parse-analysis. Queries that originate in views > (and other rule related stuff) however, can't rely on that, since > parse-analysis is done during the view's creation. Therefore in the view case, > we rely on acquiring locks via AcquireRewriteLocks(). > > Here I think we acquire locks on the graph table elements in neither of the > two paths. > Once the proper dependencies (after they are fixed) are in place , the metadata of the element tables needed for rewriting the GRAPH_TABLE can not be altered because of dependencies. E.g. changing column type results in an error. I couldn't reproduce any hazards in this area. However, I agree that we should take locks on the element tables before accessing their metadata. > > > > - As mentioned in my last email, nothing protects the graph object against > > > being altered while rewriteGraphTable() is running, due to > > > AlterPropGraph() > > > only holding an AccessShareLock. That's bad. > > > > AlterPropGraph() takes ShareRowExclusiveLock on the property graph > > itself. The comment justifies why the lock level isn't weaker, but not > > why it isn't stronger; normally, DDL takes AccessExclusiveLock, and > > that would be my baseline expectation here, too. > > Yep. Seems pretty clear that AlterPropGraph() needs an AEL as it can affect > the "result shape". It's only ok to use a lower lock level if it's ok to > continue operating with an outdated definition of the object. > > I guess we could make uses of the propgraph use a more heavyweight lock > instead, but ... :) > Agreed. AlterPropGraph needs AccessExclusiveLock. > > > > - Also bad: Afaict a table or column - involved in a property graph - > > > being > > > dropped, does not seem to actually lock the property graph? Which means > > > that > > > could happen concurrently with rewriteGraphTable() even if > > > AlterPropGraph() > > > > I think the end of this sentence got cut off. > > Ooops. I guess I was intending to say something like "even if > AlterPropGraph() were to take an AEL, as a table that is part of the property > graph could be dropped without going through AlterPropGraph(), and therefore > without locking the propgraph with an AEL". > Yes, this is again missing from the depedency handling. I think when considering a property graph component in dependency list construction, we have to take an AEL on the property graph. I will include this point in my dependency thread. I will send a separate email about locking in property graph usage consolidating all the issues in one email. -- Best Wishes, Ashutosh Bapat
