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. > > - 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 ... :) > > - 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". > [ more problems] > > To me, this class of problem seems completely unacceptable in a > committed feature. A small number of oversights of this class, in a feature of the size of PGQ, doesn't necessarily seem entirely unacceptable to me. We are all human, and we don't always have an existing pattern to model things after, making it harder to get it right (e.g. I don't think we have a real precedent for the type of dependencies that are needed here). But the density of such problems in PGQ, at this stage of the release cycle (and, TBH, probably at commit time), doesn't seem OK. Greetings, Andres Freund
