Skip to content

Commit e78d1d6

Browse files
tglsfdctglsfdc
committed
Fix assorted pretty-trivial memory leaks in the backend.
In the current system architecture, none of these are worth obsessing over; most are once-per-process leaks. However, Valgrind complains about all of them, and if we get to using threads rather than processes for backend sessions, it will become more interesting to avoid per-session leaks. * Fix leaks in StartupXLOG() and ShutdownWalRecovery(). * Fix leakage of pq_mq_handle in a parallel worker. While at it, move mq_putmessage's "Assert(pq_mq_handle != NULL)" to someplace where it's not trivially useless. * Fix leak in logicalrep_worker_detach(). * Don't leak the startup-packet buffer in ProcessStartupPacket(). * Fix leak in evtcache.c's DecodeTextArrayToBitmapset(). If the presented array is toasted, this neglected to free the detoasted copy, which was then leaked into EventTriggerCacheContext. * I'm distressed by the amount of code that BuildEventTriggerCache is willing to run while switched into a long-lived cache context. Although the detoasted array is the only leak that Valgrind reports, let's tighten things up while we're here. (DecodeTextArrayToBitmapset is still run in the cache context, so doing this doesn't remove the need for the detoast fix. But it reduces the surface area for other leaks.) * load_domaintype_info() intentionally leaked some intermediate cruft into the long-lived DomainConstraintCache's memory context, reasoning that the amount of leakage will typically not be much so it's not worth doing a copyObject() of the final tree to avoid that. But Valgrind knows nothing of engineering tradeoffs and complains anyway. On the whole, the copyObject doesn't cost that much and this is surely not a performance-critical code path, so let's do it the clean way. * MarkGUCPrefixReserved didn't bother to clean up removed placeholder GUCs at all, which shows up as a leak in one regression test. It seems appropriate for it to do as much cleanup as define_custom_variable does when replacing placeholders, so factor that code out into a helper function. define_custom_variable's logic was one brick shy of a load too: it forgot to free the separate allocation for the placeholder's name. Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Andres Freund <andres@anarazel.de> Reviewed-by: Richard Guo <guofenglinux@gmail.com> Discussion: https://postgr.es/m/285483.1746756246@sss.pgh.pa.us
1 parent 9e91901 commit e78d1d6

File tree

8 files changed

+99
-53
lines changed

8 files changed

+99
-53
lines changed

‎src/backend/access/transam/xlog.c

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,7 @@ static void InitControlFile(uint64 sysidentifier, uint32 data_checksum_version);
703703
static void WriteControlFile(void);
704704
static void ReadControlFile(void);
705705
static void UpdateControlFile(void);
706-
static char *str_time(pg_time_t tnow);
706+
static char *str_time(pg_time_t tnow, char *buf, size_t bufsize);
707707

708708
static int get_sync_bit(int method);
709709

@@ -5371,11 +5371,9 @@ BootStrapXLOG(uint32 data_checksum_version)
53715371
}
53725372

53735373
static char *
5374-
str_time(pg_time_t tnow)
5374+
str_time(pg_time_t tnow, char *buf, size_t bufsize)
53755375
{
5376-
char *buf = palloc(128);
5377-
5378-
pg_strftime(buf, 128,
5376+
pg_strftime(buf, bufsize,
53795377
"%Y-%m-%d %H:%M:%S %Z",
53805378
pg_localtime(&tnow, log_timezone));
53815379

@@ -5618,6 +5616,7 @@ StartupXLOG(void)
56185616
XLogRecPtr missingContrecPtr;
56195617
TransactionId oldestActiveXID;
56205618
bool promoted = false;
5619+
char timebuf[128];
56215620

56225621
/*
56235622
* We should have an aux process resource owner to use, and we should not
@@ -5646,41 +5645,47 @@ StartupXLOG(void)
56465645
*/
56475646
ereport(IsPostmasterEnvironment ? LOG : NOTICE,
56485647
(errmsg("database system was shut down at %s",
5649-
str_time(ControlFile->time))));
5648+
str_time(ControlFile->time,
5649+
timebuf, sizeof(timebuf)))));
56505650
break;
56515651

56525652
case DB_SHUTDOWNED_IN_RECOVERY:
56535653
ereport(LOG,
56545654
(errmsg("database system was shut down in recovery at %s",
5655-
str_time(ControlFile->time))));
5655+
str_time(ControlFile->time,
5656+
timebuf, sizeof(timebuf)))));
56565657
break;
56575658

