Re: Problem with accessing TOAST data in stored procedures

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.

Regards

Pavel