Re: SQL Property Graph Queries (SQL/PGQ) - Mailing list pgsql-hackers

On Sun, Feb 23, 2025 at 8:54 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Mon, Feb 10, 2025 at 11:00 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> >
> > Hi Ashutosh,
> >
> > On Mon, Feb 10, 2025 at 2:14 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Thu, Feb 6, 2025 at 8:22 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
> > > > > >
> > > > >
> > > > > I see you have added some negative tests - object not found tests, but
> > > > > I didn't see positive tests. Hence I haven't added those changes in
> > > > > the attached patchset. But we certainly need those changes. You may
> > > > > want to submit a patch with positive tests. That code needs to be
> > > > > fixed before committing anyway.
> > > >
> > > > The attached file adds the positive tests.
> > >
> > > Hi Junwang,
> > > Thanks for the patch, but please post it as a separate patch in the
> > > full patch-set, otherwise CFBot gets confused.
> >
> > Ok, see the attached.
> >
> > 0001-0009 are the original patches of 20250127 with rebase of master
> > 0010 fix ci error due to `Show more-intuitive titles for psql
> > commands`, see a14707da564e
> > 0011 fix ci error due to unstable collation tests, we should not
> > depend on pg_catalog."default", I observed 002_pg_upgrade.pl failure
> > caused by this.
> > 0012 trivial refactor of property graph object address
> >
> > >
> > > --
> > > Best Wishes,
> > > Ashutosh Bapat
> >
> >
> >
> > --
> > Regards
> > Junwang Zhao
>
> Here is v11 with another trivial refactor, I add a seperate patch file 0013, in
> insert_property_record, there is no need to check `type`, `typmode` and
> `collation` if the property doesn't exists before, in AlterPropGraph, there is
> no need to call `check_all_labels_properties` for each added vertex or edge.
>
> Other files remain unchanged except I’ve added some missing document and
> typo fix we discussed in the list but not included in the previous
> patch, I included
> them in 0008.
>
> --
> Regards
> Junwang Zhao

Current patch set failed the cfbot, I did some analysis on this, the reason for
the failure is that backend has a core dump, I extract some queries from
graph_table.sql to reproduce the issue:

----------------- queries begin -----------------
CREATE SCHEMA graph_table_tests;
GRANT USAGE ON SCHEMA graph_table_tests TO PUBLIC;
SET search_path = graph_table_tests;

CREATE TABLE products (
    product_no integer PRIMARY KEY,
    name varchar,
    price numeric
);

CREATE TABLE customers (
    customer_id integer PRIMARY KEY,
    name varchar,
    address varchar
);

CREATE TABLE orders (
    order_id integer PRIMARY KEY,
    ordered_when date
);

CREATE TABLE order_items (
    order_items_id integer PRIMARY KEY,
    order_id integer REFERENCES orders (order_id),
    product_no integer REFERENCES products (product_no),
    quantity integer
);

CREATE TABLE customer_orders (
    customer_orders_id integer PRIMARY KEY,
    customer_id integer REFERENCES customers (customer_id),
    order_id integer REFERENCES orders (order_id)
);


CREATE VIEW customers_view AS SELECT customer_id, 'redacted' ||
customer_id AS name_redacted, address FROM customers;

CREATE PROPERTY GRAPH myshop2
    VERTEX TABLES (
        products,
        customers_view KEY (customer_id) LABEL customers,
        orders
    )
    EDGE TABLES (
        order_items KEY (order_items_id)
            SOURCE KEY (order_id) REFERENCES orders (order_id)
            DESTINATION KEY (product_no) REFERENCES products (product_no),
        customer_orders KEY (customer_orders_id)
            SOURCE KEY (customer_id) REFERENCES customers_view (customer_id)
            DESTINATION KEY (order_id) REFERENCES orders (order_id)
    );

CREATE VIEW customers_us_redacted AS SELECT * FROM GRAPH_TABLE
(myshop2 MATCH (c IS customers WHERE c.address = 'US')-[IS
customer_orders]->(o IS orders) COLUMNS (c.name_redacted AS
customer_name_redacted));

SELECT * FROM customers_us_redacted;         <----- abort on this query
----------------- queries end -----------------

The backend aborted in ExecCheckPermissions, a recent commit 525392d57
add the following check:

----------------- code begin -----------------
/*
* Ensure that we have at least an AccessShareLock on relations
* whose permissions need to be checked.
*
* Skip this check in a parallel worker because locks won't be
* taken until ExecInitNode() performs plan initialization.
*
* XXX: ExecCheckPermissions() in a parallel worker may be
* redundant with the checks done in the leader process, so this
* should be reviewed to ensure it’s necessary.
*/
Assert(IsParallelWorker() ||
CheckRelationOidLockedByMe(rte->relid, AccessShareLock,
true));
----------------- code end -----------------

The query causing the core dump doesn't call transformRangeGraphTable
because the graph table is not in the `from` lists, so the graph table
is not locked.

I added a trivial fix(v12-0014) that called table_open/table_close in
rewriteGraphTable, it now passed the regression test and cirrus ci test,
but I'm not sure it's the correct fix.

I hope Ashutosh can chime in and take a look at this problem.




--
Regards
Junwang Zhao