Re: SQL Property Graph Queries (SQL/PGQ)

Hi Ashutosh,

On Tue, Jul 22, 2025 at 8:40 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> Here's the patchset rebased on the latest master.

Thanks for the update. I was going through the patch series today and
had a few comments on the structure that might help with reviewing and
eventually committing, whoever ends up doing that. :)

I noticed that some functions, like graph_table_property_reference(),
are introduced in one patch and then renamed or removed in a later
one. It’s easier to review if such refactoring is avoided across
patches -- ideally, each patch should introduce code in its final
intended form. That also helps reduce the number of patches and makes
each one more self-contained.

One of the later patches (0004) collects several follow-up changes.
While it fixes a real bug -- a crash when GRAPH_TABLE is used inside
plpgsql due to a conflict with the columnref hook -- it also includes
incidental cleanups like switching to foreach_node(), updating
comments, and adjusting function signatures. Those would be better
folded into the patches that introduced the relevant code, rather than
grouped into a catch-all at the end. That keeps each patch focused and
easier to review -- and easier to merge when committing.

A general principle that might help: if you're refactoring existing
code, a standalone preliminary patch makes sense. But if you're
tweaking something just added in the same series, it’s usually better
to squash that into the original patch. The rename from
graph_table_property_reference() to transformGraphTablePropertyRef()
may be a fair exception since it reflects a shift prompted by the bug
fix -- but most other adjustments could be folded in without loss of
clarity.

I understand the intent to spell out each change, but if the details
are incidental to the overall design, they don’t necessarily need to
be split out. Explaining the reasoning in the thread is always
helpful, but consolidating the patches once the design has settled
makes things easier for both reviewers and committers.

Finally, attaching the patches directly, with versioned names like
v8-000x, instead of zipping them helps. Many folks (myself included)
will casually skip a zip file because of the small hassle of unzipping
just to read a patch. I once postponed reading such a patch and didn’t
get back to it for quite a while. :)

--
Thanks, Amit Langote