> I concur, this isn't ready for v19. And I think it might not be ready to stay > in 20 either.
and > In addition to the manually found things in the last few emails, I also > started an AI review. Which found some not good stuff. I checked manually > that all of these actually are issues. My claude feature-cross check analysis also finished, and a significant amount of issues it reported on master are related to PGQ. I did not attempt a targeted detailed review of the PGQ commits independently, this was only the "Let's try to use PGQ together with <X> and see what happens" review. I planned to open separate threads about the most critical not yet reported findings today and start working on fixes for some of them, but based on the recent messages in this thread I'll just drop a short summary here for now. I think all of these are unreported, but I might have missed something. Initially I asked claude to cross-check what was already reported, then I tried to verify again manually that the remaining are not really not yet mentioned. All of these have claude generated reproducers I can share, but I want to point out that I did not properly verify them manually yet, I was planning on doing that later during the day before starting threads about them. But even if some of them are end up being false reports, the number of them itself is significant. 1. DROP OWNED BY fails as soon as the role holds any privilege on a property graph, and DROP ROLE then fails on the leftover pg_shdepend row. 2. pg_dump silently loses any graph whose edge keys were inferred from a foreign key. 3. Wrong results from keys that do not identify. The uniqueness check you mentioned is one half of it, there are two more ways to get there with a key that is enforced: - the collation guard in propgraph_edge_get_ref_keys() only fires when both sides carry an explicit non-default collation, so a plain text vertex key against an edge column declared with a nondeterministic collation is accepted, and one edge row becomes one path per case-insensitive match. ATAddForeignKeyConstraint() refuses the same column pair. - element RTEs are built with inh = true, but the implicit key comes from the parent's PK only. CREATE TABLE ... INHERITS (vertex_table) with duplicate ids in the child multiplies paths from the same edge row, and the child needs no privilege on the graph. ONLY is not spellable in the element table syntax. Partitioned tables are fine, the PK covers the partition key. 4. No relkind check on element tables either, not only on the ALTER target. 5. UPDATE / DELETE / MERGE with a GRAPH_TABLE in the source, and SELECT ... FOR UPDATE joining one, fail whenever the graph subquery is not pulled up (multi path patterns, volatile COLUMNS expressions): 'relation "g" does not have a composite type' from makeWholeRowVar(), and after fixing that 'cannot open relation "g"' from set_rel_width(). 6. The unqualified FOR UPDATE / FOR SHARE over a query using GRAPH_TABLE is accepted and silently locks nothing. The regression test even records this as "-- ignored". I want to point out that this is not only a missing feature: the element tables are opened with AccessShareLock, no UPDATE privilege is required (an equivalent security_invoker view refuses), and a two session test shows the other session's UPDATE going through while the "locked" reader sees the new value in the same transaction. 7. The privilege model has 3 surfaces that disagree, and your note about element tables being checked as the current user is one of them: - CREATE / ALTER PROPERTY GRAPH requires ownership of every element table (RangeVarCallbackOwnsRelation), not SELECT, which contradicts the "acts like a security invoker view" description, and an owner without SELECT can create a graph it cannot query. - at query time the graph ACL is checked as the view/rule owner and the element tables as the invoker, which makes property graphs a second, undocumented exception to rules.sgml's "checked against the privileges of the rule owner". - the SQL/PGQ information_schema views gate on privilege on the graph, not on the element table whose columns and property expressions they publish, and pg_element_table_properties prints the user written expression with no ownership test, unlike views.view_definition or triggers.action_condition. None of these leaks anything that pg_propgraph_* and pg_get_propgraphdef() don't already hand out ungated, but the boundary described is not the one enforced. 8. pg_get_propgraphdef() qualifies a temporary graph with get_namespace_name(), so it prints pg_temp_NN where every other deparse entry point prints pg_temp. Additionally it flagged that the current patches for the whole-row property issue and the pg_dump dependency patch don't cover all cases it discovered. I'll try to verify that later today and post separate replies about it, if it's the case.
