powa-team / powa-archivist

powa-archivist: the powa PostgreSQL extension
http://powa.readthedocs.io/
PostgreSQL License
51 stars 20 forks source link

memory leak #46

Closed RekGRpth closed 2 years ago

RekGRpth commented 2 years ago

Because powa background worker run constantly it has memory leak. I suggest exit background worker periodically (by config, default 1 hour for example), and postgres will restart it itself.

RekGRpth commented 2 years ago

see discussion https://www.postgresql.org/message-id/flat/16974-4f1a4803e727c3cf%40postgresql.org

rjuju commented 2 years ago

Hi.

I don't understand your point, it's not because a process runs constantly that it leaks memory.

Do you have an actual evidence of memory leaked by powa-archivist? The discussion you refer to is not about a leak, but about a process accessing buffers in shared memory, which is not memory leaked (or even allocated by the process).

You can refer to e.g. https://www.depesz.com/2012/06/09/how-much-ram-is-postgresql-using/ for instance for more details.

RekGRpth commented 2 years ago

after some days process powa eats 15.1GB (RES)

rjuju commented 2 years ago

That's surprising as there have never been any report of memory leak in the past.

Which version of powa-collector are you using, and how much RAM do you have on your server and what is the value of your shared_buffers setting? Can you paste the output of cat /proc/$pid/smaps where $pid is the pid of powa collector?

RekGRpth commented 2 years ago

Which version of powa-collector are you using

latest from git master

RekGRpth commented 2 years ago

how much RAM do you have on your server

64 GB

RekGRpth commented 2 years ago

what is the value of your shared_buffers setting

shared_buffers | 2097152 | 8kB | Resource Usage / Memory

RekGRpth commented 2 years ago

Can you paste the output of cat /proc/$pid/smaps where $pid is the pid of powa collector

attached cat.log

rjuju commented 2 years ago

The smaps output says:

7f0864531000-7f0c98d1b000 rw-s 00000000 00:01 74476366                   /dev/zero (deleted)
Size:           17637288 kB
KernelPageSize:        4 kB
MMUPageSize:           4 kB
Rss:            15761752 kB
Pss:             6379762 kB
Shared_Clean:          0 kB
Shared_Dirty:   15736680 kB
Private_Clean:         0 kB
Private_Dirty:     25072 kB
Referenced:     15424428 kB

So that's a 16.8GB shared memory segment, with 15.1 GB mapped in the powa-archivist process.

So as explained in the discussion you linked, this isn't a memory leak but an evidence that powa-archivist touched almost all postgres' shared buffers.

As a consequence, I see no need to make powa-archivist code more complex to handle frequent background worker restart.

RekGRpth commented 2 years ago

I suggest exit background worker periodically (by config, default 1 hour for example), and postgres will restart it itself.

for example, https://github.com/RekGRpth/powa-archivist/commit/d53ef46978904bbc51fe1b50be10420b2009981b

rjuju commented 2 years ago

This patch has multiple problems. The biggest I see, apart from the poor indentation that makes it extremely hard to read is that it doesn't preserve the regular snapshot frequency, so you can end up with two snapshots being done seconds apart. We definitely don't want to do that.

More generally, what are you trying to achieve here? As mentioned in that thread, you didn't show any evidence of memory leak. Also, I saw dozens of systems with powa being alive for months or years without any issue so I highly doubt that there is any memory leak. Similarly, I don't foresee any need to do any change in the bgworker code at any point in the future so I see no reason to think that any leak might be introduced.

As far as I can see the only problem you're solving is a broken monitoring system that doesn't know how to properly report memory usage. You can try hard to make sure that you never have any persistent connection on your database, but the only thing this will do is significantly decrease the overall performance of your system.