56585659
case DB_SHUTDOWNING:
56595660
ereport(LOG,
56605661
(errmsg("database system shutdown was interrupted; last known up at %s",
5661-
str_time(ControlFile->time))));
5662+
str_time(ControlFile->time,
5663+
timebuf, sizeof(timebuf)))));
56625664
break;
56635665

56645666
case DB_IN_CRASH_RECOVERY:
56655667
ereport(LOG,
56665668
(errmsg("database system was interrupted while in recovery at %s",
5667-
str_time(ControlFile->time)),
5669+
str_time(ControlFile->time,
5670+
timebuf, sizeof(timebuf))),
56685671
errhint("This probably means that some data is corrupted and"
56695672
" you will have to use the last backup for recovery.")));
56705673
break;
56715674

56725675
case DB_IN_ARCHIVE_RECOVERY:
56735676
ereport(LOG,
56745677
(errmsg("database system was interrupted while in recovery at log time %s",
5675-
str_time(ControlFile->checkPointCopy.time)),
5678+
str_time(ControlFile->checkPointCopy.time,
5679+
timebuf, sizeof(timebuf))),
56765680
errhint("If this has occurred more than once some data might be corrupted"
56775681
" and you might need to choose an earlier recovery target.")));
56785682
break;
56795683

56805684
case DB_IN_PRODUCTION:
56815685
ereport(LOG,
56825686
(errmsg("database system was interrupted; last known up at %s",
5683-
str_time(ControlFile->time))));
5687+
str_time(ControlFile->time,
5688+
timebuf, sizeof(timebuf)))));
56845689
break;
56855690

56865691
default:
@@ -6325,6 +6330,12 @@ StartupXLOG(void)
63256330
*/
63266331
CompleteCommitTsInitialization();
63276332

6333+
/* Clean up EndOfWalRecoveryInfo data to appease Valgrind leak checking */
6334+
if (endOfRecoveryInfo->lastPage)
6335+
pfree(endOfRecoveryInfo->lastPage);
6336+
pfree(endOfRecoveryInfo->recoveryStopReason);
6337+
pfree(endOfRecoveryInfo);
6338+
63286339
/*
63296340
* All done with end-of-recovery actions.
63306341
*

‎src/backend/access/transam/xlogrecovery.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1626,6 +1626,7 @@ ShutdownWalRecovery(void)
16261626
close(readFile);
16271627
readFile = -1;
16281628
}
1629+
pfree(xlogreader->private_data);
16291630
XLogReaderFree(xlogreader);
16301631
XLogPrefetcherFree(xlogprefetcher);
16311632

‎src/backend/libpq/pqmq.c

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
#include "tcop/tcopprot.h"
2424
#include "utils/builtins.h"
2525

26-
static shm_mq_handle *pq_mq_handle;
26+
static shm_mq_handle *pq_mq_handle = NULL;
2727
static bool pq_mq_busy = false;
2828
static pid_t pq_mq_parallel_leader_pid = 0;
2929
static ProcNumber pq_mq_parallel_leader_proc_number = INVALID_PROC_NUMBER;
@@ -66,7 +66,11 @@ pq_redirect_to_shm_mq(dsm_segment *seg, shm_mq_handle *mqh)
6666
static void
6767
pq_cleanup_redirect_to_shm_mq(dsm_segment *seg, Datum arg)
6868
{
69-
pq_mq_handle = NULL;
69+
if (pq_mq_handle != NULL)
70+
{
71+
pfree(pq_mq_handle);
72+
pq_mq_handle = NULL;
73+
}
7074
whereToSendOutput = DestNone;
7175
}
7276

@@ -131,8 +135,11 @@ mq_putmessage(char msgtype, const char *s, size_t len)
131135
if (pq_mq_busy)
132136
{
133137
if (pq_mq_handle != NULL)
138+
{
134139
shm_mq_detach(pq_mq_handle);
135-
pq_mq_handle = NULL;
140+
pfree(pq_mq_handle);
141+
pq_mq_handle = NULL;
142+
}
136143
return EOF;
137144
}
138145

@@ -152,15 +159,14 @@ mq_putmessage(char msgtype, const char *s, size_t len)
152159
iov[1].data = s;
153160
iov[1].len = len;
154161

155-
Assert(pq_mq_handle != NULL);
156-
157162
for (;;)
158163
{
159164
/*
160165
* Immediately notify the receiver by passing force_flush as true so
161166
* that the shared memory value is updated before we send the parallel
162167
* message signal right after this.
163168
*/
169+
Assert(pq_mq_handle != NULL);
164170
result = shm_mq_sendv(pq_mq_handle, iov, 2, true, true);
165171

