Is This a Memory Leak?

[Zhangwusheng]

Bug Report

In smgr.c:

void
smgr_init_standard(void)
{
	int i;
	for (i = 0; i < NSmgr; i++)
	{
		if (smgrsw[i].smgr_init)
			smgrsw[i].smgr_init();
	}
	mdinit();
}


void
smgrinit(void)
{
	if (smgr_init_hook)
		(*smgr_init_hook)();

	smgr_init_standard();

	/* register the shutdown proc */
	on_proc_exit(smgrshutdown, 0);
}

static const f_smgr smgrsw[] = {
	/* magnetic disk */
	{
		.smgr_init = mdinit,
		.smgr_shutdown = NULL,
		.smgr_close = mdclose,
		.smgr_create = mdcreate,
		.smgr_create_ao = mdcreate_ao,
		.smgr_exists = mdexists,
		.smgr_unlink = mdunlink,
		.smgr_extend = mdextend,
		.smgr_prefetch = mdprefetch,
		.smgr_read = mdread,
		.smgr_write = mdwrite,
		.smgr_nblocks = mdnblocks,
		.smgr_truncate = mdtruncate,
		.smgr_immedsync = mdimmedsync,
		.smgr_pre_ckpt = mdpreckpt,
		.smgr_sync = mdsync,
		.smgr_post_ckpt = mdpostckpt,
	},
};

static const int NSmgr = lengthof(smgrsw);

NSmgr=1 && smgrsw[0].smgr_init = mdinit.

so mdinit will be called twice.

and in mdinit MdCxt will be alloced tiwce?

void
mdinit(void)
{
	MdCxt = AllocSetContextCreate(TopMemoryContext,
								  "MdSmgr",
								  ALLOCSET_DEFAULT_MINSIZE,
								  ALLOCSET_DEFAULT_INITSIZE,
								  ALLOCSET_DEFAULT_MAXSIZE);

[SmartKeyerror]

The invocation of mdinit() in the last of function smgr_init_standard seems redundant.

@reshke I suppose we can remove it, what do you think?

[Aegeaner]

We can consider making magnetic disk's smgr_init() a no-op function, as mdinit() is always there. Any thoughts?

gpdb/src/backend/storage/smgr/smgr.c

Line 51 in 4d7c52a

.smgr_init = mdinit,

[SmartKeyerror]

Fixed by #15091, close this issue. Please feel free to reopen this if the PR doesn't fix this problem completely.

[kainwen]

Fixed by #15091, close this issue. Please feel free to reopen this if the PR doesn't fix this problem completely.

Need backport or not?

[Aegeaner]

Fixed by #15091, close this issue. Please feel free to reopen this if the PR doesn't fix this problem completely.

What if NSmgr>1? There will still be redudant mdinit() invocations as each smgr_init hook in the function array smgrsw was assigned by mdinit:

void
smgr_init_standard(void)
{
	int i;
	for (i = 0; i < NSmgr; i++)
	{
		if (smgrsw[i].smgr_init)
			smgrsw[i].smgr_init();
	}
}

static const f_smgr smgrsw[] = {
	/* magnetic disk */
	{
		.smgr_init = mdinit,
		……
	},
	/*
	 * Relation files that are different from heap, characterised by:
	 *     1. variable blocksize
	 *     2. block numbers are not consecutive
	 *     3. shared buffers are not used
	 * Append-optimized relation files currently fall in this category.
	 */
	{
		.smgr_init = mdinit,
                ……
	}
};

So there are still possible memory leaks.

[SmartKeyerror]

What if NSmgr>1?

The NSmgr will be 2 in the gpdb, since there have heap and AO table storage manager. For the design of storage manager, as many managers as there are, they should be invoked serval times.

[Aegeaner]

What if NSmgr>1?

The NSmgr will be 2 in the gpdb, since there have heap and AO table storage manager. For the design of storage manager, as many managers as there are, they should be invoked serval times.

So the MemoryContext should be allocated twice even with the same name?

[SmartKeyerror]

So the MemoryContext should be allocated twice even with the same name?

Maybe we should set the smgr_init of the storage manager of the AO table to NULL.

[ashwinstar]

So the MemoryContext should be allocated twice even with the same name?

Maybe we should set the smgr_init of the storage manager of the AO table to NULL.

or might be better to just make mdinit re-entrant and avoid leak memory

[ashwinstar]

So the MemoryContext should be allocated twice even with the same name?

Maybe we should set the smgr_init of the storage manager of the AO table to NULL.

or might be better to just make mdinit re-entrant and avoid leak memory

@SmartKeyerror this part is still remaining to be addressed from this issue.

[SmartKeyerror]

this part is still remaining to be addressed from this issue.

Did I miss something?