| From: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
|---|---|
| To: | Andres Freund <andres(at)anarazel(dot)de> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org, Melanie Plageman <melanieplageman(at)gmail(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
| Subject: | Re: PGQ catalog representation and pg_dump support |
| Date: | 2026-09-03 11:52:01 |
| Message-ID: | CAN4CZFP0qF6YH7dY9qufFuGf-wZ0+pKx3OXKDw8kN_AGOu4C1w@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> 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.
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Bertrand Drouvot | 2026-09-03 11:54:22 | Re: Offline data checksum changes can cause incorrect checksum state on standbys |
| Previous Message | Kirill Reshke | 2026-09-03 11:44:06 | Re: tablecmds: Deadlock caused by Attach Partition |