Re: [HACKERS] Logical replication ApplyContext bloat

[Stas Kelvich]

> On 9 May 2017, at 21:53, Peter Eisentraut  
> wrote:
> 
> On 5/8/17 11:26, Peter Eisentraut wrote:
>> I think it would make more sense to reset ApplyMessageContext in
>> apply_dispatch(), so that it is done consistently after every message
>> (as the name implies), not only some messages.
> 
> Committed that.
> 
>> Also, perhaps ApplyMessageContext should be a child of
>> TopTransactionContext.  (You have it as a child of ApplyContext, which
>> is under TopMemoryContext.)
> 
> Left that as is.

Thanks!

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Peter Eisentraut]

On 5/8/17 11:26, Peter Eisentraut wrote:
> I think it would make more sense to reset ApplyMessageContext in
> apply_dispatch(), so that it is done consistently after every message
> (as the name implies), not only some messages.

Committed that.

> Also, perhaps ApplyMessageContext should be a child of
> TopTransactionContext.  (You have it as a child of ApplyContext, which
> is under TopMemoryContext.)

Left that as is.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Peter Eisentraut]

On 5/5/17 03:09, Noah Misch wrote:
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

The discussion is still ongoing.  I will focus on it this week and
report back on Wednesday.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Peter Eisentraut]

I'm not sure about some of the details.

I think it would make more sense to reset ApplyMessageContext in
apply_dispatch(), so that it is done consistently after every message
(as the name implies), not only some messages.

Also, perhaps ApplyMessageContext should be a child of
TopTransactionContext.  (You have it as a child of ApplyContext, which
is under TopMemoryContext.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Noah Misch]

On Wed, May 03, 2017 at 12:02:41PM +0300, Stas Kelvich wrote:
> > On 20 Apr 2017, at 17:01, Dilip Kumar  wrote:
> > On Thu, Apr 20, 2017 at 7:04 PM, Stas Kelvich  
> > wrote:
> >> Thanks for noting.
> >> 
> >> Added short description of ApplyContext and ApplyMessageContext to README.
> > 
> > Typo
> > 
> > /analysys/analysis
> > 
> 
> Fixed, thanks.
> 
> Added this to open items list.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Stas Kelvich]

> On 20 Apr 2017, at 17:01, Dilip Kumar  wrote:
> 
> On Thu, Apr 20, 2017 at 7:04 PM, Stas Kelvich  
> wrote:
>> Thanks for noting.
>> 
>> Added short description of ApplyContext and ApplyMessageContext to README.
> 
> Typo
> 
> /analysys/analysis
> 

Fixed, thanks.

Added this to open items list.



apply_contexts_3.patch
Description: Binary data


Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Dilip Kumar]

On Thu, Apr 20, 2017 at 7:04 PM, Stas Kelvich  wrote:
> Thanks for noting.
>
> Added short description of ApplyContext and ApplyMessageContext to README.


+ApplyContext --- permanent during whole lifetime of apply worker. It is
+possible to use TopMemoryContext here as well, but for simplicity of
memory usage
+analysys we spin up different context.


Typo

/analysys/analysis


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Stas Kelvich]

> On 19 Apr 2017, at 16:07, Alvaro Herrera  wrote:
> 
> Stas Kelvich wrote:
> 
>> With patch MemoryContextStats() shows following hierarchy during slot 
>> operations in 
>> apply worker:
>> 
>> TopMemoryContext: 83824 total in 5 blocks; 9224 free (8 chunks); 74600 used
>>  ApplyContext: 8192 total in 1 blocks; 6520 free (4 chunks); 1672 used
>>ApplyMessageContext: 8192 total in 1 blocks; 6632 free (11 chunks); 1560 
>> used
>>  ExecutorState: 8192 total in 1 blocks; 7624 free (0 chunks); 568 used
>>ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
> 
> Please update src/backend/utils/mmgr/README to list the new contexts.


Thanks for noting.

Added short description of ApplyContext and ApplyMessageContext to README.



apply_contexts_2.patch
Description: Binary data




Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Alvaro Herrera]

Stas Kelvich wrote:

> With patch MemoryContextStats() shows following hierarchy during slot 
> operations in 
> apply worker:
> 
> TopMemoryContext: 83824 total in 5 blocks; 9224 free (8 chunks); 74600 used
>   ApplyContext: 8192 total in 1 blocks; 6520 free (4 chunks); 1672 used
> ApplyMessageContext: 8192 total in 1 blocks; 6632 free (11 chunks); 1560 
> used
>   ExecutorState: 8192 total in 1 blocks; 7624 free (0 chunks); 568 used
> ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used

Please update src/backend/utils/mmgr/README to list the new contexts.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Stas Kelvich]

