pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedReport a potential memory leak in setup_config()
15+ messages / 4 participants
[nested] [flat]
* Report a potential memory leak in setup_config()
@ 2022-02-15 01:49 wliang@stu.xidian.edu.cn
0 siblings, 2 replies; 15+ messages in thread
From: wliang@stu.xidian.edu.cn @ 2022-02-15 01:49 UTC (permalink / raw)
To: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hiall,
IfindapotentialmemoryleakinPostgresSQL14.1, whichisinthefunctionsetup_config (./src/bin/initdb/initdb.c).
Specifically, atline1095, functionpretty_wal_size() iscalled, whichallocatesachunkofmemorybyusing pg_mallocandreturnsit. However, thereturnchunkisdirectlypassedtosnprintfasits3rdparameter. Asaresult, thereisamemoryleak.
1053staticvoid
1054setup_config(void)
1055 {
...
1094 snprintf(repltok, sizeof(repltok), "min_wal_size = %s",
1095 pretty_wal_size(DEFAULT_MIN_WAL_SEGS));
...
1347 }
1036staticchar *
1037pretty_wal_size(intsegment_count)
1038 {
1039 int sz = wal_segment_size_mb * segment_count;
1040 char *result = pg_malloc(14);
1041
1042 if ((sz % 1024) == 0)
1043 snprintf(result, 14, "%dGB", sz / 1024);
1044 else
1045 snprintf(result, 14, "%dMB", sz);
1046
1047 returnresult;
1048 }
Webelievewecanfixthisproblembyaddingavariabletoreceivethereturnofpretty_wal_size() andemployingpg_free() tofreetheleakedmemory.
I'mlookingforwardtoyourconfirmation.
Best,
Wentao
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-15 12:10 Daniel Gustafsson <daniel@yesql.se>
parent: wliang@stu.xidian.edu.cn
1 sibling, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2022-02-15 12:10 UTC (permalink / raw)
To: wliang@stu.xidian.edu.cn; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
> On 15 Feb 2022, at 02:49, wliang@stu.xidian.edu.cn wrote:
> Specifically, at line 1095, function pretty_wal_size() is called, which allocates a chunk of memory by using pg_malloc and returns it. However, the return chunk is directly passed to snprintf as its 3rd parameter. As a result, there is a memory leak.
PostgreSQL isn't all too concerned about small static leaks in short lived
programs, like initdb. Memory will be freed shortly when the program exits.
Complicating the code to save 28 bytes seems hardly worth it, but if you feel
strongly about it I suggest proposing a patch to fix it.
--
Daniel Gustafsson https://vmware.com/
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-15 16:33 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-02-15 16:33 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
> On 15 Feb 2022, at 02:49, wliang@stu.xidian.edu.cn wrote:
>> Specifically, at line 1095, function pretty_wal_size() is called, which allocates a chunk of memory by using pg_malloc and returns it. However, the return chunk is directly passed to snprintf as its 3rd parameter. As a result, there is a memory leak.
> PostgreSQL isn't all too concerned about small static leaks in short lived
> programs, like initdb. Memory will be freed shortly when the program exits.
> Complicating the code to save 28 bytes seems hardly worth it, but if you feel
> strongly about it I suggest proposing a patch to fix it.
Yeah. A quick run of initdb under valgrind reports
==3051337== LEAK SUMMARY:
==3051337== definitely lost: 893,446 bytes in 47 blocks
==3051337== indirectly lost: 4,490 bytes in 51 blocks
==3051337== possibly lost: 0 bytes in 0 blocks
==3051337== still reachable: 2,403 bytes in 22 blocks
==3051337== suppressed: 0 bytes in 0 blocks
It might be worth trying to knock that down a bit, but I wouldn't
start with a one-time leak of 28 bytes. It looks like the biggest
offender is that we don't bother trying to reclaim the lines
malloc'd by readfile() and replace_token(). Fixing that is *maybe*
worth the trouble, but TBH no one has complained about initdb's
memory consumption.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 01:29 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-02-16 01:29 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
On 2022-02-15 11:33:26 -0500, Tom Lane wrote:
> It might be worth trying to knock that down a bit, but I wouldn't
> start with a one-time leak of 28 bytes. It looks like the biggest
> offender is that we don't bother trying to reclaim the lines
> malloc'd by readfile() and replace_token(). Fixing that is *maybe*
> worth the trouble, but TBH no one has complained about initdb's
> memory consumption.
This reminds me: I recently noticed that the replace_token() stuff is a good
portion of the userspace time on windows. IIRC in debug builds only, I assume
because of some slow checking in the debug C runtime. It doesn't matter too
much overall, because the really expensive bit is the filesystem operations
being very slow (*).
It's a bit insane that we allocate the lines[] quite so many times, when
processing the same file. ~12k lines in postgres.bki * 8 replace_token() in
bootstrap_template1() starts to add up. The replacement patterns either are
compile time constants which we just should handle in genbki.pl, or have
exactly 1 replacement....
Greetings,
Andres Freund
*it looks like ntfs does *synchronous* journalling on every metadata
operation, which seems odd for an extremely widely used filesystem in
2022. But what do I know.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 01:45 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-02-16 01:45 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Andres Freund <andres@anarazel.de> writes:
> On 2022-02-15 11:33:26 -0500, Tom Lane wrote:
>> It might be worth trying to knock that down a bit, but I wouldn't
>> start with a one-time leak of 28 bytes. It looks like the biggest
>> offender is that we don't bother trying to reclaim the lines
>> malloc'd by readfile() and replace_token(). Fixing that is *maybe*
>> worth the trouble, but TBH no one has complained about initdb's
>> memory consumption.
> It's a bit insane that we allocate the lines[] quite so many times, when
> processing the same file.
Yeah, I noticed that --- why don't we reuse the array of pointers?
Not sure it'd save much compared to freeing the strings, but it is
mighty low-hanging fruit.
> The replacement patterns either are
> compile time constants which we just should handle in genbki.pl, or have
> exactly 1 replacement....
Mmm, really? I thought most of them were data that we don't know
until initdb runs. Anything that really is known at build time,
sure, genbki.pl ought to take care of.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 02:21 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-02-16 02:21 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
On 2022-02-15 20:45:46 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-02-15 11:33:26 -0500, Tom Lane wrote:
> >> It might be worth trying to knock that down a bit, but I wouldn't
> >> start with a one-time leak of 28 bytes. It looks like the biggest
> >> offender is that we don't bother trying to reclaim the lines
> >> malloc'd by readfile() and replace_token(). Fixing that is *maybe*
> >> worth the trouble, but TBH no one has complained about initdb's
> >> memory consumption.
>
> > It's a bit insane that we allocate the lines[] quite so many times, when
> > processing the same file.
>
> Yeah, I noticed that --- why don't we reuse the array of pointers?
> Not sure it'd save much compared to freeing the strings, but it is
> mighty low-hanging fruit.
The number of replacements is low enough that the memory for the changed
strings themselves doesn't actually matter much, I think. replace_token()
doesn't allocate memory for unchanged strings...
I think we'd see memory usage of quite different proportions otherwise - my
postgres.bki is 900kB. 9 copies of that would start to add up...
for k in NAMEDATALEN SIZEOF_POINTER ALIGNOF_POINTER FLOAT8PASSBYVAL POSTGRES ENCODING LC_COLLATE LC_CTYPE;do echo $k: $(grep -c $k ./src/backend/catalog/postgres.bki);done
NAMEDATALEN: 5
SIZEOF_POINTER: 2
ALIGNOF_POINTER: 2
FLOAT8PASSBYVAL: 8
POSTGRES: 1
ENCODING: 1
LC_COLLATE: 1
LC_CTYPE: 1
> > The replacement patterns either are
> > compile time constants which we just should handle in genbki.pl, or have
> > exactly 1 replacement....
>
> Mmm, really? I thought most of them were data that we don't know
> until initdb runs. Anything that really is known at build time,
> sure, genbki.pl ought to take care of.
Only POSTGRES, ENCODING, LC_COLLATE, LC_CTYPE of the above list are runtime
variable, right? And those just affect two rows in total...
I was pondering initdb's design a bunch lately. So I started a -hackers thread:
https://postgr.es/m/20220216021219.ygzrtb3hd5bn7olz%40alap3.anarazel.de
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 02:25 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-02-16 02:25 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Andres Freund <andres@anarazel.de> writes:
> I was pondering initdb's design a bunch lately. So I started a -hackers thread:
> https://postgr.es/m/20220216021219.ygzrtb3hd5bn7olz%40alap3.anarazel.de
Yeah, I read that. I think making the backend read these files directly
makes a ton of sense, so I'm no longer very excited about improving the
performance or memory consumption of the replace_token stuff. I think
we should put this thread on hold until someone does that, and then
maybe take another look to see if initdb still leaks anything worth
worrying about.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 02:38 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-02-16 02:38 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
I wrote:
> Yeah, I read that. I think making the backend read these files directly
> makes a ton of sense, so I'm no longer very excited about improving the
> performance or memory consumption of the replace_token stuff.
Actually, wait a second. I grant your point about postgres.bki, but
a whole lot of the replace_token calls in initdb are messing with
the configuration files, which (a) is something I don't see changing,
and (b) pretty much none of that could be pushed back to build time.
So even though the config files are a lot smaller than postgres.bki,
maybe there's still a point in polishing replace_token a bit.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 04:10 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-02-16 04:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
On 2022-02-15 21:38:11 -0500, Tom Lane wrote:
> I wrote:
> > Yeah, I read that. I think making the backend read these files directly
> > makes a ton of sense, so I'm no longer very excited about improving the
> > performance or memory consumption of the replace_token stuff.
>
> Actually, wait a second. I grant your point about postgres.bki, but
> a whole lot of the replace_token calls in initdb are messing with
> the configuration files, which (a) is something I don't see changing,
> and (b) pretty much none of that could be pushed back to build time.
> So even though the config files are a lot smaller than postgres.bki,
> maybe there's still a point in polishing replace_token a bit.
Agreed. postgresql.conf.sample isn't that small either and I can't see that
moving to the backend.
When I'd looked at this last I apparently (looking at git stash, I ended up
discarding this) decided that the best way would be to change replace_token's
API to one that just processes one line at a time, with an outer loop that
processes all tokens in a line. I'm not really sure why anymore.
Definitely not finished and contains a lot of timing cruft... But it does
markedly reduce the memory usage, despite only converting bootstrap.bki
replacement:
without REPLACE_FAST:
==2217045== HEAP SUMMARY:
==2217045== in use at exit: 894,012 bytes in 119 blocks
==2217045== total heap usage: 26,284 allocs, 26,165 frees, 3,127,341 bytes allocated
==2217045==
==2217045== LEAK SUMMARY:
==2217045== definitely lost: 887,136 bytes in 46 blocks
==2217045== indirectly lost: 4,453 bytes in 50 blocks
==2217045== possibly lost: 0 bytes in 0 blocks
==2217045== still reachable: 2,423 bytes in 23 blocks
==2217045== suppressed: 0 bytes in 0 blocks
==2217045== Rerun with --leak-check=full to see details of leaked memory
with:
==2215712== HEAP SUMMARY:
==2215712== in use at exit: 120,490 bytes in 90 blocks
==2215712== total heap usage: 26,257 allocs, 26,167 frees, 2,393,822 bytes allocated
==2215712==
==2215712== LEAK SUMMARY:
==2215712== definitely lost: 116,096 bytes in 38 blocks
==2215712== indirectly lost: 1,971 bytes in 29 blocks
==2215712== possibly lost: 0 bytes in 0 blocks
==2215712== still reachable: 2,423 bytes in 23 blocks
==2215712== suppressed: 0 bytes in 0 blocks
==2215712== Rerun with --leak-check=full to see details of leaked memory
I think I was a bit underwhelmed by the wins on the runtime side on linux. A
profile without children looks quite different before / after. But initdb's
own cpu time is just such a small part of the overall runtime...
profile before:
- 46.91% 0.00% initdb initdb [.] main
- main
- 42.99% initialize_data_directory
- 25.45% bootstrap_template1
+ 10.59% replace_token
+ 7.98% readfile
+ 3.47% __GI___libc_free (inlined)
+ 2.63% __GI__IO_fputs (inlined)
+ 0.14% pclose_check
+ 0.14% progress
profile after:
- 42.17% 0.00% initdb initdb [.] main
- main
- 37.98% initialize_data_directory
- 20.32% bootstrap_template1
+ 9.11% readfile
+ 6.37% replace_one_token
+ 3.16% __GI__IO_fputs (inlined)
+ 0.79% __GI___libc_free (inlined)
+ 0.17% check_ok
+ 0.14% progress
+ 0.10% pclose_check
after both the biggest remaining cpu user is the strstr().
Both profiles are with the change included in the patch to avoid fflush()ing
for each line. That's noticeable as it turns out.
Anyway, I think this angle is kind of moot if we move this stuff to the
backend, and likely to don't perform replacement on any of the big files.
Greetings,
Andres Freund
Attachments:
initdb-stuff.diff (text/x-diff, 10.2K) download
diff --git i/src/bin/initdb/initdb.c w/src/bin/initdb/initdb.c
index 97f15971e2b..68bce3773b0 100644
--- i/src/bin/initdb/initdb.c
+++ w/src/bin/initdb/initdb.c
@@ -76,6 +76,10 @@
#include "getopt_long.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
+#include "portability/instr_time.h"
+
+
+//#define REPLACE_FAST
/* Ideally this would be in a .h file, but it hardly seems worth the trouble */
@@ -174,6 +178,8 @@ static bool caught_signal = false;
static bool output_failed = false;
static int output_errno = 0;
static char *pgdata_native;
+static instr_time initdb_start_time;
+static instr_time last_ts;
/* defaults */
static int n_connections = 10;
@@ -278,6 +284,8 @@ static void check_locale_name(int category, const char *locale,
static bool check_locale_encoding(const char *locale, int encoding);
static void setlocales(void);
static void usage(const char *progname);
+static void progress(const char *point);
+
void setup_pgdata(void);
void setup_bin_paths(const char *argv0);
void setup_data_file_paths(void);
@@ -309,10 +317,16 @@ do { \
#define PG_CMD_PUTS(line) \
do { \
- if (fputs(line, cmdfd) < 0 || fflush(cmdfd) < 0) \
- output_failed = true, output_errno = errno; \
+ if (fputs(line, cmdfd) < 0) \
+ output_failed = true, output_errno = errno; \
} while (0)
+#define PG_CMD_FLUSH \
+do { \
+ if (fflush(cmdfd) < 0) \
+ output_failed = true, output_errno = errno; \
+ } while (0)
+
#define PG_CMD_PRINTF(fmt, ...) \
do { \
if (fprintf(cmdfd, fmt, __VA_ARGS__) < 0 || fflush(cmdfd) < 0) \
@@ -362,6 +376,51 @@ escape_quotes_bki(const char *src)
return result;
}
+#ifdef REPLACE_FAST
+
+static void
+replace_one_token(char **linep, const char *token, const char *replacement)
+{
+ char *line = *linep;
+ char *where;
+ int toklen,
+ replen,
+ diff;
+ int linelen;
+ int pre;
+
+ /* nothing to do if NULL or no change needed */
+ if ((where = strstr(line, token)) == NULL)
+ return;
+
+ //fprintf(stderr, "replacing %s with %s, before: %s", token, replacement, line);
+
+ toklen = strlen(token);
+ replen = strlen(replacement);
+ diff = replen - toklen;
+
+ linelen = strlen(line);
+
+ pre = where - line;
+
+ /* ensure there's enough space */
+ if (diff > 0)
+ {
+ *linep = repalloc(line, linelen + diff + 1);
+ line = *linep;
+ where = line + pre;
+ }
+
+ /* move bit after token aside */
+ memmove(where + replen, where + toklen, linelen - pre - toklen + 1);
+
+ /* and copy in the the new contents */
+ memcpy(where, replacement, replen);
+
+ //fprintf(stderr, "after: %s\n", line);
+}
+#endif
+
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
@@ -1357,7 +1416,17 @@ bootstrap_template1(void)
char **line;
char **bki_lines;
char headerline[MAXPGPATH];
+#ifdef REPLACE_FAST
+ char namedatalen_buf[64];
+ char sizeof_pointer_buf[64];
+ int numlines;
+ const char *username_esc;
+ const char *encoding;
+ const char *lc_collate_esc;
+ const char *lc_ctype_esc;
+#else
char buf[64];
+#endif
printf(_("running bootstrap script ... "));
fflush(stdout);
@@ -1379,8 +1448,43 @@ bootstrap_template1(void)
exit(1);
}
+ progress("bootstrap read");
+
/* Substitute for various symbols used in the BKI file */
+#ifdef REPLACE_FAST
+ numlines = 0;
+ for (int i = 0; bki_lines[i]; i++)
+ numlines++;
+
+ sprintf(namedatalen_buf, "%d", NAMEDATALEN);
+ sprintf(sizeof_pointer_buf, "%d", (int) sizeof(Pointer));
+ username_esc = escape_quotes_bki(username);
+ encoding = encodingid_to_string(encodingid);
+ lc_collate_esc = escape_quotes_bki(lc_collate);
+ lc_ctype_esc = escape_quotes_bki(lc_ctype);
+
+ for (int i = 0; i < numlines; i++)
+ {
+ char **line = &bki_lines[i];
+
+ replace_one_token(line, "NAMEDATALEN", namedatalen_buf);
+ replace_one_token(line, "SIZEOF_POINTER", sizeof_pointer_buf);
+
+ replace_one_token(line, "ALIGNOF_POINTER",
+ (sizeof(Pointer) == 4) ? "i" : "d");
+
+ replace_one_token(line, "FLOAT8PASSBYVAL",
+ FLOAT8PASSBYVAL ? "true" : "false");
+
+ replace_one_token(line, "POSTGRES", username_esc);
+ replace_one_token(line, "ENCODING", encoding);
+ replace_one_token(line, "LC_COLLATE", lc_collate_esc);
+ replace_one_token(line, "LC_CTYPE", lc_ctype_esc);
+ }
+
+#else
+
sprintf(buf, "%d", NAMEDATALEN);
bki_lines = replace_token(bki_lines, "NAMEDATALEN", buf);
@@ -1404,6 +1508,8 @@ bootstrap_template1(void)
bki_lines = replace_token(bki_lines, "LC_CTYPE",
escape_quotes_bki(lc_ctype));
+#endif
+ progress("bootstrap replace");
/* Also ensure backend isn't confused by this environment var: */
unsetenv("PGCLIENTENCODING");
@@ -1419,14 +1525,24 @@ bootstrap_template1(void)
PG_CMD_OPEN;
+ progress("bootstrap start");
+
for (line = bki_lines; *line != NULL; line++)
{
PG_CMD_PUTS(*line);
free(*line);
}
+ progress("bootstrap write");
+
+ PG_CMD_FLUSH;
+
+ progress("bootstrap flush");
+
PG_CMD_CLOSE;
+ progress("bootstrap shutdown");
+
free(bki_lines);
check_ok();
@@ -1450,6 +1566,7 @@ setup_auth(FILE *cmdfd)
for (line = pg_authid_setup; *line != NULL; line++)
PG_CMD_PUTS(*line);
+ PG_CMD_FLUSH;
if (superuser_password)
PG_CMD_PRINTF("ALTER USER \"%s\" WITH PASSWORD E'%s';\n\n",
@@ -1537,6 +1654,7 @@ setup_depend(FILE *cmdfd)
for (line = pg_depend_setup; *line != NULL; line++)
PG_CMD_PUTS(*line);
+ PG_CMD_FLUSH;
}
/*
@@ -1556,6 +1674,7 @@ setup_run_file(FILE *cmdfd, const char *filename)
}
PG_CMD_PUTS("\n\n");
+ PG_CMD_FLUSH;
free(lines);
}
@@ -1579,6 +1698,7 @@ setup_description(FILE *cmdfd)
" AND NOT EXISTS (SELECT 1 FROM pg_description "
" WHERE objoid = o_oid AND classoid = 'pg_operator'::regclass"
" AND description LIKE 'deprecated%');\n\n");
+ PG_CMD_FLUSH;
}
/*
@@ -1598,6 +1718,7 @@ setup_collation(FILE *cmdfd)
/* Now import all collations we can find in the operating system */
PG_CMD_PUTS("SELECT pg_import_system_collations('pg_catalog');\n\n");
+ PG_CMD_FLUSH;
}
/*
@@ -1763,6 +1884,7 @@ setup_privileges(FILE *cmdfd)
escape_quotes(username));
for (line = priv_lines; *line != NULL; line++)
PG_CMD_PUTS(*line);
+ PG_CMD_FLUSH;
}
/*
@@ -1820,6 +1942,7 @@ static void
load_plpgsql(FILE *cmdfd)
{
PG_CMD_PUTS("CREATE EXTENSION plpgsql;\n\n");
+ PG_CMD_FLUSH;
}
/*
@@ -1830,6 +1953,7 @@ vacuum_db(FILE *cmdfd)
{
/* Run analyze before VACUUM so the statistics are frozen. */
PG_CMD_PUTS("ANALYZE;\n\nVACUUM FREEZE;\n\n");
+ PG_CMD_FLUSH;
}
/*
@@ -1888,6 +2012,7 @@ make_template0(FILE *cmdfd)
for (line = template0_setup; *line; line++)
PG_CMD_PUTS(*line);
+ PG_CMD_FLUSH;
}
/*
@@ -1907,6 +2032,7 @@ make_postgres(FILE *cmdfd)
for (line = postgres_setup; *line; line++)
PG_CMD_PUTS(*line);
+ PG_CMD_FLUSH;
}
/*
@@ -2771,6 +2897,8 @@ initialize_data_directory(void)
printf(_("creating subdirectories ... "));
fflush(stdout);
+ progress("subdirs");
+
for (i = 0; i < lengthof(subdirs); i++)
{
char *path;
@@ -2802,9 +2930,13 @@ initialize_data_directory(void)
/* Now create all the text config files */
setup_config();
+ progress("config");
+
/* Bootstrap template1 */
bootstrap_template1();
+ progress("bootstrap");
+
/*
* Make the per-database PG_VERSION for template1 only after init'ing it
*/
@@ -2814,7 +2946,7 @@ initialize_data_directory(void)
* Create the stuff we don't need to use bootstrap mode for, using a
* backend running in simple standalone mode.
*/
- fputs(_("performing post-bootstrap initialization ... "), stdout);
+ fputs(_("performing post-bootstrap initialization ... \n"), stdout);
fflush(stdout);
snprintf(cmd, sizeof(cmd),
@@ -2826,11 +2958,16 @@ initialize_data_directory(void)
setup_auth(cmdfd);
+ progress("auth");
+
setup_run_file(cmdfd, system_constraints_file);
+ progress("constraints");
setup_run_file(cmdfd, system_functions_file);
+ progress("functions");
setup_depend(cmdfd);
+ progress("depend");
/*
* Note that no objects created after setup_depend() will be "pinned".
@@ -2838,30 +2975,56 @@ initialize_data_directory(void)
*/
setup_run_file(cmdfd, system_views_file);
+ progress("views");
setup_description(cmdfd);
+ progress("desc");
setup_collation(cmdfd);
+ progress("coll");
setup_run_file(cmdfd, dictionary_file);
+ progress("dict");
setup_privileges(cmdfd);
+ progress("priv");
setup_schema(cmdfd);
+ progress("schema");
load_plpgsql(cmdfd);
+ progress("plpgsql");
vacuum_db(cmdfd);
+ progress("vacuum");
make_template0(cmdfd);
+ progress("db dict");
make_postgres(cmdfd);
+ progress("db postgres");
PG_CMD_CLOSE;
check_ok();
+
+ progress("shutdown");
}
+static void
+progress(const char *point)
+{
+ instr_time now;
+
+ INSTR_TIME_SET_CURRENT(now);
+
+ fprintf(stderr, "%.3f ms + %.3f ms: %s\n",
+ (INSTR_TIME_GET_DOUBLE(now) - INSTR_TIME_GET_DOUBLE(initdb_start_time)) * 1000,
+ (INSTR_TIME_GET_DOUBLE(now) - INSTR_TIME_GET_DOUBLE(last_ts)) * 1000,
+ point);
+
+ last_ts = now;
+}
int
main(int argc, char *argv[])
@@ -2912,6 +3075,9 @@ main(int argc, char *argv[])
PQExpBuffer start_db_cmd;
char pg_ctl_path[MAXPGPATH];
+ INSTR_TIME_SET_CURRENT(initdb_start_time);
+ last_ts = initdb_start_time;
+
/*
* Ensure that buffering behavior of stdout matches what it is in
* interactive usage (at least on most platforms). This prevents
@@ -3100,6 +3266,8 @@ main(int argc, char *argv[])
exit(1);
}
+ progress("start");
+
check_authmethod_unspecified(&authmethodlocal);
check_authmethod_unspecified(&authmethodhost);
@@ -3133,8 +3301,12 @@ main(int argc, char *argv[])
get_restricted_token();
+ progress("checks");
+
setup_pgdata();
+ progress("setup_pgdata");
+
setup_bin_paths(argv[0]);
effective_user = get_id();
@@ -3160,6 +3332,8 @@ main(int argc, char *argv[])
setup_text_search();
+ progress("setup_more");
+
printf("\n");
if (data_checksums)
@@ -3174,12 +3348,16 @@ main(int argc, char *argv[])
initialize_data_directory();
+ progress("initialize data dir complete");
+
if (do_sync)
{
fputs(_("syncing data to disk ... "), stdout);
fflush(stdout);
fsync_pgdata(pg_data, PG_VERSION_NUM);
check_ok();
+
+ progress("fsync");
}
else
printf(_("\nSync to disk skipped.\nThe data directory might become corrupt if the operating system crashes.\n"));
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 04:30 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 15+ messages in thread
From: Tom Lane @ 2022-02-16 04:30 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Andres Freund <andres@anarazel.de> writes:
> When I'd looked at this last I apparently (looking at git stash, I ended up
> discarding this) decided that the best way would be to change replace_token's
> API to one that just processes one line at a time, with an outer loop that
> processes all tokens in a line. I'm not really sure why anymore.
Hmm, I did it the other way, as attached. This gets it down to
==3254266== LEAK SUMMARY:
==3254266== definitely lost: 342 bytes in 17 blocks
==3254266== indirectly lost: 152 bytes in 2 blocks
==3254266== possibly lost: 0 bytes in 0 blocks
==3254266== still reachable: 2,403 bytes in 22 blocks
==3254266== suppressed: 0 bytes in 0 blocks
It seems that actually the pointer arrays *are* a big chunk of
the leakage, because the individual strings get freed in the
output loops! That's a bit ugly but I feel no need to change it.
regards, tom lane
Attachments:
alternate-initdb-cleanup.patch (text/x-diff, 12.8K) download
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 97f15971e2..76cb3a142e 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -237,11 +237,11 @@ static const char *const subdirs[] = {
static char bin_path[MAXPGPATH];
static char backend_exec[MAXPGPATH];
-static char **replace_token(char **lines,
+static void replace_token(char **lines,
const char *token, const char *replacement);
#ifndef HAVE_UNIX_SOCKETS
-static char **filter_lines_with_token(char **lines, const char *token);
+static void remove_lines_with_token(char **lines, const char *token);
#endif
static char **readfile(const char *path);
static void writefile(char *path, char **lines);
@@ -363,43 +363,33 @@ escape_quotes_bki(const char *src)
}
/*
- * make a copy of the array of lines, with token replaced by replacement
+ * Modify an array of malloc'd strings, replacing token by replacement
* the first time it occurs on each line.
*
* This does most of what sed was used for in the shell script, but
* doesn't need any regexp stuff.
*/
-static char **
+static void
replace_token(char **lines, const char *token, const char *replacement)
{
- int numlines = 1;
int i;
- char **result;
int toklen,
replen,
diff;
- for (i = 0; lines[i]; i++)
- numlines++;
-
- result = (char **) pg_malloc(numlines * sizeof(char *));
-
toklen = strlen(token);
replen = strlen(replacement);
diff = replen - toklen;
- for (i = 0; i < numlines; i++)
+ for (i = 0; lines[i]; i++)
{
char *where;
char *newline;
int pre;
- /* just copy pointer if NULL or no change needed */
- if (lines[i] == NULL || (where = strstr(lines[i], token)) == NULL)
- {
- result[i] = lines[i];
+ /* no work if no change needed */
+ if ((where = strstr(lines[i], token)) == NULL)
continue;
- }
/* if we get here a change is needed - set up new line */
@@ -413,39 +403,31 @@ replace_token(char **lines, const char *token, const char *replacement)
strcpy(newline + pre + replen, lines[i] + pre + toklen);
- result[i] = newline;
+ free(lines[i]);
+ lines[i] = newline;
}
-
- return result;
}
/*
- * make a copy of lines without any that contain the token
+ * remove strings from lines[] if they contain the token
*
* a sort of poor man's grep -v
*/
#ifndef HAVE_UNIX_SOCKETS
-static char **
-filter_lines_with_token(char **lines, const char *token)
+static void
+remove_lines_with_token(char **lines, const char *token)
{
- int numlines = 1;
- int i,
- src,
+ int src,
dst;
- char **result;
-
- for (i = 0; lines[i]; i++)
- numlines++;
-
- result = (char **) pg_malloc(numlines * sizeof(char *));
- for (src = 0, dst = 0; src < numlines; src++)
+ for (src = 0, dst = 0; lines[src]; src++)
{
- if (lines[src] == NULL || strstr(lines[src], token) == NULL)
- result[dst++] = lines[src];
+ if (strstr(lines[src], token) == NULL)
+ lines[dst++] = lines[src];
+ else
+ free(lines[src]);
}
-
- return result;
+ lines[dst] = NULL;
}
#endif
@@ -1066,7 +1048,7 @@ setup_config(void)
conflines = readfile(conf_file);
snprintf(repltok, sizeof(repltok), "max_connections = %d", n_connections);
- conflines = replace_token(conflines, "#max_connections = 100", repltok);
+ replace_token(conflines, "#max_connections = 100", repltok);
if ((n_buffers * (BLCKSZ / 1024)) % 1024 == 0)
snprintf(repltok, sizeof(repltok), "shared_buffers = %dMB",
@@ -1074,7 +1056,7 @@ setup_config(void)
else
snprintf(repltok, sizeof(repltok), "shared_buffers = %dkB",
n_buffers * (BLCKSZ / 1024));
- conflines = replace_token(conflines, "#shared_buffers = 128MB", repltok);
+ replace_token(conflines, "#shared_buffers = 128MB", repltok);
#ifdef HAVE_UNIX_SOCKETS
snprintf(repltok, sizeof(repltok), "#unix_socket_directories = '%s'",
@@ -1082,38 +1064,38 @@ setup_config(void)
#else
snprintf(repltok, sizeof(repltok), "#unix_socket_directories = ''");
#endif
- conflines = replace_token(conflines, "#unix_socket_directories = '/tmp'",
+ replace_token(conflines, "#unix_socket_directories = '/tmp'",
repltok);
#if DEF_PGPORT != 5432
snprintf(repltok, sizeof(repltok), "#port = %d", DEF_PGPORT);
- conflines = replace_token(conflines, "#port = 5432", repltok);
+ replace_token(conflines, "#port = 5432", repltok);
#endif
/* set default max_wal_size and min_wal_size */
snprintf(repltok, sizeof(repltok), "min_wal_size = %s",
pretty_wal_size(DEFAULT_MIN_WAL_SEGS));
- conflines = replace_token(conflines, "#min_wal_size = 80MB", repltok);
+ replace_token(conflines, "#min_wal_size = 80MB", repltok);
snprintf(repltok, sizeof(repltok), "max_wal_size = %s",
pretty_wal_size(DEFAULT_MAX_WAL_SEGS));
- conflines = replace_token(conflines, "#max_wal_size = 1GB", repltok);
+ replace_token(conflines, "#max_wal_size = 1GB", repltok);
snprintf(repltok, sizeof(repltok), "lc_messages = '%s'",
escape_quotes(lc_messages));
- conflines = replace_token(conflines, "#lc_messages = 'C'", repltok);
+ replace_token(conflines, "#lc_messages = 'C'", repltok);
snprintf(repltok, sizeof(repltok), "lc_monetary = '%s'",
escape_quotes(lc_monetary));
- conflines = replace_token(conflines, "#lc_monetary = 'C'", repltok);
+ replace_token(conflines, "#lc_monetary = 'C'", repltok);
snprintf(repltok, sizeof(repltok), "lc_numeric = '%s'",
escape_quotes(lc_numeric));
- conflines = replace_token(conflines, "#lc_numeric = 'C'", repltok);
+ replace_token(conflines, "#lc_numeric = 'C'", repltok);
snprintf(repltok, sizeof(repltok), "lc_time = '%s'",
escape_quotes(lc_time));
- conflines = replace_token(conflines, "#lc_time = 'C'", repltok);
+ replace_token(conflines, "#lc_time = 'C'", repltok);
switch (locale_date_order(lc_time))
{
@@ -1128,12 +1110,12 @@ setup_config(void)
strcpy(repltok, "datestyle = 'iso, mdy'");
break;
}
- conflines = replace_token(conflines, "#datestyle = 'iso, mdy'", repltok);
+ replace_token(conflines, "#datestyle = 'iso, mdy'", repltok);
snprintf(repltok, sizeof(repltok),
"default_text_search_config = 'pg_catalog.%s'",
escape_quotes(default_text_search_config));
- conflines = replace_token(conflines,
+ replace_token(conflines,
"#default_text_search_config = 'pg_catalog.simple'",
repltok);
@@ -1141,46 +1123,46 @@ setup_config(void)
{
snprintf(repltok, sizeof(repltok), "timezone = '%s'",
escape_quotes(default_timezone));
- conflines = replace_token(conflines, "#timezone = 'GMT'", repltok);
+ replace_token(conflines, "#timezone = 'GMT'", repltok);
snprintf(repltok, sizeof(repltok), "log_timezone = '%s'",
escape_quotes(default_timezone));
- conflines = replace_token(conflines, "#log_timezone = 'GMT'", repltok);
+ replace_token(conflines, "#log_timezone = 'GMT'", repltok);
}
snprintf(repltok, sizeof(repltok), "dynamic_shared_memory_type = %s",
dynamic_shared_memory_type);
- conflines = replace_token(conflines, "#dynamic_shared_memory_type = posix",
+ replace_token(conflines, "#dynamic_shared_memory_type = posix",
repltok);
#if DEFAULT_BACKEND_FLUSH_AFTER > 0
snprintf(repltok, sizeof(repltok), "#backend_flush_after = %dkB",
DEFAULT_BACKEND_FLUSH_AFTER * (BLCKSZ / 1024));
- conflines = replace_token(conflines, "#backend_flush_after = 0",
+ replace_token(conflines, "#backend_flush_after = 0",
repltok);
#endif
#if DEFAULT_BGWRITER_FLUSH_AFTER > 0
snprintf(repltok, sizeof(repltok), "#bgwriter_flush_after = %dkB",
DEFAULT_BGWRITER_FLUSH_AFTER * (BLCKSZ / 1024));
- conflines = replace_token(conflines, "#bgwriter_flush_after = 0",
+ replace_token(conflines, "#bgwriter_flush_after = 0",
repltok);
#endif
#if DEFAULT_CHECKPOINT_FLUSH_AFTER > 0
snprintf(repltok, sizeof(repltok), "#checkpoint_flush_after = %dkB",
DEFAULT_CHECKPOINT_FLUSH_AFTER * (BLCKSZ / 1024));
- conflines = replace_token(conflines, "#checkpoint_flush_after = 0",
+ replace_token(conflines, "#checkpoint_flush_after = 0",
repltok);
#endif
#ifndef USE_PREFETCH
- conflines = replace_token(conflines,
+ replace_token(conflines,
"#effective_io_concurrency = 1",
"#effective_io_concurrency = 0");
#endif
#ifdef WIN32
- conflines = replace_token(conflines,
+ replace_token(conflines,
"#update_process_title = on",
"#update_process_title = off");
#endif
@@ -1194,7 +1176,7 @@ setup_config(void)
(strcmp(authmethodhost, "md5") == 0 &&
strcmp(authmethodlocal, "scram-sha-256") != 0))
{
- conflines = replace_token(conflines,
+ replace_token(conflines,
"#password_encryption = scram-sha-256",
"password_encryption = md5");
}
@@ -1207,7 +1189,7 @@ setup_config(void)
*/
if (pg_dir_create_mode == PG_DIR_MODE_GROUP)
{
- conflines = replace_token(conflines,
+ replace_token(conflines,
"#log_file_mode = 0600",
"log_file_mode = 0640");
}
@@ -1248,9 +1230,9 @@ setup_config(void)
conflines = readfile(hba_file);
#ifndef HAVE_UNIX_SOCKETS
- conflines = filter_lines_with_token(conflines, "@remove-line-for-nolocal@");
+ remove_lines_with_token(conflines, "@remove-line-for-nolocal@");
#else
- conflines = replace_token(conflines, "@remove-line-for-nolocal@", "");
+ replace_token(conflines, "@remove-line-for-nolocal@", "");
#endif
#ifdef HAVE_IPV6
@@ -1287,33 +1269,33 @@ setup_config(void)
if (err != 0 ||
getaddrinfo("::1", NULL, &hints, &gai_result) != 0)
{
- conflines = replace_token(conflines,
+ replace_token(conflines,
"host all all ::1",
"#host all all ::1");
- conflines = replace_token(conflines,
+ replace_token(conflines,
"host replication all ::1",
"#host replication all ::1");
}
}
#else /* !HAVE_IPV6 */
/* If we didn't compile IPV6 support at all, always comment it out */
- conflines = replace_token(conflines,
+ replace_token(conflines,
"host all all ::1",
"#host all all ::1");
- conflines = replace_token(conflines,
+ replace_token(conflines,
"host replication all ::1",
"#host replication all ::1");
#endif /* HAVE_IPV6 */
/* Replace default authentication methods */
- conflines = replace_token(conflines,
+ replace_token(conflines,
"@authmethodhost@",
authmethodhost);
- conflines = replace_token(conflines,
+ replace_token(conflines,
"@authmethodlocal@",
authmethodlocal);
- conflines = replace_token(conflines,
+ replace_token(conflines,
"@authcomment@",
(strcmp(authmethodlocal, "trust") == 0 || strcmp(authmethodhost, "trust") == 0) ? AUTHTRUST_WARNING : "");
@@ -1382,27 +1364,27 @@ bootstrap_template1(void)
/* Substitute for various symbols used in the BKI file */
sprintf(buf, "%d", NAMEDATALEN);
- bki_lines = replace_token(bki_lines, "NAMEDATALEN", buf);
+ replace_token(bki_lines, "NAMEDATALEN", buf);
sprintf(buf, "%d", (int) sizeof(Pointer));
- bki_lines = replace_token(bki_lines, "SIZEOF_POINTER", buf);
+ replace_token(bki_lines, "SIZEOF_POINTER", buf);
- bki_lines = replace_token(bki_lines, "ALIGNOF_POINTER",
+ replace_token(bki_lines, "ALIGNOF_POINTER",
(sizeof(Pointer) == 4) ? "i" : "d");
- bki_lines = replace_token(bki_lines, "FLOAT8PASSBYVAL",
+ replace_token(bki_lines, "FLOAT8PASSBYVAL",
FLOAT8PASSBYVAL ? "true" : "false");
- bki_lines = replace_token(bki_lines, "POSTGRES",
+ replace_token(bki_lines, "POSTGRES",
escape_quotes_bki(username));
- bki_lines = replace_token(bki_lines, "ENCODING",
+ replace_token(bki_lines, "ENCODING",
encodingid_to_string(encodingid));
- bki_lines = replace_token(bki_lines, "LC_COLLATE",
+ replace_token(bki_lines, "LC_COLLATE",
escape_quotes_bki(lc_collate));
- bki_lines = replace_token(bki_lines, "LC_CTYPE",
+ replace_token(bki_lines, "LC_CTYPE",
escape_quotes_bki(lc_ctype));
/* Also ensure backend isn't confused by this environment var: */
@@ -1624,7 +1606,8 @@ setup_privileges(FILE *cmdfd)
{
char **line;
char **priv_lines;
- static char *privileges_setup[] = {
+ int i;
+ static const char * const privileges_setup[] = {
"UPDATE pg_class "
" SET relacl = (SELECT array_agg(a.acl) FROM "
" (SELECT E'=r/\"$POSTGRES_SUPERUSERNAME\"' as acl "
@@ -1759,10 +1742,19 @@ setup_privileges(FILE *cmdfd)
NULL
};
- priv_lines = replace_token(privileges_setup, "$POSTGRES_SUPERUSERNAME",
+ priv_lines = (char**) pg_malloc(lengthof(privileges_setup) * sizeof(char*));
+ for (i = 0; privileges_setup[i]; i++)
+ priv_lines[i] = pg_strdup(privileges_setup[i]);
+ priv_lines[i] = NULL;
+
+ replace_token(priv_lines, "$POSTGRES_SUPERUSERNAME",
escape_quotes(username));
for (line = priv_lines; *line != NULL; line++)
+ {
PG_CMD_PUTS(*line);
+ free(*line);
+ }
+ free(priv_lines);
}
/*
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 07:13 wliang@stu.xidian.edu.cn
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 15+ messages in thread
From: wliang@stu.xidian.edu.cn @ 2022-02-16 07:13 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
I have read all your discussion.
Thank you so much for your steady effort on this problem.
I would like to know the name of the software which is used to calculate the size of leaked memory.
It could be a great help to me to have a powerful tool like that.
Thanks again.
regards,
Wentao
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 08:13 wliang@stu.xidian.edu.cn
parent: wliang@stu.xidian.edu.cn
1 sibling, 0 replies; 15+ messages in thread
From: wliang@stu.xidian.edu.cn @ 2022-02-16 08:13 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
By the way,
This html files are the bug reports produced by static detection of clang's MallocChecker.
All of those bugs are the memory leak bugs in the initdb.c.
Some of them shares the same reason to the one I reported yesterday, while some of them are not.
Since you guys have already got the patch and fixed the problem, I don't know if it is too late to send this email.
However, if you can confirm those bugs really exist, it will be a great help to my study.
Once again, I want to give you my sincerely thanks.
regards,
Wentao
Attachments:
report-0c40f6.html (text/html, 603.3K) download
report-4ded84.html (text/html, 602.6K) download
report-55f328.html (text/html, 602.6K) download
report-a28ac0.html (text/html, 610.2K) download
report-da7c7f.html (text/html, 603.2K) download
report-e80982.html (text/html, 603.1K) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 15:37 Tom Lane <tgl@sss.pgh.pa.us>
parent: wliang@stu.xidian.edu.cn
0 siblings, 0 replies; 15+ messages in thread
From: Tom Lane @ 2022-02-16 15:37 UTC (permalink / raw)
To: wliang@stu.xidian.edu.cn; +Cc: Andres Freund <andres@anarazel.de>; Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
wliang@stu.xidian.edu.cn writes:
> I would like to know the name of the software which is used to calculate the size of leaked memory.
I was using valgrind, and I think Andres was too:
https://valgrind.org
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 17:37 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-02-16 17:37 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
On 2022-02-15 23:30:07 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > When I'd looked at this last I apparently (looking at git stash, I ended up
> > discarding this) decided that the best way would be to change replace_token's
> > API to one that just processes one line at a time, with an outer loop that
> > processes all tokens in a line. I'm not really sure why anymore.
>
> Hmm, I did it the other way, as attached.
My goal when I did this was to improve the performance, rather than reduce the
memory usage (this was a few months back). It's clearly better to perform all
the replacements of a line soon after each other, rather than iterate over all
the lines, once for each replacement. The latter is guaranteed to not have the
data in L2/L1 anymore.
But if we move to not needing replacement for postgres.bki anymore, that's not
an important goal anymore.
Not that it matters anymore: At least for postgres.bki, it doesn't seem to
make sense to use a line based approach in the first place? Seems like it
should really be a processing the input file as a whole, doing all the
replacements, into a resizable output buffer.
I didn't review it in detail, but I think your approach makes sense.
> This gets it down to
> ==3254266== LEAK SUMMARY:
> ==3254266== definitely lost: 342 bytes in 17 blocks
> ==3254266== indirectly lost: 152 bytes in 2 blocks
> ==3254266== possibly lost: 0 bytes in 0 blocks
> ==3254266== still reachable: 2,403 bytes in 22 blocks
> ==3254266== suppressed: 0 bytes in 0 blocks
That's presumably not about the approach, but about doing it for all
replacements, rather than just bootstrap, like I did :)
> It seems that actually the pointer arrays *are* a big chunk of
> the leakage, because the individual strings get freed in the
> output loops!
Right, isn't that what I had said?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Report a potential memory leak in setup_config()
@ 2022-02-16 17:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 15+ messages in thread
From: Tom Lane @ 2022-02-16 17:53 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <daniel@yesql.se>; wliang@stu.xidian.edu.cn, pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Andres Freund <andres@anarazel.de> writes:
> On 2022-02-15 23:30:07 -0500, Tom Lane wrote:
>> Hmm, I did it the other way, as attached.
> My goal when I did this was to improve the performance, rather than reduce the
> memory usage (this was a few months back). It's clearly better to perform all
> the replacements of a line soon after each other, rather than iterate over all
> the lines, once for each replacement. The latter is guaranteed to not have the
> data in L2/L1 anymore.
I'm skeptical that that effect is large enough to be interesting ...
> But if we move to not needing replacement for postgres.bki anymore, that's not
> an important goal anymore.
... and as you say, it won't matter if we push this processing to
the backend. So I'd prefer to go with the less-invasive change.
> Not that it matters anymore: At least for postgres.bki, it doesn't seem to
> make sense to use a line based approach in the first place? Seems like it
> should really be a processing the input file as a whole, doing all the
> replacements, into a resizable output buffer.
If we push this to the backend then it'd all have to be rethought.
I kind of like Peter's idea that maybe the run-time-variable items
could be handled by doing UPDATEs after the initial bootstrap.
(Very far down the road, maybe that would put us in a position where
the bootstrap phase could be replaced by copying some prefab data files.)
>> It seems that actually the pointer arrays *are* a big chunk of
>> the leakage, because the individual strings get freed in the
>> output loops!
> Right, isn't that what I had said?
I hadn't absorbed the existence of the per-string free() calls;
the fact that the reported leakage was close to the size of
postgres.bki misled me into thinking we were leaking the
strings too.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2022-02-16 17:53 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-15 01:49 Report a potential memory leak in setup_config() wliang@stu.xidian.edu.cn
2022-02-15 12:10 ` Daniel Gustafsson <daniel@yesql.se>
2022-02-15 16:33 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 01:29 ` Andres Freund <andres@anarazel.de>
2022-02-16 01:45 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 02:21 ` Andres Freund <andres@anarazel.de>
2022-02-16 02:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 02:38 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 04:10 ` Andres Freund <andres@anarazel.de>
2022-02-16 04:30 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 07:13 ` wliang@stu.xidian.edu.cn
2022-02-16 15:37 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 17:37 ` Andres Freund <andres@anarazel.de>
2022-02-16 17:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-02-16 08:13 ` wliang@stu.xidian.edu.cn
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