Re: PGQ catalog representation and pg_dump support

On Wed, Sep 2, 2026 at 5:33 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> - Afaict the locking in rewriteGraphTable() is wrong. Unless I am missing
> something, we're accessing the schema of the referenced tables without
> holding any locks on the tables (c.f. get_atttypetypmodcoll() calls in
> build_edge_vertex_link_quals())

Ouch.

> 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?

> - 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. This takes
AccessShareLock on the tables involved. As you also sort of say in
your followup email, there's no justification in here of any of the
lock levels taken, and some of them are non-standard for the types of
operations that they are.

> - 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.

What generally bothers me here is that it's sort of systematically
unclear how the integrity model is supposed to work. If we look at
something like build_edge_vertex_link_quals(), I think it incidentally
depends on quite a bit of stuff. That function does three consecutive
syscache lookups on PROPGRAPHELOID, which gives it two arrays of
attribute numbers and an array of equality operators. But this
function doesn't document why things can't change between one of those
syscache lookups and the next. Maybe we're guaranteed to have a strong
enough lock on the property graph at this point to prevent that, but
there's no clear documentation of that. Then we call
get_atttypetypmodcoll(), as you said, but I don't think we're
guaranteed to have any locks on the tables at all at that point, as
you said. So what guarantees that the table properties are still in
sync with the property graph properties? In fact, even apart from
locking, it doesn't appear that everything relevant is checked.

For example, uniqueness is a requirement:

CREATE TABLE v (a int, b text);
CREATE TABLE e (id int PRIMARY KEY, va int);
CREATE PROPERTY GRAPH g
VERTEX TABLES (v)
EDGE TABLES (e SOURCE KEY (va) REFERENCES v (a)
DESTINATION KEY (va) REFERENCES v (a));

This fails, because v has no primary key. But if you do ALTER TABLE v
ADD PRIMARY KEY (a) first then it succeeds, and you can immediately
turn around and do ALTER TABLE v DROP CONSTRAINT v_pkey afterwards.

Foreign keys are are a requirement, so this fails:

CREATE TABLE v1 (a int PRIMARY KEY, name text);
CREATE TABLE v2 (m int PRIMARY KEY, name text);
CREATE TABLE e (id int PRIMARY KEY, k1 int, k2 int);
CREATE PROPERTY GRAPH g
VERTEX TABLES (v1, v2)
EDGE TABLES (e SOURCE v1 DESTINATION v2);

But if you temporarily add foreign keys with ALTER TABLE e ADD
CONSTRAINT e_k1_fkey FOREIGN KEY (k1) REFERENCES v1, ADD CONSTRAINT
e_k2_fkey FOREIGN KEY (k2) REFERENCES v2, then the above command
succeeds, and you can turn around and drop the foreign keys afterward
with ALTER TABLE e DROP CONSTRAINT e_k1_fkey.

Or consider this example:

CREATE TABLE t1 (a int PRIMARY KEY, b text);
CREATE TABLE t2 (a int PRIMARY KEY);
CREATE PROPERTY GRAPH g
VERTEX TABLES (t1 LABEL l PROPERTIES ALL COLUMNS,
t2 LABEL l PROPERTIES ALL COLUMNS);

This fails, because the number of properties doesn't match. If we do
ALTER TABLE t2 ADD COLUMN b text the this works, and a command like
SELECT * FROM GRAPH_TABLE (g MATCH (n IS l) COLUMNS (n.a, n.b)) ORDER
BY a; succeeds. However, afterward we can do ALTER TABLE t2 DROP
COLUMN b CASCADE (the CASCADE is required), and the same SELECT now
fails. So once again, we can put the object into a state after
creation that wouldn't have been valid at creation time.

Also consider this:

CREATE TABLE v1 (a int PRIMARY KEY, b text, unused text);
CREATE TABLE v2 (m int PRIMARY KEY);
CREATE TABLE e (id int PRIMARY KEY, k1 int, k2 int);

CREATE PROPERTY GRAPH g
VERTEX TABLES (v1 LABEL l PROPERTIES (a, upper(b) AS bu),
v2)
EDGE TABLES (e KEY (id)
SOURCE KEY (k1) REFERENCES v1 (a)
DESTINATION KEY (k2) REFERENCES v2 (m));

ALTER TABLE v1 ALTER COLUMN a TYPE bigint;

This produces "ERROR: unexpected object depending on column: vertex
v1 of property graph g" which is obviously not good enough. I see Sami
has reported this one, too.

To me, this class of problem seems completely unacceptable in a
committed feature. Generally, if creating the object requires certain
properties to hold, then later DDL that would cause those properties
to be violated should also be blocked. That's not an absolutely
inviolable rule, and I believe we do have other cases where certain
things are only checked at creation time. But I think those are
usually hard-to-avoid corner cases with careful justification and
careful testing, which doesn't seem to be the case here. And notice
that we're not reaching the question of locking, where we might fear
that certain kinds of invalid states can be created under concurrency
that wouldn't normally be allowed. In these cases, we don't even need
the concurrency; the sanity checks aren't even there for the
single-session case. And it also doesn't look like this is a case
where some design principle exists but the code fails to fully conform
to it. There's just a big design gap here, AFAICS, which is not the
sort of thing we should be trying to plug after the initial commit.

> - The code doesn't seem to follow postgres' coding style much. E.g. there are
> a lot of very long lines without there being any reason for that.

Yes, this code is really hard to read.

--
Robert Haas
EDB: http://www.enterprisedb.com