Hi Ashutosh, On Tue, Jul 22, 2025 at 8:40 PM Ashutosh Bapat <ashutosh.bapat....@gmail.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