166172
if (pq_mq_parallel_leader_pid != 0)

‎src/backend/replication/logical/launcher.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,8 @@ logicalrep_worker_detach(void)
790790
}
791791

792792
LWLockRelease(LogicalRepWorkerLock);
793+
794+
list_free(workers);
793795
}
794796

795797
/* Block concurrent access. */

‎src/backend/tcop/backend_startup.c

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -492,7 +492,7 @@ static int
492492
ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
493493
{
494494
int32 len;
495-
char *buf;
495+
char *buf = NULL;
496496
ProtocolVersion proto;
497497
MemoryContext oldcontext;
498498

@@ -516,7 +516,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
516516
* scanners, which may be less benign, but it's not really our job to
517517
* notice those.)
518518
*/
519-
return STATUS_ERROR;
519+
goto fail;
520520
}
521521

522522
if (pq_getbytes(((char *) &len) + 1, 3) == EOF)
@@ -526,7 +526,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
526526
ereport(COMMERROR,
527527
(errcode(ERRCODE_PROTOCOL_VIOLATION),
528528
errmsg("incomplete startup packet")));
529-
return STATUS_ERROR;
529+
goto fail;
530530
}
531531

532532
len = pg_ntoh32(len);
@@ -538,7 +538,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
538538
ereport(COMMERROR,
539539
(errcode(ERRCODE_PROTOCOL_VIOLATION),
540540
errmsg("invalid length of startup packet")));
541-
return STATUS_ERROR;
541+
goto fail;
542542
}
543543

544544
/*
@@ -554,7 +554,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
554554
ereport(COMMERROR,
555555
(errcode(ERRCODE_PROTOCOL_VIOLATION),
556556
errmsg("incomplete startup packet")));
557-
return STATUS_ERROR;
557+
goto fail;
558558
}
559559
pq_endmsgread();
560560

@@ -568,7 +568,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
568568
{
569569
ProcessCancelRequestPacket(port, buf, len);
570570
/* Not really an error, but we don't want to proceed further */
571-
return STATUS_ERROR;
571+
goto fail;
572572
}
573573

574574
if (proto == NEGOTIATE_SSL_CODE && !ssl_done)
@@ -607,14 +607,16 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
607607
ereport(COMMERROR,
608608
(errcode_for_socket_access(),
609609
errmsg("failed to send SSL negotiation response: %m")));
610-
return STATUS_ERROR; /* close the connection */
610+
goto fail; /* close the connection */
611611
}
612612

613613
#ifdef USE_SSL
614614
if (SSLok == 'S' && secure_open_server(port) == -1)
615-
return STATUS_ERROR;
615+
goto fail;
616616
#endif
617617

618+
pfree(buf);
619+
618620
/*
619621
* At this point we should have no data already buffered. If we do,
620622
* it was received before we performed the SSL handshake, so it wasn't
@@ -661,14 +663,16 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
661663
ereport(COMMERROR,
662664
(errcode_for_socket_access(),
663665
errmsg("failed to send GSSAPI negotiation response: %m")));
664-
return STATUS_ERROR; /* close the connection */
666+
goto fail; /* close the connection */
665667
}
666668

667669
#ifdef ENABLE_GSS
668670
if (GSSok == 'G' && secure_open_gssapi(port) == -1)
669-
return STATUS_ERROR;
671+
goto fail;
670672
#endif
671673

674+
pfree(buf);
675+
672676
/*
673677
* At this point we should have no data already buffered. If we do,
674678
* it was received before we performed the GSS handshake, so it wasn't
@@ -863,7 +867,16 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
863867
*/
864868
MemoryContextSwitchTo(oldcontext);
865869

870+
pfree(buf);
871+
866872
return STATUS_OK;
873+
874+
fail:
875+
/* be tidy, just to avoid Valgrind complaints */
876+
if (buf)
877+
pfree(buf);
878+
879+
return STATUS_ERROR;
867880
}
868881

