Re: Problem with accessing TOAST data in stored procedures

čt 18. 2. 2021 v 18:10 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> čt 18. 2. 2021 v 16:01 odesílatel Konstantin Knizhnik <
> k(dot)knizhnik(at)postgrespro(dot)ru> napsal:
>
>>
>>
>> On 19.08.2020 22:20, Pavel Stehule wrote:
>>
>>
>>
>> st 19. 8. 2020 v 20:59 odesílatel Konstantin Knizhnik <
>> k(dot)knizhnik(at)postgrespro(dot)ru> napsal:
>>
>>>
>>>
>>> On 19.08.2020 21:50, Pavel Stehule wrote:
>>>
>>> Hi
>>>
>>> st 19. 8. 2020 v 19:22 odesílatel Konstantin Knizhnik <
>>> k(dot)knizhnik(at)postgrespro(dot)ru> napsal:
>>>
>>>> Hi hackers,
>>>>
>>>> More than month ago I have sent bug report to pgsql-bugs:
>>>>
>>>>
>>>> https://www.postgresql.org/message-id/flat/5d335911-fb25-60cd-4aa7-a5bd0954aea0%40postgrespro.ru
>>>>
>>>> with the proposed patch but have not received any response.
>>>>
>>>> I wonder if there is some other way to fix this issue and does somebody
>>>> working on it.
>>>> While the added check itself is trivial (just one line) the total patch
>>>> is not so small because I have added walker for
>>>> plpgsql statements tree. It is not strictly needed in this case (it is
>>>> possible to find some other way to determine that stored procedure
>>>> contains transaction control statements), but I hope such walker may be
>>>> useful in other cases.
>>>>
>>>> In any case, I will be glad to receive any response,
>>>> because this problem was reported by one of our customers and we need
>>>> to
>>>> provide some fix.
>>>> It is better to include it in vanilla, rather than in our pgpro-ee fork.
>>>>
>>>> If it is desirable, I can add this patch to commitfest.
>>>>
>>>
>>>
>>> I don't like this design. It is not effective to repeat the walker for
>>> every execution. Introducing a walker just for this case looks like
>>> overengineering.
>>> Personally I am not sure if a walker for plpgsql is a good idea (I
>>> thought about it more times, when I wrote plpgsql_check). But anyway -
>>> there should be good reason for introducing the walker and clean use case.
>>>
>>> If you want to introduce stmt walker, then it should be a separate patch
>>> with some benefit on plpgsql environment length.
>>>
>>> If you think that plpgsql statement walker is not needed, then I do not
>>> insist.
>>> Are you going to commit your version of the patch?
>>>
>>
>> I am afraid so it needs significantly much more work :(. My version is
>> correct just for the case that you describe, but it doesn't fix the
>> possibility of the end of the transaction inside the nested CALL.
>>
>> Some like
>>
>> DO $$ DECLARE v_r record; BEGIN FOR v_r in SELECT data FROM toasted LOOP
>> INSERT INTO toasted(data) VALUES(v_r.data) CALL check_and_commit();END
>> LOOP;END;$$;
>>
>> Probably my patch (or your patch) will fix on 99%, but still there will
>> be a possibility of this issue. It is very similar to fixing releasing expr
>> plans inside CALL statements. Current design of CALL statement is ugly
>> workaround - it is slow, and there is brutal memory leak. Fixing memory
>> leak is not hard. Fixing every time replaning (and sometimes useless) needs
>> depper fix. Please check patch
>> https://www.postgresql.org/message-id/attachment/112489/plpgsql-stmt_call-fix-2.patch
>> Maybe this mechanism can be used for a clean fix of the problem mentioned
>> in this thread.
>>
>>
>> Sorry for delay with answer.
>> Today we have received another bug report from the client.
>> And now as you warned, there was no direct call of COMMIT/ROLLBACK
>> statements in stored procedures, but instead of it it is calling some other
>> pprocedures
>> which I suspect contains some transaction control statements.
>>
>> I looked at the plpgsql-stmt_call-fix-2.patch
>> <https://www.postgresql.org/message-id/attachment/112489/plpgsql-stmt_call-fix-2.patch>
>> It invalidates prepared plan in case of nested procedure call.
>> But here invalidation approach will not work. We have already prefetched
>> rows and to access them we need snapshot.
>> We can not restore the same snapshot after CALL - it will be not correct.
>> In case of ATX (autonomous transactions supported by PgPro) we really
>> save/restore context after ATX. But transaction control semantic in
>> procedures is different:
>> we commit current transaction and start new one.
>>
>> So I didn't find better solution than just slightly extend you patch and
>> consider any procedures containing CALLs as potentially performing
>> transaction control.
>> I updated version of your patch.
>> What do you think about it?
>>
>
> This has a negative impact on performance - and a lot of users use
> procedures without transaction control. So it doesn't look like a good
> solution.
>
> I am more concentrated on the Pg 14 release, where the work with SPI is
> redesigned, and I hope so this issue is fixed there. For older releases, I
> don't know. Is this issue related to Postgres or it is related to PgPro
> only? If it is related to community pg, then we should fix and we should
> accept not too good performance, because there is no better non invasive
> solution. If it is PgPro issue (because there are ATX support) you can fix
> it (or you can try backport the patch
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ee895a655ce4341546facd6f23e3e8f2931b96bf
> ). You have more possibilities on PgPro code base.
>

I am sorry, maybe my reply was not (is not) correct - this issue was
reported four months ago, and now I think more about your words about ATX,
and I have no idea, how much it is related to community pg or to pgpro.

I am sure so implementation of autonomous transactions is pretty hard, but
the described issue is related to PgPro implementation of ATX, and then it
should be fixed there. Disabling prefetching doesn't look like a good idea.
You try to fix the result, not the source of the problem - but I have not
any idea, what is possible and what not, because I don't know how PgPro ATX
is implemented.

> Regards
>
> Pavel
>
>
>
>
>
>> --
>> Konstantin Knizhnik
>> Postgres Professional: http://www.postgrespro.com
>> The Russian Postgres Company
>>
>>