> On 19 Apr 2017, at 14:30, Petr Jelinek  wrote:
> 
> On 19/04/17 12:46, Stas Kelvich wrote:
>> 
>> Right now ApplyContext cleaned after each transaction and by this patch I 
>> basically 
>> suggest to clean it after each command counter increment. 
>> 
>> So in your definitions that is ApplyMessageContext, most short-lived one. We 
>> can
>> rename now ApplyContext -> ApplyMessageContext to make that clear and avoid
>> possible name conflict when somebody will decide to keep something during 
>> whole
>> transaction or worker lifespan.
> 
> Yeah we can rename, and then rename the ApplyCacheContext to
> ApplyContext. That would leave the ApplyTransactionContext from Simon's
> proposal which is not really need it anywhere right now and I am not
> sure it will be needed given there is still TopTransactionContext
> present. If we really want ApplyTransactionContext we can probably just
> assign it to TopTransactionContext in ensure_transaction.
> 
>> 
>> P.S. It also seems to me now that resetting context in ensure_transaction() 
>> isn’t that
>> good idea, probably better just explicitly call reset at the end of each 
>> function involved.
>> 
> 
> Well it would definitely improve clarity.
> 

Okay, done.

With patch MemoryContextStats() shows following hierarchy during slot 
operations in 
apply worker:

TopMemoryContext: 83824 total in 5 blocks; 9224 free (8 chunks); 74600 used
  ApplyContext: 8192 total in 1 blocks; 6520 free (4 chunks); 1672 used
ApplyMessageContext: 8192 total in 1 blocks; 6632 free (11 chunks); 1560 
used
  ExecutorState: 8192 total in 1 blocks; 7624 free (0 chunks); 568 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used




apply_contexts.patch
Description: Binary data

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Petr Jelinek]

On 19/04/17 12:46, Stas Kelvich wrote:
> 
>> On 19 Apr 2017, at 13:34, Simon Riggs  wrote:
>>
>> On 19 April 2017 at 11:24, Petr Jelinek  wrote:
>>
>>> I'd still like you to add comment to the ApplyContext saying that it's
>>> cleaned after handling each message so nothing that needs to survive for
>>> example whole transaction can be allocated in it as that's not very well
>>> visible IMHO (and I know I will forget about it when writing next patch
>>> in that area).
>>
>> It would be better to invent the contexts we want now, so we get the
>> architecture right for future work. Otherwise we have problems with
>> "can't backpatch this fix because that infrastructure doesn't exist in
>> PG10" in a couple of years.
>>
>> So I suggest we have
>>
>> ApplyMessageContext - cleaned after every message
>> ApplyTransactionContext - cleaned at EOXact
>> ApplyContext - persistent
> 
> Right now ApplyContext cleaned after each transaction and by this patch I 
> basically 
> suggest to clean it after each command counter increment. 
> 
> So in your definitions that is ApplyMessageContext, most short-lived one. We 
> can
> rename now ApplyContext -> ApplyMessageContext to make that clear and avoid
> possible name conflict when somebody will decide to keep something during 
> whole
> transaction or worker lifespan.

Yeah we can rename, and then rename the ApplyCacheContext to
ApplyContext. That would leave the ApplyTransactionContext from Simon's
proposal which is not really need it anywhere right now and I am not
sure it will be needed given there is still TopTransactionContext
present. If we really want ApplyTransactionContext we can probably just
assign it to TopTransactionContext in ensure_transaction.

> 
> P.S. It also seems to me now that resetting context in ensure_transaction() 
> isn’t that
> good idea, probably better just explicitly call reset at the end of each 
> function involved.
> 

Well it would definitely improve clarity.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Stas Kelvich]

> On 19 Apr 2017, at 13:34, Simon Riggs  wrote:
> 
> On 19 April 2017 at 11:24, Petr Jelinek  wrote:
> 
>> I'd still like you to add comment to the ApplyContext saying that it's
>> cleaned after handling each message so nothing that needs to survive for
>> example whole transaction can be allocated in it as that's not very well
>> visible IMHO (and I know I will forget about it when writing next patch
>> in that area).
> 
> It would be better to invent the contexts we want now, so we get the
> architecture right for future work. Otherwise we have problems with
> "can't backpatch this fix because that infrastructure doesn't exist in
> PG10" in a couple of years.
> 
> So I suggest we have
> 
> ApplyMessageContext - cleaned after every message
> ApplyTransactionContext - cleaned at EOXact
> ApplyContext - persistent

Right now ApplyContext cleaned after each transaction and by this patch I 
basically 
suggest to clean it after each command counter increment. 

So in your definitions that is ApplyMessageContext, most short-lived one. We can
rename now ApplyContext -> ApplyMessageContext to make that clear and avoid
possible name conflict when somebody will decide to keep something during whole
transaction or worker lifespan.

P.S. It also seems to me now that resetting context in ensure_transaction() 
isn’t that
good idea, probably better just explicitly call reset at the end of each 
function involved.

> 
> -- 
> Simon Riggshttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Simon Riggs]

