Re: Support prepared statement invalidation when result types change

On 25.08.2023 8:57 PM, Jelte Fennema wrote:
> The cached plan for a prepared statements can get invalidated when DDL
> changes the tables used in the query, or when search_path changes. When
> this happens the prepared statement can still be executed, but it will
> be replanned in the new context. This means that the prepared statement
> will do something different e.g. in case of search_path changes it will
> select data from a completely different table. This won't throw an
> error, because it is considered the responsibility of the operator and
> query writers that the query will still do the intended thing.
>
> However, we would throw an error if the the result of the query is of a
> different type than it was before:
> ERROR: cached plan must not change result type
>
> This requirement was not documented anywhere and it
> can thus be a surprising error to hit. But it's actually not needed for
> this to be an error, as long as we send the correct RowDescription there
> does not have to be a problem for clients when the result types or
> column counts change.
>
> This patch starts to allow a prepared statement to continue to work even
> when the result type changes.
>
> Without this change all clients that automatically prepare queries as a
> performance optimization will need to handle or avoid the error somehow,
> often resulting in deallocating and re-preparing queries when its
> usually not necessary. With this change connection poolers can also
> safely prepare the same query only once on a connection and share this
> one prepared query across clients that prepared that exact same query.
>
> Some relevant previous discussions:
> [1]: https://www.postgresql.org/message-id/flat/CAB%3DJe-GQOW7kU9Hn3AqP1vhaZg_wE9Lz6F4jSp-7cm9_M6DyVA%40mail.gmail.com
> [2]: https://stackoverflow.com/questions/2783813/postgres-error-cached-plan-must-not-change-result-type
> [3]: https://stackoverflow.com/questions/42119365/how-to-avoid-cached-plan-must-not-change-result-type-error
> [4]: https://github.com/pgjdbc/pgjdbc/pull/451
> [5]: https://github.com/pgbouncer/pgbouncer/pull/845#discussion_r1305295551
> [6]: https://github.com/jackc/pgx/issues/927
> [7]: https://elixirforum.com/t/postgrex-errors-with-cached-plan-must-not-change-result-type-during-migration/51235/2
> [8]: https://github.com/rails/rails/issues/12330

The following assignment of format is not corrects:

     /* Do nothing if portal won't return tuples */
     if (portal->tupDesc == NULL)
+    {
+        /*
+         * For SELECT like queries we delay filling in the tupDesc
until after
+         * PortalRunUtility, because we don't know what rows an EXECUTE
+         * command will return. Because we delay setting tupDesc, we
also need
+         * to delay setting formats. We do this in a pretty hacky way, by
+         * temporarily setting the portal formats to the passed in formats.
+         * Then once we fill in tupDesc, we call PortalSetResultFormat
again
+         * with portal->formats to fill in the final formats value.
+         */
+        if (portal->strategy == PORTAL_UTIL_SELECT)
+            portal->formats = formats;
         return;

because it is create in other memory context:

postgres.c:
    /* Done storing stuff in portal's context */
    MemoryContextSwitchTo(oldContext);
    ...
     /* Get the result format codes */
    numRFormats = pq_getmsgint(input_message, 2);
    if (numRFormats > 0)
    {
        rformats = palloc_array(int16, numRFormats);
        for (int i = 0; i < numRFormats; i++)
            rformats[i] = pq_getmsgint(input_message, 2);
    }

It has to be copied as below:

    portal->formats = (int16 *)
        MemoryContextAlloc(portal->portalContext,
                           natts * sizeof(int16));
        memcpy(portal->formats, formats, natts * sizeof(int16));

or alternatively  MemoryContextSwitchTo(oldContext) should be moved
after initialization of rformat