Обсуждение: pgsql: Fix plpgsql to re-look-up composite type names at need.

Поиск
Список
Период
Сортировка

pgsql: Fix plpgsql to re-look-up composite type names at need.

От
Tom Lane
Дата:
Fix plpgsql to re-look-up composite type names at need.

Commit 4b93f5799 rearranged things in plpgsql to make it cope better with
composite types changing underneath it intra-session.  However, I failed to
consider the case of a composite type being dropped and recreated entirely.
In my defense, the previous coding didn't consider that possibility at all
either --- but it would accidentally work so long as you didn't change the
type's field list, because the built-at-compile-time list of component
variables would then still match the type's new definition.  The new
coding, however, occasionally tries to re-look-up the type by OID, and
then fails to find the dropped type.

To fix this, we need to save the TypeName struct, and then redo the type
OID lookup from that.  Of course that's expensive, so we don't want to do
it every time we need the type OID.  This can be fixed in the same way that
4b93f5799 dealt with changes to composite types' definitions: keep an eye
on the type's typcache entry to see if its tupledesc has been invalidated.
(Perhaps, at some point, this mechanism should be generalized so it can
work for non-composite types too; but for now, plpgsql only tries to
cope with intra-session redefinitions of composites.)

I'm slightly hesitant to back-patch this into v11, because it changes
the contents of struct PLpgSQL_type as well as the signature of
plpgsql_build_datatype(), so in principle it could break code that is
poking into the innards of plpgsql.  However, the only popular extension
of that ilk is pldebugger, and it doesn't seem to be affected.  Since
this is a regression for people who were relying on the old behavior,
it seems worth taking the small risk of causing compatibility issues.

Per bug #15913 from Daniel Fiori.  Back-patch to v11 where 4b93f5799
came in.

Discussion: https://postgr.es/m/15913-a7e112e16dedcffc@postgresql.org

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/fe9b7b2fe5973309c0a5f7d9240dde91aeeb94aa

Modified Files
--------------
src/backend/utils/cache/typcache.c             |  10 ++
src/pl/plpgsql/src/expected/plpgsql_record.out |  29 ++++++
src/pl/plpgsql/src/pl_comp.c                   | 132 +++++++++++++++++++------
src/pl/plpgsql/src/pl_exec.c                   |  79 ++++++++++++++-
src/pl/plpgsql/src/pl_gram.y                   |  22 +++--
src/pl/plpgsql/src/plpgsql.h                   |  14 ++-
src/pl/plpgsql/src/sql/plpgsql_record.sql      |  16 +++
7 files changed, 265 insertions(+), 37 deletions(-)


Re: pgsql: Fix plpgsql to re-look-up composite type names at need.

От
Pavel Stehule
Дата:
Hi

čt 15. 8. 2019 v 21:22 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:


I'm slightly hesitant to back-patch this into v11, because it changes
the contents of struct PLpgSQL_type as well as the signature of
plpgsql_build_datatype(), so in principle it could break code that is
poking into the innards of plpgsql.  However, the only popular extension
of that ilk is pldebugger, and it doesn't seem to be affected.  Since
this is a regression for people who were relying on the old behavior,
it seems worth taking the small risk of causing compatibility issues.



this change breaks compilation plpgsql_check extension.


Backpatching changed plpgsql_build_datatype was not good idea. Now, I have not a idea how to ensure compilation on 11 and higher

Can you add some symbol to source code, so four parameters of plpgsql_build_datatype should be used?

Regards

Pavel

Re: pgsql: Fix plpgsql to re-look-up composite type names at need.

От
Tom Lane
Дата:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> čt 15. 8. 2019 v 21:22 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> I'm slightly hesitant to back-patch this into v11, because it changes
>> the contents of struct PLpgSQL_type as well as the signature of
>> plpgsql_build_datatype(), so in principle it could break code that is
>> poking into the innards of plpgsql.  However, the only popular extension
>> of that ilk is pldebugger, and it doesn't seem to be affected.  Since
>> this is a regression for people who were relying on the old behavior,
>> it seems worth taking the small risk of causing compatibility issues.

> this change breaks compilation plpgsql_check extension.

plpgsql_check?  Why would that need to call plpgsql_build_datatype?

> Backpatching changed plpgsql_build_datatype was not good idea. Now, I have
> not a idea how to ensure compilation on 11 and higher
> Can you add some symbol to source code, so four parameters of
> plpgsql_build_datatype should be used?

That wouldn't help you much, because it would only tell you which
version you were compiling against, not what you were executing in.

You might be best off to put your own copy of the new version of
plpgsql_build_datatype into the v11 extension, and call that all
the time.  When executing in an older server, it would generate
structs with some extra fields that plpgsql wouldn't use, but
that shouldn't hurt anything.  (You could likely get away with
setting the new fields to nulls/zeroes even for composite types,
shortening your extra code a bit.  They'd only matter for
compiled trees that survive across queries, which I imagine
plpgsql_check doesn't need to deal with.)

            regards, tom lane



Re: pgsql: Fix plpgsql to re-look-up composite type names at need.

От
Pavel Stehule
Дата:


pá 16. 8. 2019 v 17:06 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
Pavel Stehule <pavel.stehule@gmail.com> writes:
> čt 15. 8. 2019 v 21:22 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> I'm slightly hesitant to back-patch this into v11, because it changes
>> the contents of struct PLpgSQL_type as well as the signature of
>> plpgsql_build_datatype(), so in principle it could break code that is
>> poking into the innards of plpgsql.  However, the only popular extension
>> of that ilk is pldebugger, and it doesn't seem to be affected.  Since
>> this is a regression for people who were relying on the old behavior,
>> it seems worth taking the small risk of causing compatibility issues.

> this change breaks compilation plpgsql_check extension.

plpgsql_check?  Why would that need to call plpgsql_build_datatype?

It is used for check of expressions related to plpgsql CASE stmt. CASE variable type is computed dynamically from CASE expr.


> Backpatching changed plpgsql_build_datatype was not good idea. Now, I have
> not a idea how to ensure compilation on 11 and higher
> Can you add some symbol to source code, so four parameters of
> plpgsql_build_datatype should be used?

That wouldn't help you much, because it would only tell you which
version you were compiling against, not what you were executing in.

You might be best off to put your own copy of the new version of
plpgsql_build_datatype into the v11 extension, and call that all
the time.  When executing in an older server, it would generate
structs with some extra fields that plpgsql wouldn't use, but
that shouldn't hurt anything.  (You could likely get away with
setting the new fields to nulls/zeroes even for composite types,
shortening your extra code a bit.  They'd only matter for
compiled trees that survive across queries, which I imagine
plpgsql_check doesn't need to deal with.)

I though more time about it and problem is only if somebody will try compile plpgsql_check against stable Postgres 11. I can use constraint

#ifdef PG_VERSION_NUM > 110005

Almost all people uses plpgsql_check against some packaged postgres, so this constraint should to work.

I can ignore already released PostgreSQL 12, because it should not be used in production mode.

Pavel
 

                        regards, tom lane