Locking and property graphs

Hi All,
In [1] we discussed many issues related to SQL/PGQ. There were many
locking related issues reported. Collecting them together with
respective solutions in one thread for future reference. I this the
issues and the solutions are straight forward. But having them
together will serve as a good checklist for the next round.

1. ALTER PROPERTY GRAPH did not exclude concurrent graph readers.
AlterPropGraph() used ShareRowExclusiveLock which serializes graph
alterations but does not conflict with the AccessShareLock held by
GRAPH_TABLE queries. GRAPH_TABLE rewriter reads different parts of the
property graph definition at different times. A concurrent
modification of the property graph could cause different reads to be
inconsistent with each other. Solution is to use AccessExclusiveLock
in AlterPropGraph().

2. DROP CASCADE could modify graph components without locking their
parent graph.
Locking a component object, being dropped as part of the DROP CASCADE,
alone does not conflict with readers or ALTER PROPERTY GRAPH, which
lock the graph relation. Thus modifications of property graphs caused
because of a CASCADed DROP may render a property graph inconsistent
especially in the presence of a concurrent AlterPropGraph(). Also the
concurrent readers may read inconsistent property graph definition.
Solution is to extend AcquireDeletionLock() to find and take
AccessExclusiveLock (like AlterPropGraph()) on the containing graph
for a property graph component. graph-before-component order. When
performDeletion() encounters a property graph component it should
invalidate all the caches that reference the property graph. This has
been briefly mentioned in [2] as well.

3. insert_property_records() read element-table attributes before
locking that table.
ALTER PROPERTY GRAPH ADD LABEL and ADD PROPERTIES can reach it without
an element-table lock; PROPERTIES ALL may scan pg_attribute() without
a lock on the element table. Solution, call table_open(pgerelid,
AccessShareLock) at the beginning of the function instead of later.

4. Read-only catalog scans used unexplained RowShareLock modes.
is_property_associated_with_label() and get_element_property_expr()
used RowShareLock on pg_propgraph_element_label;
insert_property_records() used it on pg_attribute. RowShareLock does
not conflict with the RowExclusiveLock used for catalog writes, so it
does not provide the required protection against concurrent metadata
changes. The scans should instead use AccessShareLock. The relevant
graph or element relation lock provides that protection. This is
lock-mode cleanup, separate from the missing element-table lock above.

5. Document in replace_property_refs()'s prologue that its callers
have to take a lock on the property graph. Better to confirm that
assumption with a Assert(CheckRelationOidLockedByMe(propgraphid,
AccessShareLock, true)) there.

6. Rewriter element-table metadata access before locking.
build_edge_vertex_link_quals() calls get_atttypetypmodcoll() before
the locking the corresponding element table. We did not find a visible
hazard, but it's better to lock the element table before accessing its
metadata.

7. Concurrent DDL needed coverage while a query was actually being rewritten.
Add isolation tests for DDL vs query, AlterPropGraph vs DROP CASCADE.

[1] https://www.postgresql.org/message-id/dqa5mstx5mna3i7s23pdwl4m6bek7gqsgfccef44wjpswizufi@3aa6vzri3cat
[2] https://www.postgresql.org/message-id/CAExHW5sUhT9MmJSEGAKMX8z8NYkt0dxeGw_Q4f7_afVDtRkCZw@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat