Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On 22.10.2019 20:22, Tomas Vondra wrote:
> On Tue, Oct 22, 2019 at 11:01:48AM +0530, Dilip Kumar wrote:
>> On Tue, Oct 22, 2019 at 10:46 AM Amit Kapila 
>> <amit.kapila16@gmail.com> wrote:
>>   In general, yours and Alexy's test results
>>> show that there is merit by having workers applying such transactions.
>>>   OTOH, as noted above [1], we are also worried about the performance
>>> of Rollbacks if we follow that approach.  I am not sure how much we
>>> need to worry about Rollabcks if commits are faster, but can we think
>>> of recording the changes in memory and only write to a file if the
>>> changes are above a certain threshold?  I think that might help saving
>>> I/O in many cases.  I am not very sure if we do that how much
>>> additional workers can help, but they might still help.  I think we
>>> need to do some tests and experiments to figure out what is the best
>>> approach?  What do you think?
>> I agree with the point.  I think we might need to do some small
>> changes and test to see what could be the best method to handle the
>> streamed changes at the subscriber end.
>>
>>>
>>> Tomas, Alexey, do you have any thoughts on this matter?  I think it is
>>> important that we figure out the way to proceed in this patch.
>>>
>>> [1] - 
>>> https://www.postgresql.org/message-id/b25ce80e-f536-78c8-d5c8-a5df3e230785%40postgrespro.ru
>>>
>>
>
> I think the patch should do the simplest thing possible, i.e. what it
> does today. Otherwise we'll never get it committed.
>

I have to agree with Tomas, that keeping things as simple as possible 
should be a main priority right now. Otherwise, the entire patch set 
will pass next release cycle without being committed at least partially. 
In the same time, it resolves important problem from my perspective. It 
moves I/O overhead from primary to replica using large transactions 
streaming, which is a nice to have feature I guess.

Later it would be possible to replace logical apply worker with 
bgworkers pool in a separated patch, if we decide that it is a viable 
solution. Anyway, regarding the Amit's questions:

- I doubt that maintaining a separate buffer on the apply side before 
spilling to disk would help enough. We already have ReorderBuffer with 
logical_work_mem limit, and if we exceeded that limit on the sender 
side, then most probably we exceed it on the applier side as well, 
excepting the case when this new buffer size will be significantly 
higher then logical_work_mem to keep multiple open xacts.

- I still think that we should optimize database for commits, not 
rollbacks. BGworkers pool is dramatically slower for rollbacks-only 
load, though being at least twice as faster for commits-only. I do not 
know how it will perform with real life load, but this drawback may be 
inappropriate for such a general purpose database like Postgres.

- Tomas' implementation of streaming with spilling does not have this 
bias between commits/aborts. However, it has a noticeable performance 
drop (~x5 slower compared with master [1]) for large transaction 
consisting of many small rows. Although it is not of an order of 
magnitude slower.

Another thing is it that about a year ago I have found some problems 
with MVCC/visibility and fixed them somehow [1]. If I get it correctly 
Tomas adapted some of those fixes into his patch set, but I think that 
this part should be reviewed carefully again. I would be glad to check 
it, but now I am a little bit confused with all the patch set variants 
in the thread. Which is the last one? Is it still dependent on 2pc decoding?

[1] 

https://www.postgresql.org/message-id/flat/40c38758-04b5-74f4-c963-cf300f9e5dff%40postgrespro.ru#98d06fefc88122385dacb2f03f7c30f7


Thanks for moving this patch forward!

-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company