pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedMemory leaks in BufFileOpenShared()
8+ messages / 3 participants
[nested] [flat]
* Memory leaks in BufFileOpenShared()
@ 2018-06-15 06:54 Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 8+ messages in thread
From: Antonin Houska @ 2018-06-15 06:54 UTC (permalink / raw)
To: pgsql-hackers
Memory is allocated twice for "file" and "files" variables. Possible fix:
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index d8a18dd3dc..00f61748b3 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
BufFile *
BufFileOpenShared(SharedFileSet *fileset, const char *name)
{
- BufFile *file = (BufFile *) palloc(sizeof(BufFile));
+ BufFile *file;
char segment_name[MAXPGPATH];
Size capacity = 16;
- File *files = palloc(sizeof(File) * capacity);
+ File *files;
int nfiles = 0;
file = (BufFile *) palloc(sizeof(BufFile));
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-15 07:20 Tatsuo Ishii <ishii@sraoss.co.jp>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 8+ messages in thread
From: Tatsuo Ishii @ 2018-06-15 07:20 UTC (permalink / raw)
To: ah@cybertec.at; +Cc: pgsql-hackers
> Memory is allocated twice for "file" and "files" variables. Possible fix:
>
> diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
> index d8a18dd3dc..00f61748b3 100644
> --- a/src/backend/storage/file/buffile.c
> +++ b/src/backend/storage/file/buffile.c
> @@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
> BufFile *
> BufFileOpenShared(SharedFileSet *fileset, const char *name)
> {
> - BufFile *file = (BufFile *) palloc(sizeof(BufFile));
> + BufFile *file;
> char segment_name[MAXPGPATH];
> Size capacity = 16;
> - File *files = palloc(sizeof(File) * capacity);
> + File *files;
> int nfiles = 0;
>
> file = (BufFile *) palloc(sizeof(BufFile));
Good catch. Thanks.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-15 07:59 Antonin Houska <ah@cybertec.at>
parent: Tatsuo Ishii <ishii@sraoss.co.jp>
0 siblings, 1 reply; 8+ messages in thread
From: Antonin Houska @ 2018-06-15 07:59 UTC (permalink / raw)
To: Tatsuo Ishii <ishii@sraoss.co.jp>; +Cc: pgsql-hackers
Now I see that BufFileCreateShared() has similar problem with file->name.
More generic problem I see is that the common initialization of BufFile is
repeated a few times. The attached patch tries to improve that (it also fixes
the duplicate allocation of file->name).
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> > Memory is allocated twice for "file" and "files" variables. Possible fix:
> >
> > diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
> > index d8a18dd3dc..00f61748b3 100644
> > --- a/src/backend/storage/file/buffile.c
> > +++ b/src/backend/storage/file/buffile.c
> > @@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
> > BufFile *
> > BufFileOpenShared(SharedFileSet *fileset, const char *name)
> > {
> > - BufFile *file = (BufFile *) palloc(sizeof(BufFile));
> > + BufFile *file;
> > char segment_name[MAXPGPATH];
> > Size capacity = 16;
> > - File *files = palloc(sizeof(File) * capacity);
> > + File *files;
> > int nfiles = 0;
> >
> > file = (BufFile *) palloc(sizeof(BufFile));
>
> Good catch. Thanks.
>
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp
>
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
Attachments:
[text/x-diff] buffile_refactor.patch (3.2K, ../../16139.1529049566@localhost/2-buffile_refactor.patch)
download | inline diff:
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index d8a18dd3dc..e345d008b5 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -99,6 +99,7 @@ struct BufFile
char buffer[BLCKSZ];
};
+static BufFile *makeBufFileCommon(int nfiles);
static BufFile *makeBufFile(File firstfile);
static void extendBufFile(BufFile *file);
static void BufFileLoadBuffer(BufFile *file);
@@ -106,21 +107,16 @@ static void BufFileDumpBuffer(BufFile *file);
static int BufFileFlush(BufFile *file);
static File MakeNewSharedSegment(BufFile *file, int segment);
-
/*
- * Create a BufFile given the first underlying physical file.
- * NOTE: caller must set isInterXact if appropriate.
+ * Create BufFile and perform the common initialization.
*/
static BufFile *
-makeBufFile(File firstfile)
+makeBufFileCommon(int nfiles)
{
BufFile *file = (BufFile *) palloc(sizeof(BufFile));
- file->numFiles = 1;
- file->files = (File *) palloc(sizeof(File));
- file->files[0] = firstfile;
- file->offsets = (off_t *) palloc(sizeof(off_t));
- file->offsets[0] = 0L;
+ file->numFiles = nfiles;
+ file->offsets = (off_t *) palloc0(sizeof(off_t) * nfiles);
file->isInterXact = false;
file->dirty = false;
file->resowner = CurrentResourceOwner;
@@ -128,6 +124,21 @@ makeBufFile(File firstfile)
file->curOffset = 0L;
file->pos = 0;
file->nbytes = 0;
+
+ return file;
+}
+
+/*
+ * Create a BufFile given the first underlying physical file.
+ * NOTE: caller must set isInterXact if appropriate.
+ */
+static BufFile *
+makeBufFile(File firstfile)
+{
+ BufFile *file = makeBufFileCommon(1);
+
+ file->files = (File *) palloc(sizeof(File));
+ file->files[0] = firstfile;
file->readOnly = false;
file->fileset = NULL;
file->name = NULL;
@@ -246,23 +257,12 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
{
BufFile *file;
- file = (BufFile *) palloc(sizeof(BufFile));
+ file = makeBufFileCommon(1);
file->fileset = fileset;
file->name = pstrdup(name);
- file->numFiles = 1;
file->files = (File *) palloc(sizeof(File));
file->files[0] = MakeNewSharedSegment(file, 0);
- file->offsets = (off_t *) palloc(sizeof(off_t));
- file->offsets[0] = 0L;
- file->isInterXact = false;
- file->dirty = false;
- file->resowner = CurrentResourceOwner;
- file->curFile = 0;
- file->curOffset = 0L;
- file->pos = 0;
- file->nbytes = 0;
file->readOnly = false;
- file->name = pstrdup(name);
return file;
}
@@ -286,4 +286,3 @@
- file = (BufFile *) palloc(sizeof(BufFile));
files = palloc(sizeof(File) * capacity);
/*
@@ -317,16 +316,8 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name)
(errcode_for_file_access(),
errmsg("could not open BufFile \"%s\"", name)));
- file->numFiles = nfiles;
+ file = makeBufFileCommon(nfiles);
file->files = files;
- file->offsets = (off_t *) palloc0(sizeof(off_t) * nfiles);
- file->isInterXact = false;
- file->dirty = false;
- file->resowner = CurrentResourceOwner; /* Unused, can't extend */
- file->curFile = 0;
- file->curOffset = 0L;
- file->pos = 0;
- file->nbytes = 0;
file->readOnly = true; /* Can't write to files opened this way */
file->fileset = fileset;
file->name = pstrdup(name);
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-15 08:22 Tatsuo Ishii <ishii@sraoss.co.jp>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 8+ messages in thread
From: Tatsuo Ishii @ 2018-06-15 08:22 UTC (permalink / raw)
To: ah@cybertec.at; +Cc: thomas.munro@enterprisedb.com; pgsql-hackers
> Now I see that BufFileCreateShared() has similar problem with file->name.
Right.
> More generic problem I see is that the common initialization of BufFile is
> repeated a few times. The attached patch tries to improve that (it also fixes
> the duplicate allocation of file->name).
The changes were made by this commit to add infrastructure for sharing
temporary files between backends, according to the author (Thomas
Munro).
https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
Your proposal looks reasonable but I would like to hear from Thomas's
opinion as well.
Thomas?
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-15 08:42 Antonin Houska <ah@cybertec.at>
parent: Tatsuo Ishii <ishii@sraoss.co.jp>
0 siblings, 1 reply; 8+ messages in thread
From: Antonin Houska @ 2018-06-15 08:42 UTC (permalink / raw)
To: Tatsuo Ishii <ishii@sraoss.co.jp>; +Cc: thomas.munro@enterprisedb.com; pgsql-hackers
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> The changes were made by this commit to add infrastructure for sharing
> temporary files between backends, according to the author (Thomas
> Munro).
>
> https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
>
> Your proposal looks reasonable but I would like to hear from Thomas's
> opinion as well.
o.k. He can actually have different feeling about details, e.g. if a new
function makeBufFileCommon() should be introduced or if makeBufFile() should
do the common settings. In the latter case, BufFileCreateTemp() would have to
do more work than it does now.
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-15 08:54 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Munro @ 2018-06-15 08:54 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Tatsuo Ishii <ishii@sraoss.co.jp>; pgsql-hackers
On Fri, Jun 15, 2018 at 8:42 PM, Antonin Houska <ah@cybertec.at> wrote:
> Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>
>> The changes were made by this commit to add infrastructure for sharing
>> temporary files between backends, according to the author (Thomas
>> Munro).
>>
>> https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
>>
>> Your proposal looks reasonable but I would like to hear from Thomas's
>> opinion as well.
>
> o.k. He can actually have different feeling about details, e.g. if a new
> function makeBufFileCommon() should be introduced or if makeBufFile() should
> do the common settings. In the latter case, BufFileCreateTemp() would have to
> do more work than it does now.
Thanks for finding these accidental duplications, and to Ishii-san for
committing the fix. Oops.
+1 for this refactoring.
--
Thomas Munro
http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-15 09:07 Tatsuo Ishii <ishii@sraoss.co.jp>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tatsuo Ishii @ 2018-06-15 09:07 UTC (permalink / raw)
To: thomas.munro@enterprisedb.com; +Cc: ah@cybertec.at; ishii@sraoss.co.jp; pgsql-hackers
Hi Thomas,
> On Fri, Jun 15, 2018 at 8:42 PM, Antonin Houska <ah@cybertec.at> wrote:
>> Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>>
>>> The changes were made by this commit to add infrastructure for sharing
>>> temporary files between backends, according to the author (Thomas
>>> Munro).
>>>
>>> https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
>>>
>>> Your proposal looks reasonable but I would like to hear from Thomas's
>>> opinion as well.
>>
>> o.k. He can actually have different feeling about details, e.g. if a new
>> function makeBufFileCommon() should be introduced or if makeBufFile() should
>> do the common settings. In the latter case, BufFileCreateTemp() would have to
>> do more work than it does now.
>
> Thanks for finding these accidental duplications, and to Ishii-san for
> committing the fix. Oops.
>
> +1 for this refactoring.
Thanks for confirming. I will go ahead commit/push the patch.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Memory leaks in BufFileOpenShared()
@ 2018-06-16 05:44 Tatsuo Ishii <ishii@sraoss.co.jp>
parent: Tatsuo Ishii <ishii@sraoss.co.jp>
0 siblings, 0 replies; 8+ messages in thread
From: Tatsuo Ishii @ 2018-06-16 05:44 UTC (permalink / raw)
To: thomas.munro@enterprisedb.com; ah@cybertec.at; +Cc: pgsql-hackers
>> Thanks for finding these accidental duplications, and to Ishii-san for
>> committing the fix. Oops.
>>
>> +1 for this refactoring.
>
> Thanks for confirming. I will go ahead commit/push the patch.
Done.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2018-06-16 05:44 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15 06:54 Memory leaks in BufFileOpenShared() Antonin Houska <ah@cybertec.at>
2018-06-15 07:20 ` Tatsuo Ishii <ishii@sraoss.co.jp>
2018-06-15 07:59 ` Antonin Houska <ah@cybertec.at>
2018-06-15 08:22 ` Tatsuo Ishii <ishii@sraoss.co.jp>
2018-06-15 08:42 ` Antonin Houska <ah@cybertec.at>
2018-06-15 08:54 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-06-15 09:07 ` Tatsuo Ishii <ishii@sraoss.co.jp>
2018-06-16 05:44 ` Tatsuo Ishii <ishii@sraoss.co.jp>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox