Re: Support prepared statement invalidation when result types change

On Tue, Sep 12, 2023, at 10:17 AM, Jelte Fennema wrote:
> When running the Postgres JDBC tests with this patchset I found dumb
> mistake in my last patch where I didn't initialize the contents of
> orig_params correctly. This new patchset fixes that.
>
0001:

Don't you want to execute this code path only for EXECUTE command?
PORTAL_UTIL_SELECT includes other commands such as CALL, FETCH, SHOW, and
EXPLAIN. If so, check if commandTag is CMDTAG_EXECUTE.

Regarding tupDesc, you don't need to call FreeTupleDesc instead you can modify
PortalStart as

case PORTAL_UTIL_SELECT:

/*
* We don't set snapshot here, because PortalRunUtility will
* take care of it if needed.
*/
{
PlannedStmt *pstmt = PortalGetPrimaryStmt(portal);

Assert(pstmt->commandType == CMD_UTILITY);
/*
* tupDesc will be filled by FillPortalStore later because
* it might change due to replanning when ExecuteQuery calls
* GetCachedPlan.
*/
if (portal->commandTag != CMDTAG_EXECUTE)
portal->tupDesc = UtilityTupleDescriptor(pstmt->utilityStmt);
}

Regarding the commit message, ...if the the result... should be fixed. The
sentence "it's actually not needed..." could be "It doesn't need to be an error
as long as it sends the RowDescription...". The sentence "This patch starts to
allow a prepared ..." could be "This patch allows a prepared ...".

0002:

You should remove this comment because it refers to the option you are
removing.

- plan->cursor_options,
- false); /* not fixed result */
+ plan->cursor_options); /* not fixed result */

You should also remove the sentence that refers to fixed_result in
CompleteCachedPlan.

* cursor_options: options bitmask to pass to planner
* fixed_result: true to disallow future changes in query's result tupdesc
*/
void
CompleteCachedPlan(CachedPlanSource *plansource,
List *querytree_list,
MemoryContext querytree_context,

0003:

You should initialize the new parameters (orig_param_types and orig_num_params)
in CreateCachedPlan. One suggestion is to move the following code to
CompleteCachedPlan because plansource->param_types are assigned there.

@@ -108,6 +108,10 @@ PrepareQuery(ParseState *pstate, PrepareStmt *stmt,

argtypes[i++] = toid;
}
+
+ plansource->orig_num_params = nargs;
+ plansource->orig_param_types = MemoryContextAlloc(plansource->context, nargs * sizeof(Oid));
+ memcpy(plansource->orig_param_types, argtypes, nargs * sizeof(Oid));
}

This comment is confusing. Since the new function
(GetCachedPlanFromRevalidated) contains almost all code from GetCachedPlan, its
content is the same as the *previous* GetCachedPlan function. You could expand
this comment a bit to make it clear that it contains the logic to decide
between generic x custom plan. I don't like the function name but have only a
not-so-good suggestion: GetCachedPlanAfterRevalidate. I also don't like the
revalidationResult as a variable name. Why don't you keep qlist? Or use a name
near query-tree list (query_list? qtlist?). s/te caller/the caller/

+ * GetCachedPlanFromRevalidated: is the same as get GetCachedPlan, but requires
+ * te caller to first revalidate the query. This is needed for callers that
+ * need to use the revalidated plan to generate boundParams.
+ */
+CachedPlan *
+GetCachedPlanFromRevalidated(CachedPlanSource *plansource,
+ ParamListInfo boundParams,
+ ResourceOwner owner,
+ QueryEnvironment *queryEnv,
+ List *revalidationResult)

Are these names accurate? The original are the current ones; new ones are
"runtime" data. It would be good to explain why a new array is required.

Oid *param_types; /* array of parameter type OIDs, or NULL */
int num_params; /* length of param_types array */
+ Oid *orig_param_types; /* array of original parameter type OIDs,
+ * or NULL */
+ int orig_num_params; /* length of orig_param_types array */

You should expand the commit message a bit. Explain this feature. Inform the
behavior change.

--
Euler Taveira
EDB https://www.enterprisedb.com/