Skip to content

Commit 37c87e6

Browse files
anarazelanarazel
committed
Change relpath() et al to return path by value
For AIO, and also some other recent patches, we need the ability to call relpath() in a critical section. Until now that was not feasible, as it allocated memory. The fact that relpath() allocated memory also made it awkward to use in log messages because we had to take care to free the memory afterwards. Which we e.g. didn't do for when zeroing out an invalid buffer. We discussed other solutions, e.g. filling a pre-allocated buffer that's passed to relpath(), but they all came with plenty downsides or were larger projects. The easiest fix seems to be to make relpath() return the path by value. To be able to return the path by value we need to determine the maximum length of a relation path. This patch adds a long #define that computes the exact maximum, which is verified to be correct in a regression test. As this change the signature of relpath(), extensions using it will need to adapt their code. We discussed leaving a backward-compat shim in place, but decided it's not worth it given the use of relpath() doesn't seem widespread. Discussion: https://postgr.es/m/xeri5mla4b5syjd5a25nok5iez2kr3bm26j2qn4u7okzof2bmf@kwdh2vf7npra
1 parent 32c393f commit 37c87e6

File tree

18 files changed

+218
-159
lines changed

18 files changed

+218
-159
lines changed

‎src/backend/access/rmgrdesc/smgrdesc.c

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,19 +26,17 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
2626
if (info == XLOG_SMGR_CREATE)
2727
{
2828
xl_smgr_create *xlrec = (xl_smgr_create *) rec;
29-
char *path = relpathperm(xlrec->rlocator, xlrec->forkNum);
3029

31-
appendStringInfoString(buf, path);
32-
pfree(path);
30+
appendStringInfoString(buf,
31+
relpathperm(xlrec->rlocator, xlrec->forkNum).str);
3332
}
3433
else if (info == XLOG_SMGR_TRUNCATE)
3534
{
3635
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) rec;
37-
char *path = relpathperm(xlrec->rlocator, MAIN_FORKNUM);
3836

39-
appendStringInfo(buf, "%s to %u blocks flags %d", path,
37+
appendStringInfo(buf, "%s to %u blocks flags %d",
38+
relpathperm(xlrec->rlocator, MAIN_FORKNUM).str,
4039
xlrec->blkno, xlrec->flags);
41-
pfree(path);
4240
}
4341
}
4442

‎src/backend/access/rmgrdesc/xactdesc.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,10 +287,8 @@ xact_desc_relations(StringInfo buf, char *label, int nrels,
287287
appendStringInfo(buf, "; %s:", label);
288288
for (i = 0; i < nrels; i++)
289289
{
290-
char *path = relpathperm(xlocators[i], MAIN_FORKNUM);
291-
292-
appendStringInfo(buf, " %s", path);
293-
pfree(path);
290+
appendStringInfo(buf, " %s",
291+
relpathperm(xlocators[i], MAIN_FORKNUM).str);
294292
}
295293
}
296294
}

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

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,14 @@ static void
8686
report_invalid_page(int elevel, RelFileLocator locator, ForkNumber forkno,
8787
BlockNumber blkno, bool present)
8888
{
89-
char *path = relpathperm(locator, forkno);
89+
RelPathStr path = relpathperm(locator, forkno);
9090

9191
if (present)
9292
elog(elevel, "page %u of relation %s is uninitialized",
93-
blkno, path);
93+
blkno, path.str);
9494
else
9595
elog(elevel, "page %u of relation %s does not exist",
96-
blkno, path);
97-
pfree(path);
96+
blkno, path.str);
9897
}
9998

