DBA-725 Fixed the issue with running explain plan on cursor #29
Conversation
| default=None, | ||
| ] | ||
| If there is no hypothetical index, you can pass an empty list.""", | ||
| default=[], |
There was a problem hiding this comment.
If we have a default value why instruct the LLM to pass in an empty list?
There was a problem hiding this comment.
Just to make sure that it doesn't HAVE TO send a list of hypothetical indexes.
| hypothetical_indexes: list[dict[str, Any]] | None = Field( | ||
| description="""Optional list of hypothetical indexes to simulate. Each index must be a dictionary with these keys: | ||
| hypothetical_indexes: list[dict[str, Any]] = Field( | ||
| description="""A list of hypothetical indexes to simulate. Each index must be a dictionary with these keys: |
There was a problem hiding this comment.
See comment below, I think I could keep it optional.
There was a problem hiding this comment.
That was the main fix to not have it as optional.
src/postgres_mcp/server.py
Outdated
| @@ -363,7 +364,7 @@ async def explain_query( | |||
| result: ExplainPlanArtifact | ErrorResult | None = None | |||
|
|
|||
| # If hypothetical indexes are specified, check for HypoPG extension | |||
| if hypothetical_indexes: | |||
| if len(hypothetical_indexes) > 0: | |||
There was a problem hiding this comment.
I'm not certain whether it is still possible to pass in None. The type annotation does not apply at runtime, and I'm not certain how the Field handles None inputs.
Suggest `if hypothetical_indexes and len(hypothetical_indexes) > 0:
There was a problem hiding this comment.
That was the main problem: with having None in the parameter, the MCP server had a hard time figuring out the required type and as failing in Cursor. Added this check.
|
In general, we should probably be thinking in terms of Postel's Law for tools. It's not always the right approach but for calls from LLMs it probably is: "be conservative in what you send, and liberal in what you accept" |
No description provided.