Re: OOM on EXPLAIN with lots of nodes

On 01/13/2015 07:24 PM, Tom Lane wrote:
> It is, but FDWs are not at risk here: they merely reference ExplainStates
> that were allocated by core backend code. So as long as we add the new
> field at the end it's not a problem for them. Problematic usage would be
> like what auto_explain does:
>
> ExplainState es;
>
> ExplainInitState(&es);
> ...
>
> In hindsight, that's a bad API and we should change it to something like
>
> ExplainState *es = NewExplainState();
>
> so that the sizeof the struct isn't embedded in extension code. But we
> definitely can't do that in back branches.

Actually, it would make sense to do exactly that, to break any
extensions that are doing the unsafe thing in an obvious way. The
downside would be that an extension using the new API would then not
work on an old server.

We could repurpose one of the existing fields in ExplainState to point
to another struct that contains more fields. Something like this:

*** a/src/include/commands/explain.h
--- b/src/include/commands/explain.h
***************
*** 40,48 ****
List *rtable; /* range table */
List *rtable_names; /* alias names for RTEs */
int indent; /* current indentation level */
! List *grouping_stack; /* format-specific grouping state */
} ExplainState;

/* Hook for plugins to get control in ExplainOneQuery() */
typedef void (*ExplainOneQuery_hook_type) (Query *query,
IntoClause *into,
--- 40,55 ----
List *rtable; /* range table */
List *rtable_names; /* alias names for RTEs */
int indent; /* current indentation level */
! ExplainStateExtraFields *extra; /* more fields */
} ExplainState;

+ typedef struct ExplainStateExtraFields
+ {
+ List *grouping_stack; /* format-specific grouping state */
+ ...
+ }
+

That's pretty ugly, but it would work even if there are ExplainState
structs embeded in extensions. As long as they don't try to look at the
grouping_stack field; I think that's fairly safe assumption.

But do we really need to backpatch any of this?

- Heikki