10099
/* Log a reference to an invalid page */
@@ -180,14 +179,9 @@ forget_invalid_pages(RelFileLocator locator, ForkNumber forkno,
180179
hentry->key.forkno == forkno &&
181180
hentry->key.blkno >= minblkno)
182181
{
183-
if (message_level_is_interesting(DEBUG2))
184-
{
185-
char *path = relpathperm(hentry->key.locator, forkno);
186-
187-
elog(DEBUG2, "page %u of relation %s has been dropped",
188-
hentry->key.blkno, path);
189-
pfree(path);
190-
}
182+
elog(DEBUG2, "page %u of relation %s has been dropped",
183+
hentry->key.blkno,
184+
relpathperm(hentry->key.locator, forkno).str);
191185

192186
if (hash_search(invalid_page_tab,
193187
&hentry->key,
@@ -213,14 +207,9 @@ forget_invalid_pages_db(Oid dbid)
213207
{
214208
if (hentry->key.locator.dbOid == dbid)
215209
{
216-
if (message_level_is_interesting(DEBUG2))
217-
{
218-
char *path = relpathperm(hentry->key.locator, hentry->key.forkno);
219-
220-
elog(DEBUG2, "page %u of relation %s has been dropped",
221-
hentry->key.blkno, path);
222-
pfree(path);
223-
}
210+
elog(DEBUG2, "page %u of relation %s has been dropped",
211+
hentry->key.blkno,
212+
relpathperm(hentry->key.locator, hentry->key.forkno).str);
224213

225214
if (hash_search(invalid_page_tab,
226215
&hentry->key,

‎src/backend/backup/basebackup_incremental.c

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -625,23 +625,21 @@ char *
625625
GetIncrementalFilePath(Oid dboid, Oid spcoid, RelFileNumber relfilenumber,
626626
ForkNumber forknum, unsigned segno)
627627
{
628-
char *path;
628+
RelPathStr path;
629629
char *lastslash;
630630
char *ipath;
631631

632632
path = GetRelationPath(dboid, spcoid, relfilenumber, INVALID_PROC_NUMBER,
633633
forknum);
634634

635-
lastslash = strrchr(path, '/');
635+
lastslash = strrchr(path.str, '/');
636636
Assert(lastslash != NULL);
637637
*lastslash = '\0';
638638

639639
if (segno > 0)
640-
ipath = psprintf("%s/INCREMENTAL.%s.%u", path, lastslash + 1, segno);
640+
ipath = psprintf("%s/INCREMENTAL.%s.%u", path.str, lastslash + 1, segno);
641641
else
642-
ipath = psprintf("%s/INCREMENTAL.%s", path, lastslash + 1);
643-
644-
pfree(path);
642+
ipath = psprintf("%s/INCREMENTAL.%s", path.str, lastslash + 1);
645643

646644
return ipath;
647645
}

‎src/backend/catalog/catalog.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,7 @@ RelFileNumber
528528
GetNewRelFileNumber(Oid reltablespace, Relation pg_class, char relpersistence)
529529
{
530530
RelFileLocatorBackend rlocator;
531-
char *rpath;
531+
RelPathStr rpath;
532532
bool collides;
533533
ProcNumber procNumber;
534534

@@ -580,7 +580,7 @@ GetNewRelFileNumber(Oid reltablespace, Relation pg_class, char relpersistence)
580580
/* Check for existing file of same name */
581581
rpath = relpath(rlocator, MAIN_FORKNUM);
582582

583-
if (access(rpath, F_OK) == 0)
583+
if (access(rpath.str, F_OK) == 0)
584584
{
585585
/* definite collision */
586586
collides = true;
@@ -596,8 +596,6 @@ GetNewRelFileNumber(Oid reltablespace, Relation pg_class, char relpersistence)
596596
*/
597597
collides = false;
598598
}
599-
600-
pfree(rpath);
601599
} while (collides);
602600

603601
return rlocator.locator.relNumber;

‎src/backend/catalog/storage.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -524,14 +524,14 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
524524
* (errcontext callbacks shouldn't be risking any such thing, but
525525
* people have been known to forget that rule.)
526526
*/
527-
char *relpath = relpathbackend(src->smgr_rlocator.locator,
527+
RelPathStr relpath = relpathbackend(src->smgr_rlocator.locator,
528528
src->smgr_rlocator.backend,
529529
forkNum);
530530

531531
ereport(ERROR,
532532
(errcode(ERRCODE_DATA_CORRUPTED),
533533
errmsg("invalid page in block %u of relation %s",
534-
blkno, relpath)));
534+
blkno, relpath.str)));
535535
}
536536

537537
/*

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2329,15 +2329,15 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
23292329
else if (reloid == InvalidOid)
23302330
elog(ERROR, "could not map filenumber \"%s\" to relation OID",
23312331
relpathperm(change->data.tp.rlocator,
2332-
MAIN_FORKNUM));
2332+
MAIN_FORKNUM).str);
23332333

23342334
relation = RelationIdGetRelation(reloid);
23352335

23362336
if (!RelationIsValid(relation))
23372337
elog(ERROR, "could not open relation with OID %u (for filenumber \"%s\")",
23382338
reloid,
23392339
relpathperm(change->data.tp.rlocator,
2340-
MAIN_FORKNUM));
2340+
MAIN_FORKNUM).str);
23412341

23422342
if (!RelationIsLogicallyLogged(relation))
23432343
goto change_done;

‎src/backend/storage/buffer/bufmgr.c

Lines changed: 18 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,15 +1541,15 @@ WaitReadBuffers(ReadBuffersOperation *operation)
15411541
(errcode(ERRCODE_DATA_CORRUPTED),
15421542
errmsg("invalid page in block %u of relation %s; zeroing out page",
15431543
io_first_block + j,
1544-
relpath(operation->smgr->smgr_rlocator, forknum))));
1544+
relpath(operation->smgr->smgr_rlocator, forknum).str)));
15451545
memset(bufBlock, 0, BLCKSZ);
15461546
}
15471547
else
15481548
ereport(ERROR,
15491549
(errcode(ERRCODE_DATA_CORRUPTED),
15501550
errmsg("invalid page in block %u of relation %s",
15511551
io_first_block + j,
1552-
relpath(operation->smgr->smgr_rlocator, forknum))));
1552+
relpath(operation->smgr->smgr_rlocator, forknum).str)));
15531553
}
15541554

15551555
/* Terminate I/O and set BM_VALID. */
@@ -2284,7 +2284,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
22842284
ereport(ERROR,
22852285
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
22862286
errmsg("cannot extend relation %s beyond %u blocks",
2287-
relpath(bmr.smgr->smgr_rlocator, fork),
2287+
relpath(bmr.smgr->smgr_rlocator, fork).str,
22882288
MaxBlockNumber)));
22892289

22902290
/*
@@ -2355,7 +2355,8 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
23552355
if (valid && !PageIsNew((Page) buf_block))
23562356
ereport(ERROR,
23572357
(errmsg("unexpected data beyond EOF in block %u of relation %s",
2358-
existing_hdr->tag.blockNum, relpath(bmr.smgr->smgr_rlocator, fork)),
2358+
existing_hdr->tag.blockNum,
2359+
relpath(bmr.smgr->smgr_rlocator, fork).str),
23592360
errhint("This has been seen to occur with buggy kernels; consider updating your system.")));
23602361

23612362
/*
@@ -3663,7 +3664,6 @@ DebugPrintBufferRefcount(Buffer buffer)
36633664
{
36643665
BufferDesc *buf;
36653666
int32 loccount;
3666-
char *path;
36673667
char *result;
36683668
ProcNumber backend;
36693669
uint32 buf_state;
@@ -3683,15 +3683,14 @@ DebugPrintBufferRefcount(Buffer buffer)
36833683
}
36843684

36853685
/* theoretically we should lock the bufhdr here */
3686-
path = relpathbackend(BufTagGetRelFileLocator(&buf->tag), backend,
3687-
BufTagGetForkNum(&buf->tag));
36883686
buf_state = pg_atomic_read_u32(&buf->state);
36893687

36903688
result = psprintf("[%03d] (rel=%s, blockNum=%u, flags=0x%x, refcount=%u %d)",
3691-
buffer, path,
3689+
buffer,
3690+
relpathbackend(BufTagGetRelFileLocator(&buf->tag), backend,
3691+
BufTagGetForkNum(&buf->tag)).str,
36923692
buf->tag.blockNum, buf_state & BUF_FLAG_MASK,
36933693
BUF_STATE_GET_REFCOUNT(buf_state), loccount);
3694-
pfree(path);
36953694
return result;
36963695
}
36973696

@@ -5611,16 +5610,13 @@ AbortBufferIO(Buffer buffer)
56115610
if (buf_state & BM_IO_ERROR)
56125611
{
56135612
/* Buffer is pinned, so we can read tag without spinlock */
5614-
char *path;
5615-
5616-
path = relpathperm(BufTagGetRelFileLocator(&buf_hdr->tag),
5617-
BufTagGetForkNum(&buf_hdr->tag));
56185613
ereport(WARNING,
56195614
(errcode(ERRCODE_IO_ERROR),
56205615
errmsg("could not write block %u of %s",
5621-
buf_hdr->tag.blockNum, path),
5616+
buf_hdr->tag.blockNum,
5617+
relpathperm(BufTagGetRelFileLocator(&buf_hdr->tag),
5618+
BufTagGetForkNum(&buf_hdr->tag)).str),
56225619
errdetail("Multiple failures --- write error might be permanent.")));
5623-
pfree(path);
56245620
}
56255621
}
56265622