On 19 April 2017 at 11:24, Petr Jelinek  wrote:

> I'd still like you to add comment to the ApplyContext saying that it's
> cleaned after handling each message so nothing that needs to survive for
> example whole transaction can be allocated in it as that's not very well
> visible IMHO (and I know I will forget about it when writing next patch
> in that area).

It would be better to invent the contexts we want now, so we get the
architecture right for future work. Otherwise we have problems with
"can't backpatch this fix because that infrastructure doesn't exist in
PG10" in a couple of years.

So I suggest we have

ApplyMessageContext - cleaned after every message
ApplyTransactionContext - cleaned at EOXact
ApplyContext - persistent

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Petr Jelinek]

On 19/04/17 11:55, Stas Kelvich wrote:
> 
>> On 19 Apr 2017, at 12:37, Petr Jelinek  wrote:
>>
>> On 18/04/17 13:45, Stas Kelvich wrote:
>>> Hi,
>>>
>>> currently logical replication worker uses ApplyContext to decode received 
>>> data
>>> and that context is never freed during transaction processing. Hence if 
>>> publication
>>> side is performing something like 10M row inserts in single transaction, 
>>> then
>>> subscription worker will occupy more than 10G of ram (and can be killed by 
>>> OOM).
>>>
>>> Attached patch resets ApplyContext after each insert/update/delete/commit.
>>> I’ve tried to reset context only on each 100/1000/1 value of 
>>> CommandCounter,
>>> but didn’t spotted any measurable difference in speed.
>>>
>>> Problem spotted by Mikhail Shurutov.
>>>
>>
>> Hmm we also do replication protocol handling inside the ApplyContext
>> (LogicalRepApplyLoop), are you sure this change is safe in that regard?
> 
> Memory is bloated by logicalrep_read_* when palloc happens inside.
> I’ve inserted context reset in ensure_transaction() which is only called in 
> beginning
> of hande_insert/update/delete/commit when data from previous call of that
> function isn’t needed. So probably it is safe to clear memory there. At least
> i don’t see any state that can be accessed later in this functions. Do you
> have any specific concerns?
> 

I wanted to make sure you checked things that are happening outside of
the apply_handle_* stuff. I checked myself now, the allocations that
happen outside don't use postgres memory contexts (libpqrcv_receive) so
that should not be problem. Other allocations that I see in neighboring
code switch to permanent contexts anyway. So looks safe on first look
indeed. Eventually we'll want to cache some of the executor stuff so
this will be problem but not in v10.

I'd still like you to add comment to the ApplyContext saying that it's
cleaned after handling each message so nothing that needs to survive for
example whole transaction can be allocated in it as that's not very well
visible IMHO (and I know I will forget about it when writing next patch
in that area).

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Stas Kelvich]

> On 19 Apr 2017, at 12:37, Petr Jelinek  wrote:
> 
> On 18/04/17 13:45, Stas Kelvich wrote:
>> Hi,
>> 
>> currently logical replication worker uses ApplyContext to decode received 
>> data
>> and that context is never freed during transaction processing. Hence if 
>> publication
>> side is performing something like 10M row inserts in single transaction, then
>> subscription worker will occupy more than 10G of ram (and can be killed by 
>> OOM).
>> 
>> Attached patch resets ApplyContext after each insert/update/delete/commit.
>> I’ve tried to reset context only on each 100/1000/1 value of 
>> CommandCounter,
>> but didn’t spotted any measurable difference in speed.
>> 
>> Problem spotted by Mikhail Shurutov.
>> 
> 
> Hmm we also do replication protocol handling inside the ApplyContext
> (LogicalRepApplyLoop), are you sure this change is safe in that regard?

Memory is bloated by logicalrep_read_* when palloc happens inside.
I’ve inserted context reset in ensure_transaction() which is only called in 
beginning
of hande_insert/update/delete/commit when data from previous call of that
function isn’t needed. So probably it is safe to clear memory there. At least
i don’t see any state that can be accessed later in this functions. Do you
have any specific concerns?


Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Logical replication ApplyContext bloat

[Petr Jelinek]

On 18/04/17 13:45, Stas Kelvich wrote:
> Hi,
> 
> currently logical replication worker uses ApplyContext to decode received data
> and that context is never freed during transaction processing. Hence if 
> publication
> side is performing something like 10M row inserts in single transaction, then
> subscription worker will occupy more than 10G of ram (and can be killed by 
> OOM).
> 
> Attached patch resets ApplyContext after each insert/update/delete/commit.
> I’ve tried to reset context only on each 100/1000/1 value of 
> CommandCounter,
> but didn’t spotted any measurable difference in speed.
> 
> Problem spotted by Mikhail Shurutov.
> 

Hmm we also do replication protocol handling inside the ApplyContext
(LogicalRepApplyLoop), are you sure this change is safe in that regard?

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers