Re: PGQ catalog representation and pg_dump support

On Thu, Sep 3, 2026 at 4:35 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> 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.
>
>
> - out of bound reads due to broken unknown handling
>
>
> The problem here is that transformRangeGraphTable() uses p_resolve_unknowns
> to control what unknown is resolved to. Other places explicitly specify
> resolve_uknowns for from items (c.f. transformRangeSubselect() passing
> resolve_unknowns = true to parse_sub_analyze()).
>
> The problem is that
> generate_setop_from_pathqueries()->constructSetOpTargetlist() then resolves
> those uknowns as text. But the upper query levels still think its an
> unknown.
>
> Depending on the path taken, this then either triggers errors via
> CheckVarSlotCompatibility() or out-of-bound reads / crashes.
>
> This whole thing requires resolve_unknown = false, which I think basically
> means an INSERT.
>
>
> Repro in pgq-1.sql.
>

This reproduces. The first fix at [1] called
resolveTargetListUnknowns() unconditionally. I had found below query
which would end up in an error
SELECT c1, pg_typeof(c1), "null", pg_typeof("null") FROM
(SELECT * FROM (SELECT '1' AS c1, NULL AS "null") AS source
UNION
SELECT 2, NULL) AS combined;

ERROR: UNION types text and integer cannot be matched

which led me to make resolveTargetListUnknowns() conditional. But
looking at it now, equivalent query without GRAPH_TABLE also fails
with the same error
SELECT c1, pg_typeof(c1), "null", pg_typeof("null") FROM
(SELECT * FROM (SELECT '1' AS c1, NULL AS "null") AS source
UNION
SELECT 2, NULL) AS combined;
ERROR: UNION types text and integer cannot be matched

So, I think we need to call resolveTargetListUnknowns()
unconditionally. Have created a patch, which I will submit when the
SQL/PGQ is resurrected.

>
> - Whole-row reference to a GRAPH_TABLE expands to zero columns / triggers errors
>
> `addRangeTableEntryForGraphTable()` computes `coltypes`/`coltypmods`/`colcollations` locally and
> passes them only to `buildNSItemFromLists()` (`parse_relation.c:2219-2249`); it never assigns them
> to the RTE. The VALUES, TABLEFUNC, CTE and ENR cases in the same file all do. `expandRTE()` handles
> `RTE_GRAPH_TABLE` by iterating exactly those three lists (`parse_relation.c:3155`), reached from
> `expandRecordVariable()` (`parse_target.c:1558`).
>
> Repro in pgq-2.sql.

I remember going through all the fields of RangeTblEntry figuring out
which ones apply to GRAPH_TABLE RTE. It's not easy to figure out which
fields are applicable to a new RTE. Further the comment explaining
these fields hardly mentions whole-row reference. Maybe it's implied
but it's not obvious to me.

I have a patch fixing this and will submit when SQL/PGQ is resurrected.

>
>
> - No uniqueness check on element keys or edge reference keys
>
> propgraph_element_get_key(), in the column-list != NIL case, doesn't check
> that there's actually a unique key. But the docs seem to pretty clearly say
> that the key should be unique?
>
> Repro in pgq-3.sql.
>

Robert also has some comments about keys. I will respond to that email.

>
> - ALTER PROPERTY GRAPH ... ADD LABEL/PROPERTIES` doesn't check privs on the element table
>
> While vertex and edge table paths do check permissions, the add_labels path
> does not.
>
> Repro in pgq-4.sql
>

The permissions on the element tables have puzzled me since the
beginning. The code required the owner of the property graph also to
be owner of the element tables. But the standard doesn't seem to
specify so. A point I had raised earlier [2], but didn't get an answer
to. Probably it was playing defensive first and then expanding it.

>
> - Whole row references inside a graph break pg_dump (and \d+)
>
> make_propgraphdef_properties() calls get_attname() with missing_ok = false
> for any Var property expression. A whole-row reference has varattno == 0 and
> causes get_attname() to error out. The property does work at query time.
>
> Repro in pgq-5.sql
>

Responded to Taha who also raised this.

>
>
> There's also some minor stuff:
>
> - Wrong SQLSTATE + missing error position on undefined property: parse_graphtable.c:132-135 uses
> ERRCODE_SYNTAX_ERROR and omits parser_errposition, unlike the sibling label case at :189 (
> ERRCODE_UNDEFINED_OBJECT). Undefined label at :187-191 also omits the position.

Sami has already given reference to the thread where it was being discussed.

>
> - Element tables in a rewritten GRAPH_TABLE are always checked as the current
> user, while the property-graph object's own ACL is checked as the view owner
>
> I think this is intentional and somewhat defensible? I don't think it can
> realistically lead to more disclosure than intended? But it might be worth
> for others to think about.
>

This was also discussed in [2]. It was done to avoid exposing any data
through a property graph to a user who doesn't have access to the data
otherwise. However, I think there's a possibility that if element
tables are being accessed through a view, and use the invoker
semantics, there is a danger that we will leak data available to the
current user to the view owner. Needs investigation and fix. Have
noted it.

[1] https://www.postgresql.org/message-id/flat/CAHg%2BQDcyKNWyzDoKMxiZNjv7C-wAxs8y0ZoNkOV137Y%2Bnk3UXg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAExHW5v8u7-2H2LqWP3ybhh5GnAVVeCOYuTfkg9pmdnrLwAtNA%40mail.gmail.com
[3] https://commitfest.postgresql.org/patch/7173/

--
Best Wishes,
Ashutosh Bapat