@@ -5637,14 +5633,10 @@ shared_buffer_write_error_callback(void *arg)
56375633

56385634
/* Buffer is pinned, so we can read the tag without locking the spinlock */
56395635
if (bufHdr != NULL)
5640-
{
5641-
char *path = relpathperm(BufTagGetRelFileLocator(&bufHdr->tag),
5642-
BufTagGetForkNum(&bufHdr->tag));
5643-
56445636
errcontext("writing block %u of relation %s",
5645-
bufHdr->tag.blockNum, path);
5646-
pfree(path);
5647-
}
5637+
bufHdr->tag.blockNum,
5638+
relpathperm(BufTagGetRelFileLocator(&bufHdr->tag),
5639+
BufTagGetForkNum(&bufHdr->tag)).str);
56485640
}
56495641

56505642
/*
@@ -5656,15 +5648,11 @@ local_buffer_write_error_callback(void *arg)
56565648
BufferDesc *bufHdr = (BufferDesc *) arg;
56575649

56585650
if (bufHdr != NULL)
5659-
{
5660-
char *path = relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag),
5661-
MyProcNumber,
5662-
BufTagGetForkNum(&bufHdr->tag));
5663-
56645651
errcontext("writing block %u of relation %s",
5665-
bufHdr->tag.blockNum, path);
5666-
pfree(path);
5667-
}
5652+
bufHdr->tag.blockNum,
5653+
relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag),
5654+
MyProcNumber,
5655+
BufTagGetForkNum(&bufHdr->tag)).str);
56685656
}
56695657

56705658
/*

‎src/backend/storage/buffer/localbuf.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ ExtendBufferedRelLocal(BufferManagerRelation bmr,
360360
ereport(ERROR,
361361
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
362362
errmsg("cannot extend relation %s beyond %u blocks",
363-
relpath(bmr.smgr->smgr_rlocator, fork),
363+
relpath(bmr.smgr->smgr_rlocator, fork).str,
364364
MaxBlockNumber)));
365365

366366
for (uint32 i = 0; i < extend_by; i++)
@@ -510,7 +510,7 @@ DropRelationLocalBuffers(RelFileLocator rlocator, ForkNumber forkNum,
510510
bufHdr->tag.blockNum,
511511
relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag),
512512
MyProcNumber,
513-
BufTagGetForkNum(&bufHdr->tag)),
513+
BufTagGetForkNum(&bufHdr->tag)).str,
514514
LocalRefCount[i]);
515515

516516
/* Remove entry from hashtable */
@@ -555,7 +555,7 @@ DropRelationAllLocalBuffers(RelFileLocator rlocator)
555555
bufHdr->tag.blockNum,
556556
relpathbackend(BufTagGetRelFileLocator(&bufHdr->tag),
557557
MyProcNumber,
558-
BufTagGetForkNum(&bufHdr->tag)),
558+
BufTagGetForkNum(&bufHdr->tag)).str,
559559
LocalRefCount[i]);
560560
/* Remove entry from hashtable */
561561
hresult = (LocalBufferLookupEnt *)

0 commit comments

Comments
 (0)