| From: | Peter Eisentraut <peter(at)eisentraut(dot)org> |
|---|---|
| To: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
| Cc: | Amit Langote <amitlangote09(at)gmail(dot)com>, Junwang Zhao <zhjwpku(at)gmail(dot)com>, Vik Fearing <vik(at)postgresfriends(dot)org>, Ajay Pal <ajay(dot)pal(dot)k(at)gmail(dot)com>, Imran Zaheer <imran(dot)zhir(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: SQL Property Graph Queries (SQL/PGQ) |
| Date: | 2025-11-25 13:52:21 |
| Message-ID: | 3365298a-6a49-4d13-a343-69b1b0c545fc@eisentraut.org |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
I have done a rough review of the patch set version 20251120.
- There are "No newline at end of file" warnings from git diff for a
couple of files:
contrib/pg_overexplain/sql/pg_overexplain.sql
src/test/regress/sql/graph_table_rls.sql
Please fix those in the next patch set, and maybe check your editor
settings.
- Attached are two small patches with some small fixes I found in
passing. (I think some of them were also reported by Junwang Zhao in
the meantime.)
- In the test files, especially src/test/regress/sql/graph_table.sql,
there are wildly different SQL styles (capitalization, formatting) used,
depending on who added the test case (I guess). Let's keep that more
consistent.
- In src/backend/parser/analyze.c, the extracted function
constructSetOpTargetlist() needs a detailed comment.
- I'm not so sure about the semantics chosen in the patch "Access
permissions on property graph". I think according to the SQL standard,
once you have access to the property graph, you don't need access to the
underlying tables as well. I guess you did this to align with how views
work? We might need to think about this a bit more, and document
whatever the conclusion is. But for now it's just small amount of code
affected.
I think you could collapse all the patches into one patch now. I have
reviewed all the incremental patches and they all look ok to me. I have
made some notes about which things I want to review in more detail, such
as the access control issue, but that doesn't need to be kept as a
separate patch.
When you create future patches, consider using the git format-patch -v
option.
And then you can also just gzip the patch. That should make it even
smaller than the .zip file.
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-Typos-and-minor-style-fixes.patch.nocfbot | text/plain | 5.2 KB |
| 0002-Remove-added-tokens-from-plpgsql.patch.nocfbot | text/plain | 932 bytes |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Eisentraut | 2025-11-25 13:58:31 | Re: SQL Property Graph Queries (SQL/PGQ) |
| Previous Message | 河田達也 | 2025-11-25 13:48:06 | Re: [Proposal] Adding TRIM_SPACE option to COPY |