Re: pgsql: Drop unnamed portal immediately after execution to completion

> Thinking a bit outside the box.. I was wondering about a plan D (plan
> A is what's on HEAD, plan B is copying around the query string, plan C
> this Vfd approach) where we shut down the executor when we have
> finished the execution of an unnamed portal, with something like the
> attached based on a more aggressive PortalCleanup().

I am not sure I like this idea as-is, because besides that fact
that it's still a wire level change, it's not safe at all to
re-enter exec_execute_message after you just cleaned the
portal but did not drop it.

if (!PortalIsValid(portal)) will tell you the portal is still valid, but its
resources, like queryDesc and others are no longer available.
You can actually see what happens there with this handy
extended.py that communicates directly over the wire-protocol.
See the "back-to-back execute" print.

It results in
```
TRAP: failed Assert("queryDesc || portal->holdStore"), File:
"../src/backend/tcop/pquery.c", Line: 875, PID: 32358
0 postgres 0x00000001028ce52c
ExceptionalCondition + 108
1 postgres 0x00000001027a2470 PortalRunMulti + 0
2 postgres 0x00000001027a1fc0 PortalRun + 492
3 postgres 0x000000010279ff54 PostgresMain + 8896
4 postgres 0x0000000102799bbc BackendInitialize + 0
5 postgres 0x00000001026f1264
postmaster_child_launch + 364
6 postgres 0x00000001026f588c ServerLoop + 5840
7 postgres 0x00000001026f3800
InitProcessGlobals + 0
8 postgres 0x0000000102641564 help + 0
9 dyld 0x00000001930d6b98 start + 6076
```

This idea could perhaps work, but needs more thought.

> Perhaps the best answer for now is to revert and continue the
> discussion for this cycle as there seem to be little love for the
> current HEAD's solution with the protocol change.
>
> If folks have more ideas or input, please feel free.

That is probably the best idea now.

> This is new, attaching the information to a Vfd in fd.c. Not sure
> that adding this information to this structure is a good concept.
> This layer of the code has no idea of query strings currently, so that
> feels a bit like a layer violation.

Thanks for having a look! FWIW I found a way for Plan C to work without
including tcoprot into fd, see attached.

There's a new field in that structure indeed, but maybe not that far
fetched, it's the query that triggered the creation of the file.

To Michael's point, this looks like a layering violation. Besides,
I think this will double log, although I did not test, because you log
the statement once when closing the temp and once when logging
the STATEMENT.

One thing I am still not so sure about is we currently say that things
like the query_string will out live the portal, so I am still not clear what
is the risk of copying the query_string to debug_query_string during
PortalDrop?

```
/*
* We don't have to copy anything into the portal, because everything
* we are passing here is in MessageContext or the
* per_parsetree_context, and so will outlive the portal anyway.
*/
PortalDefineQuery(portal,
NULL,
query_string,
commandTag,
plantree_list,
NULL);
```

--
Sami Imseih
Amazon Web Services (AWS)