Hi Robert, Thank you very much for your work on the pg_plan_advice patch series. It is an impressive and substantial contribution, and it seems like a meaningful step forward toward addressing long-standing query plan stability issues in PostgreSQL.
While reviewing the v7 patches, I noticed a few points that I wanted to raise for discussion: 1. GEQO interaction (patch 4): Since GEQO relies on randomized search, is there a risk that the optimizer may fail to explore the specific join order or path that is being enforced by the advice mask? In that case, could this lead to failures such as inability to construct the required join relation or excessive planning time if the desired path is not sampled? 2. Parallel query serialization (patches 1–3): Several new fields (subrtinfos, elidedNodes, child_append_relid_sets) are added to PlannedStmt, but I did not see corresponding changes in outfuncs.c / readfuncs.c. Without serialization support, parallel workers executing subplans or Append nodes may not receive this metadata. Is this handled elsewhere, or is it something still pending? 3. Alias handling when generating advice (patch 5): In pgpa_output_relation_name, the advice string is generated using get_rel_name(relid), which resolves to the underlying table name rather than the RTE alias. In self-join cases this could be ambiguous (e.g., my_table vs my_table). Would it be more appropriate to use the RTE alias when available? 4. Minor typo (patch 4): In src/include/nodes/relation.h, parititonwise appears to be a typo and should likely be partitionwise. I hope these comments are helpful, and I apologize in advance if any of this is already addressed elsewhere in the series. Best regards, Haibo On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <[email protected]> wrote: > Here's v7. > > In 0001, I removed "const" from a node's struct declaration, because > Tom gave me some feedback to avoid that on another recent patch, and I > noticed I had done it here also. 0002, 0003, and 0004 are unchanged. > > In 0005: > > - Refactored the code to avoid issuing SEMIJOIN_NON_UNIQUE() advice in > cases where uniqueness wasn't actually considered. > - Adjusted the code not to issue NO_GATHER() advice for non-relation > RTEs. (This is the issue reported by Ajay Pal in a recent message to > this thread, which was also mentioned in an XXX in the code.) > - Reject zero-length delimited identifiers, per Jacob's email. > - Properly initialize element->indexes in pgpa_trove_add_to_hash, per > Jacob'e email. > - Add gather((d d/d.d)) test case, per Jacob, and fix the related bug > in pgpa_identifier_matches_target, per Jacob's email. > - Add EXPLAIN SELECT 1 after various test cases in syntax.sql, to > improve test coverage, per analysis of why the existing test case > didn't catch a bug previously reported by Jacob. > - Added a dummy initialization to pgpa_collector.c to placate nervous > compilers, per discussion with Jacob. > > I think this mostly catches me up on responding to issues reported > here, although there is one thing reported to me off-list that I > haven't dealt with yet. If there's anything reported on thread that > isn't addressed here, let me know. > > Thanks, > > -- > Robert Haas > EDB: http://www.enterprisedb.com >