869882
/*

‎src/backend/utils/cache/evtcache.c

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ BuildEventTriggerCache(void)
7878
{
7979
HASHCTL ctl;
8080
HTAB *cache;
81-
MemoryContext oldcontext;
8281
Relation rel;
8382
Relation irel;
8483
SysScanDesc scan;
@@ -110,9 +109,6 @@ BuildEventTriggerCache(void)
110109
(Datum) 0);
111110
}
112111

113-
/* Switch to correct memory context. */
114-
oldcontext = MemoryContextSwitchTo(EventTriggerCacheContext);
115-
116112
/* Prevent the memory context from being nuked while we're rebuilding. */
117113
EventTriggerCacheState = ETCS_REBUILD_STARTED;
118114

@@ -145,6 +141,7 @@ BuildEventTriggerCache(void)
145141
bool evttags_isnull;
146142
EventTriggerCacheEntry *entry;
147143
bool found;
144+
MemoryContext oldcontext;
148145

149146
/* Get next tuple. */
150147
tup = systable_getnext_ordered(scan, ForwardScanDirection);
@@ -171,6 +168,9 @@ BuildEventTriggerCache(void)
171168
else
172169
continue;
173170

171+
/* Switch to correct memory context. */
172+
oldcontext = MemoryContextSwitchTo(EventTriggerCacheContext);
173+
174174
/* Allocate new cache item. */
175175
item = palloc0(sizeof(EventTriggerCacheItem));
176176
item->fnoid = form->evtfoid;
@@ -188,16 +188,16 @@ BuildEventTriggerCache(void)
188188
entry->triggerlist = lappend(entry->triggerlist, item);
189189
else
190190
entry->triggerlist = list_make1(item);
191+
192+
/* Restore previous memory context. */
193+
MemoryContextSwitchTo(oldcontext);
191194
}
192195

193196
/* Done with pg_event_trigger scan. */
194197
systable_endscan_ordered(scan);
195198
index_close(irel, AccessShareLock);
196199
relation_close(rel, AccessShareLock);
197200

198-
/* Restore previous memory context. */
199-
MemoryContextSwitchTo(oldcontext);
200-
201201
/* Install new cache. */
202202
EventTriggerCache = cache;
203203

@@ -240,6 +240,8 @@ DecodeTextArrayToBitmapset(Datum array)
240240
}
241241

242242
pfree(elems);
243+
if ((Pointer) arr != DatumGetPointer(array))
244+
pfree(arr);
243245

244246
return bms;
245247
}

‎src/backend/utils/cache/typcache.c

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,9 +1171,6 @@ load_domaintype_info(TypeCacheEntry *typentry)
11711171
elog(ERROR, "domain \"%s\" constraint \"%s\" has NULL conbin",
11721172
NameStr(typTup->typname), NameStr(c->conname));
11731173

1174-
/* Convert conbin to C string in caller context */
1175-
constring = TextDatumGetCString(val);
1176-
11771174
/* Create the DomainConstraintCache object and context if needed */
11781175
if (dcc == NULL)
11791176
{
@@ -1189,9 +1186,8 @@ load_domaintype_info(TypeCacheEntry *typentry)
11891186
dcc->dccRefCount = 0;
11901187
}
11911188

1192-
/* Create node trees in DomainConstraintCache's context */
1193-
oldcxt = MemoryContextSwitchTo(dcc->dccContext);
1194-
1189+
/* Convert conbin to a node tree, still in caller's context */
1190+
constring = TextDatumGetCString(val);
11951191
check_expr = (Expr *) stringToNode(constring);
11961192

11971193
/*
@@ -1206,10 +1202,13 @@ load_domaintype_info(TypeCacheEntry *typentry)
12061202
*/
12071203
check_expr = expression_planner(check_expr);
12081204

1205+
/* Create only the minimally needed stuff in dccContext */
1206+
oldcxt = MemoryContextSwitchTo(dcc->dccContext);
1207+
12091208
r = makeNode(DomainConstraintState);
12101209
r->constrainttype = DOM_CONSTRAINT_CHECK;
12111210
r->name = pstrdup(NameStr(c->conname));
1212-
r->check_expr = check_expr;
1211+
r->check_expr = copyObject(check_expr);
12131212
r->check_exprstate = NULL;
12141213

12151214
MemoryContextSwitchTo(oldcxt);

0 commit comments

Comments
 (0)