On Fri, Oct 21, 2022 at 09:02:04PM +0530, Bharath Rupireddy wrote:
> After all, that is what is being discussed here; what if palloc down
> below fails and they're not reset to NULL after MemoryContextReset()?
It does not seem to matter much to me for that, so left these as
proposed.
> On Fri, Oc
On Fri, Oct 21, 2022 at 11:58 AM Michael Paquier wrote:
>
> To be exact, it seems to me that tablespace_map and backup_state
> should be reset before deleting backupcontext, but the reset of
> backupcontext should happen after the fact.
>
> + backup_state = NULL;
> tablespace_map = N
On Fri, Oct 21, 2022 at 2:18 AM Michael Paquier wrote:
> AFAIK, one of the callbacks associated to a memory context could
> fail, see comments before MemoryContextCallResetCallbacks() in
> MemoryContextDelete(). I agree that it should not matter here, but I
> think that it is better to reset the
At Thu, 20 Oct 2022 19:47:07 +0200, Alvaro Herrera
wrote in
> I agree that's a good idea, and the patch looks good to me, but I don't
> think asserting that they are null afterwards is useful.
+1 for this direction. And the patch is fine to me.
> oldcontext = MemoryContextSwitchTo(backu
On Fri, Oct 21, 2022 at 11:34:27AM +0530, Bharath Rupireddy wrote:
> On Fri, Oct 21, 2022 at 12:21 AM Robert Haas wrote:
>> On Thu, Oct 20, 2022 at 1:35 PM Bharath Rupireddy
>> wrote:
>>> I think elsewhere in the code we reset dangling pointers either ways -
>>> before or after deleting/resettin
On Thu, Oct 20, 2022 at 02:51:21PM -0400, Robert Haas wrote:
> On Thu, Oct 20, 2022 at 1:35 PM Bharath Rupireddy
> wrote:
>> I think elsewhere in the code we reset dangling pointers either ways -
>> before or after deleting/resetting memory context. But placing them
>> before would give us extra s
On Thu, Oct 20, 2022 at 11:17 PM Alvaro Herrera wrote:
>
> On 2022-Oct-20, Bharath Rupireddy wrote:
>
> > I think elsewhere in the code we reset dangling pointers either ways -
> > before or after deleting/resetting memory context. But placing them
> > before would give us extra safety in case mem
On Thu, Oct 20, 2022 at 1:35 PM Bharath Rupireddy
wrote:
> I think elsewhere in the code we reset dangling pointers either ways -
> before or after deleting/resetting memory context. But placing them
> before would give us extra safety in case memory context
> deletion/reset fails. Not sure what's
On 2022-Oct-20, Bharath Rupireddy wrote:
> I think elsewhere in the code we reset dangling pointers either ways -
> before or after deleting/resetting memory context. But placing them
> before would give us extra safety in case memory context
> deletion/reset fails. Not sure what's the best way. H
On Thu, Oct 20, 2022 at 9:48 PM Robert Haas wrote:
>
> On Thu, Oct 20, 2022 at 6:47 AM Bharath Rupireddy
> wrote:
> > I tried implementing this, please see the attached v7 patch.
>
> I haven't checked this in detail but it looks much more reasonable in
> terms of code footprint. However, we shoul
On Thu, Oct 20, 2022 at 6:47 AM Bharath Rupireddy
wrote:
> I tried implementing this, please see the attached v7 patch.
I haven't checked this in detail but it looks much more reasonable in
terms of code footprint. However, we should, I think, set backup_state
= NULL and tablespace_map = NULL bef
On Wed, Oct 19, 2022 at 9:23 PM Bharath Rupireddy
wrote:
>
> On Wed, Oct 19, 2022 at 8:10 PM Robert Haas wrote:
>
> > One option is to just have do_pg_start_backup() blow
> > away any old memory context before it allocates any new memory, and
> > forget about releasing anything in PostgresMain().
On Wed, Oct 19, 2022 at 8:10 PM Robert Haas wrote:
>
> Well this still touches postgres.c. And I still think it's an awful
> lot of machinery for a pretty trivial problem. As a practical matter,
> nobody is going to open a connection and sit there and try to start a
> backup over and over again on
On Wed, Oct 19, 2022 at 3:04 AM Bharath Rupireddy
wrote:
> > Echoing with what I mentioned upthread in [1], I don't quite
> > understand why this patch needs to touch basebackup.c, walsender.c
> > and postgres.c. In the case of a replication command processed by a
> > WAL sender, memory allocatio
On Mon, Oct 17, 2022 at 1:09 PM Michael Paquier wrote:
>
> On Fri, Oct 14, 2022 at 09:56:31PM +, Cary Huang wrote:
> > I applied your v5 patch on the current master and run valgrind on it
> > while doing a basebackup with simulated error. No memory leak
> > related to backup is observed. Regre
On Fri, Oct 14, 2022 at 09:56:31PM +, Cary Huang wrote:
> I applied your v5 patch on the current master and run valgrind on it
> while doing a basebackup with simulated error. No memory leak
> related to backup is observed. Regression is also passing.
Echoing with what I mentioned upthread in
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation:not tested
Hello
I applied your v5 patch on the current master and run valgrind
On Thu, Sep 29, 2022 at 10:38 PM Bharath Rupireddy
wrote:
>
> Please review the v4 patch.
I used valgrind for testing. Without patch, there's an obvious memory
leak [1], with patch no memory leak.
I used ALLOCSET_START_SMALL_SIZES instead of ALLOCSET_DEFAULT_SIZES
for backup memory context so th
On Thu, Sep 29, 2022 at 7:05 PM Robert Haas wrote:
>
> On Thu, Sep 29, 2022 at 4:29 AM Bharath Rupireddy
> wrote:
> > Please review the attached v3 patch.
>
> template1=# select * from pg_backup_start('sdgkljsdgkjdsg', true);
> pg_backup_start
> -
> 0/228
> (1 row)
>
> templ
On Thu, Sep 29, 2022 at 4:29 AM Bharath Rupireddy
wrote:
> Please review the attached v3 patch.
template1=# select * from pg_backup_start('sdgkljsdgkjdsg', true);
pg_backup_start
-
0/228
(1 row)
template1=# select 1/0;
ERROR: division by zero
template1=# select * from pg_b
On Wed, Sep 28, 2022 at 8:46 PM Robert Haas wrote:
>
> I feel like we ought to be trying to tie
> the cleanup into WalSndErrorCleanup() or WalSndResourceCleanup() based
> on having the memory context that we ought to be blowing away stored
> in a global variable, rather than using a try/catch bloc
On Wed, Sep 28, 2022 at 5:30 AM Bharath Rupireddy
wrote:
> I'm attaching the v2 patch designed as described above. Please review it.
>
> I've added an entry in CF - https://commitfest.postgresql.org/40/3915/
This looks odd to me. In the case of a regular backend, the
sigsetjmp() handler in src/ba
On Wed, Sep 28, 2022 at 10:09 AM Bharath Rupireddy
wrote:
>
> Here's what I think - for backup functions, we
> can have the new memory context child of TopMemoryContext and for
> perform_base_backup(), we can have the memory context child of
> CurrentMemoryContext. With PG_TRY()-PG_FINALLY()-PG_EN
At Wed, 28 Sep 2022 13:16:36 +0900, Michael Paquier wrote
in
> On Tue, Sep 27, 2022 at 05:32:26PM +0900, Kyotaro Horiguchi wrote:
> > At Tue, 27 Sep 2022 11:33:56 +0530, Bharath Rupireddy
> > wrote in
> > > On Mon, Sep 26, 2022 at 7:34 PM Tom Lane wrote:
> > > > This ... seems like inventing
On Wed, Sep 28, 2022 at 10:19 AM Tom Lane wrote:
>
> Bharath Rupireddy writes:
> > I had the same opinion. Here's what I think - for backup functions, we
> > can have the new memory context child of TopMemoryContext and for
> > perform_base_backup(), we can have the memory context child of
> > Cu
Bharath Rupireddy writes:
> I had the same opinion. Here's what I think - for backup functions, we
> can have the new memory context child of TopMemoryContext and for
> perform_base_backup(), we can have the memory context child of
> CurrentMemoryContext. With PG_TRY()-PG_FINALLY()-PG_END_TRY(), w
On Wed, Sep 28, 2022 at 9:46 AM Michael Paquier wrote:
>
> On Tue, Sep 27, 2022 at 05:32:26PM +0900, Kyotaro Horiguchi wrote:
> > At Tue, 27 Sep 2022 11:33:56 +0530, Bharath Rupireddy
> > wrote in
> > > On Mon, Sep 26, 2022 at 7:34 PM Tom Lane wrote:
> > > > This ... seems like inventing your o
On Tue, Sep 27, 2022 at 05:32:26PM +0900, Kyotaro Horiguchi wrote:
> At Tue, 27 Sep 2022 11:33:56 +0530, Bharath Rupireddy
> wrote in
> > On Mon, Sep 26, 2022 at 7:34 PM Tom Lane wrote:
> > > This ... seems like inventing your own shape of wheel. The
> > > normal mechanism for preventing this
At Tue, 27 Sep 2022 11:33:56 +0530, Bharath Rupireddy
wrote in
> On Mon, Sep 26, 2022 at 7:34 PM Tom Lane wrote:
> > This ... seems like inventing your own shape of wheel. The
> > normal mechanism for preventing this type of leak is to put the
> > allocations in a memory context that can be re
On Mon, Sep 26, 2022 at 7:34 PM Tom Lane wrote:
>
> > I'm proposing a patch that leverages the error callback mechanism and
> > memory context.
>
> This ... seems like inventing your own shape of wheel. The
> normal mechanism for preventing this type of leak is to put the
> allocations in a memor
Bharath Rupireddy writes:
> Postgres currently can leak memory if a failure occurs during base
> backup in do_pg_backup_start() or do_pg_backup_stop() or
> perform_base_backup(). The palloc'd memory such as backup_state or
> tablespace_map in xlogfuncs.c or basebackup.c or tablespaceinfo or the
>
Hi,
Postgres currently can leak memory if a failure occurs during base
backup in do_pg_backup_start() or do_pg_backup_stop() or
perform_base_backup(). The palloc'd memory such as backup_state or
tablespace_map in xlogfuncs.c or basebackup.c or tablespaceinfo or the
memory that gets allocated by bb
32 matches
Mail list logo
