public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/4] GiST-Optimal-WAL-Usage
20+ messages / 10 participants
[nested] [flat]
* [PATCH 3/4] GiST-Optimal-WAL-Usage
@ 2019-04-01 04:07 Andrey V. Lepikhov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Andrey V. Lepikhov @ 2019-04-01 04:07 UTC (permalink / raw)
---
src/backend/access/gist/gist.c | 46 ++++++++++++++++++--------
src/backend/access/gist/gistbuild.c | 32 ++++++++++--------
src/backend/access/gist/gistutil.c | 2 +-
src/backend/access/gist/gistxlog.c | 22 ------------
src/backend/access/rmgrdesc/gistdesc.c | 5 ---
src/include/access/gist_private.h | 7 ++--
src/include/access/gistxlog.h | 1 -
7 files changed, 56 insertions(+), 59 deletions(-)
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 2fddb23496..0e2b6c3014 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -172,7 +172,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
values, isnull, true /* size is currently bogus */ );
itup->t_tid = *ht_ctid;
- gistdoinsert(r, itup, 0, giststate, heapRel);
+ gistdoinsert(r, itup, 0, giststate, heapRel, false);
/* cleanup */
MemoryContextSwitchTo(oldCxt);
@@ -219,7 +219,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
Buffer leftchildbuf,
List **splitinfo,
bool markfollowright,
- Relation heapRel)
+ Relation heapRel,
+ bool is_build)
{
BlockNumber blkno = BufferGetBlockNumber(buffer);
Page page = BufferGetPage(buffer);
@@ -458,7 +459,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
* insertion for that. NB: The number of pages and data segments
* specified here must match the calculations in gistXLogSplit()!
*/
- if (RelationNeedsWAL(rel))
+ if (RelationNeedsWAL(rel) && !is_build)
XLogEnsureRecordSpace(npage, 1 + npage * 2);
START_CRIT_SECTION();
@@ -479,18 +480,20 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
PageRestoreTempPage(dist->page, BufferGetPage(dist->buffer));
dist->page = BufferGetPage(dist->buffer);
- /* Write the WAL record */
- if (RelationNeedsWAL(rel))
+ /*
+ * Write the WAL record.
+ * Do not write XLog entry if the insertion is caused by
+ * index build process.
+ */
+ if (RelationNeedsWAL(rel) && !is_build)
recptr = gistXLogSplit(is_leaf,
- dist, oldrlink, oldnsn, leftchildbuf,
- markfollowright);
+ dist, oldrlink, oldnsn, leftchildbuf,
+ markfollowright);
else
recptr = gistGetFakeLSN(rel);
for (ptr = dist; ptr; ptr = ptr->next)
- {
PageSetLSN(ptr->page, recptr);
- }
/*
* Return the new child buffers to the caller.
@@ -544,7 +547,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
if (BufferIsValid(leftchildbuf))
MarkBufferDirty(leftchildbuf);
- if (RelationNeedsWAL(rel))
+
+ if (RelationNeedsWAL(rel) && !is_build)
{
OffsetNumber ndeloffs = 0,
deloffs[1];
@@ -567,6 +571,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
PageSetLSN(page, recptr);
}
+
if (newblkno)
*newblkno = blkno;
}
@@ -583,17 +588,28 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
* the full page image. There's a chicken-and-egg problem: if we updated
* the child pages first, we wouldn't know the recptr of the WAL record
* we're about to write.
+ *
+ * We use fakeLSNs for inserions caused by index build. And when it is
+ * finished, we write generic_xlog entry for each index page and update
+ * all LSNs. In order to keep NSNs less then LSNs after this update, we
+ * set NSN to InvalidXLogRecPtr, which is the smallest possible NSN.
*/
+
if (BufferIsValid(leftchildbuf))
{
Page leftpg = BufferGetPage(leftchildbuf);
+ XLogRecPtr fakerecptr = InvalidXLogRecPtr;
- GistPageSetNSN(leftpg, recptr);
- GistClearFollowRight(leftpg);
+ if (!is_build)
+ GistPageSetNSN(leftpg, recptr);
+ else
+ GistPageSetNSN(leftpg, fakerecptr);
+ GistClearFollowRight(leftpg);
PageSetLSN(leftpg, recptr);
}
+
END_CRIT_SECTION();
return is_split;
@@ -606,7 +622,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
*/
void
gistdoinsert(Relation r, IndexTuple itup, Size freespace,
- GISTSTATE *giststate, Relation heapRel)
+ GISTSTATE *giststate, Relation heapRel, bool is_build)
{
ItemId iid;
IndexTuple idxtuple;
@@ -619,6 +635,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
state.freespace = freespace;
state.r = r;
state.heapRel = heapRel;
+ state.is_build = is_build;
/* Start from the root */
firststack.blkno = GIST_ROOT_BLKNO;
@@ -1251,7 +1268,8 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
leftchild,
&splitinfo,
true,
- state->heapRel);
+ state->heapRel,
+ state->is_build);
/*
* Before recursing up in case the page was split, release locks on the
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 3652fde5bb..8d0d285cab 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -17,6 +17,7 @@
#include <math.h>
#include "access/genam.h"
+#include "access/generic_xlog.h"
#include "access/gist_private.h"
#include "access/gistxlog.h"
#include "access/tableam.h"
@@ -181,18 +182,12 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
MarkBufferDirty(buffer);
- if (RelationNeedsWAL(index))
- {
- XLogRecPtr recptr;
-
- XLogBeginInsert();
- XLogRegisterBuffer(0, buffer, REGBUF_WILL_INIT);
-
- recptr = XLogInsert(RM_GIST_ID, XLOG_GIST_CREATE_INDEX);
- PageSetLSN(page, recptr);
- }
- else
- PageSetLSN(page, gistGetFakeLSN(heap));
+ /*
+ * Do not write index pages to WAL unitl index build is finished.
+ * But we still need increasing LSNs on each page, so use FakeLSN,
+ * even for relations which eventually need WAL.
+ */
+ PageSetLSN(page, gistGetFakeLSN(heap));
UnlockReleaseBuffer(buffer);
@@ -226,6 +221,15 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
freeGISTstate(buildstate.giststate);
+ /*
+ * Create generic wal records for all pages of relation, if necessary.
+ * It seems reasonable not to generate WAL, if we recieved interrupt
+ * signal.
+ */
+ CHECK_FOR_INTERRUPTS();
+ if (RelationNeedsWAL(index))
+ generic_log_relation(index);
+
/*
* Return statistics
*/
@@ -488,7 +492,7 @@ gistBuildCallback(Relation index,
* locked, we call gistdoinsert directly.
*/
gistdoinsert(index, itup, buildstate->freespace,
- buildstate->giststate, buildstate->heaprel);
+ buildstate->giststate, buildstate->heaprel, true);
}
/* Update tuple count and total size. */
@@ -695,7 +699,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
InvalidBuffer,
&splitinfo,
false,
- buildstate->heaprel);
+ buildstate->heaprel, true);
/*
* If this is a root split, update the root path item kept in memory. This
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 2163cc482d..af278e5ded 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -1004,6 +1004,7 @@ gistproperty(Oid index_oid, int attno,
* Temporary and unlogged GiST indexes are not WAL-logged, but we need LSNs
* to detect concurrent page splits anyway. This function provides a fake
* sequence of LSNs for that purpose.
+ * Persistent relations are also not WAL-logged while we build index.
*/
XLogRecPtr
gistGetFakeLSN(Relation rel)
@@ -1024,7 +1025,6 @@ gistGetFakeLSN(Relation rel)
* Unlogged relations are accessible from other backends, and survive
* (clean) restarts. GetFakeLSNForUnloggedRel() handles that for us.
*/
- Assert(rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
return GetFakeLSNForUnloggedRel();
}
}
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index cb80ab00cd..4fb1855e89 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -490,25 +490,6 @@ gistRedoPageSplitRecord(XLogReaderState *record)
UnlockReleaseBuffer(firstbuffer);
}
-static void
-gistRedoCreateIndex(XLogReaderState *record)
-{
- XLogRecPtr lsn = record->EndRecPtr;
- Buffer buffer;
- Page page;
-
- buffer = XLogInitBufferForRedo(record, 0);
- Assert(BufferGetBlockNumber(buffer) == GIST_ROOT_BLKNO);
- page = (Page) BufferGetPage(buffer);
-
- GISTInitBuffer(buffer, F_LEAF);
-
- PageSetLSN(page, lsn);
-
- MarkBufferDirty(buffer);
- UnlockReleaseBuffer(buffer);
-}
-
/* redo page deletion */
static void
gistRedoPageDelete(XLogReaderState *record)
@@ -594,9 +575,6 @@ gist_redo(XLogReaderState *record)
case XLOG_GIST_PAGE_SPLIT:
gistRedoPageSplitRecord(record);
break;
- case XLOG_GIST_CREATE_INDEX:
- gistRedoCreateIndex(record);
- break;
case XLOG_GIST_PAGE_DELETE:
gistRedoPageDelete(record);
break;
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index 3ff4f83d38..eb308c72d6 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -71,8 +71,6 @@ gist_desc(StringInfo buf, XLogReaderState *record)
case XLOG_GIST_PAGE_SPLIT:
out_gistxlogPageSplit(buf, (gistxlogPageSplit *) rec);
break;
- case XLOG_GIST_CREATE_INDEX:
- break;
case XLOG_GIST_PAGE_DELETE:
out_gistxlogPageDelete(buf, (gistxlogPageDelete *) rec);
break;
@@ -98,9 +96,6 @@ gist_identify(uint8 info)
case XLOG_GIST_PAGE_SPLIT:
id = "PAGE_SPLIT";
break;
- case XLOG_GIST_CREATE_INDEX:
- id = "CREATE_INDEX";
- break;
case XLOG_GIST_PAGE_DELETE:
id = "PAGE_DELETE";
break;
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 02dc285a78..78e2e3fb31 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -244,6 +244,7 @@ typedef struct
Relation r;
Relation heapRel;
Size freespace; /* free space to be left */
+ bool is_build;
GISTInsertStack *stack;
} GISTInsertState;
@@ -393,7 +394,8 @@ extern void gistdoinsert(Relation r,
IndexTuple itup,
Size freespace,
GISTSTATE *GISTstate,
- Relation heapRel);
+ Relation heapRel,
+ bool is_build);
/* A List of these is returned from gistplacetopage() in *splitinfo */
typedef struct
@@ -409,7 +411,8 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
Buffer leftchildbuf,
List **splitinfo,
bool markleftchild,
- Relation heapRel);
+ Relation heapRel,
+ bool is_build);
extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
int len, GISTSTATE *giststate);
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 2f87b67a53..80931497ca 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -23,7 +23,6 @@
* FSM */
#define XLOG_GIST_PAGE_SPLIT 0x30
/* #define XLOG_GIST_INSERT_COMPLETE 0x40 */ /* not used anymore */
-#define XLOG_GIST_CREATE_INDEX 0x50
#define XLOG_GIST_PAGE_DELETE 0x60
/*
--
2.17.1
--------------5B85550EC82B346D9FBFC34E
Content-Type: text/x-patch;
name="v2_0004-SP-GiST-Optimal-WAL-Usage.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="v2_0004-SP-GiST-Optimal-WAL-Usage.patch"
^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH 3/4] GiST-Optimal-WAL-Usage
@ 2019-04-02 04:43 Andrey V. Lepikhov <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Andrey V. Lepikhov @ 2019-04-02 04:43 UTC (permalink / raw)
---
src/backend/access/gist/gist.c | 46 ++++++++++++++++++--------
src/backend/access/gist/gistbuild.c | 32 ++++++++++--------
src/backend/access/gist/gistutil.c | 2 +-
src/backend/access/gist/gistxlog.c | 22 ------------
src/backend/access/rmgrdesc/gistdesc.c | 5 ---
src/include/access/gist_private.h | 7 ++--
src/include/access/gistxlog.h | 1 -
7 files changed, 56 insertions(+), 59 deletions(-)
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 2fddb23496..0e2b6c3014 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -172,7 +172,7 @@ gistinsert(Relation r, Datum *values, bool *isnull,
values, isnull, true /* size is currently bogus */ );
itup->t_tid = *ht_ctid;
- gistdoinsert(r, itup, 0, giststate, heapRel);
+ gistdoinsert(r, itup, 0, giststate, heapRel, false);
/* cleanup */
MemoryContextSwitchTo(oldCxt);
@@ -219,7 +219,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
Buffer leftchildbuf,
List **splitinfo,
bool markfollowright,
- Relation heapRel)
+ Relation heapRel,
+ bool is_build)
{
BlockNumber blkno = BufferGetBlockNumber(buffer);
Page page = BufferGetPage(buffer);
@@ -458,7 +459,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
* insertion for that. NB: The number of pages and data segments
* specified here must match the calculations in gistXLogSplit()!
*/
- if (RelationNeedsWAL(rel))
+ if (RelationNeedsWAL(rel) && !is_build)
XLogEnsureRecordSpace(npage, 1 + npage * 2);
START_CRIT_SECTION();
@@ -479,18 +480,20 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
PageRestoreTempPage(dist->page, BufferGetPage(dist->buffer));
dist->page = BufferGetPage(dist->buffer);
- /* Write the WAL record */
- if (RelationNeedsWAL(rel))
+ /*
+ * Write the WAL record.
+ * Do not write XLog entry if the insertion is caused by
+ * index build process.
+ */
+ if (RelationNeedsWAL(rel) && !is_build)
recptr = gistXLogSplit(is_leaf,
- dist, oldrlink, oldnsn, leftchildbuf,
- markfollowright);
+ dist, oldrlink, oldnsn, leftchildbuf,
+ markfollowright);
else
recptr = gistGetFakeLSN(rel);
for (ptr = dist; ptr; ptr = ptr->next)
- {
PageSetLSN(ptr->page, recptr);
- }
/*
* Return the new child buffers to the caller.
@@ -544,7 +547,8 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
if (BufferIsValid(leftchildbuf))
MarkBufferDirty(leftchildbuf);
- if (RelationNeedsWAL(rel))
+
+ if (RelationNeedsWAL(rel) && !is_build)
{
OffsetNumber ndeloffs = 0,
deloffs[1];
@@ -567,6 +571,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
PageSetLSN(page, recptr);
}
+
if (newblkno)
*newblkno = blkno;
}
@@ -583,17 +588,28 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
* the full page image. There's a chicken-and-egg problem: if we updated
* the child pages first, we wouldn't know the recptr of the WAL record
* we're about to write.
+ *
+ * We use fakeLSNs for inserions caused by index build. And when it is
+ * finished, we write generic_xlog entry for each index page and update
+ * all LSNs. In order to keep NSNs less then LSNs after this update, we
+ * set NSN to InvalidXLogRecPtr, which is the smallest possible NSN.
*/
+
if (BufferIsValid(leftchildbuf))
{
Page leftpg = BufferGetPage(leftchildbuf);
+ XLogRecPtr fakerecptr = InvalidXLogRecPtr;
- GistPageSetNSN(leftpg, recptr);
- GistClearFollowRight(leftpg);
+ if (!is_build)
+ GistPageSetNSN(leftpg, recptr);
+ else
+ GistPageSetNSN(leftpg, fakerecptr);
+ GistClearFollowRight(leftpg);
PageSetLSN(leftpg, recptr);
}
+
END_CRIT_SECTION();
return is_split;
@@ -606,7 +622,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
*/
void
gistdoinsert(Relation r, IndexTuple itup, Size freespace,
- GISTSTATE *giststate, Relation heapRel)
+ GISTSTATE *giststate, Relation heapRel, bool is_build)
{
ItemId iid;
IndexTuple idxtuple;
@@ -619,6 +635,7 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
state.freespace = freespace;
state.r = r;
state.heapRel = heapRel;
+ state.is_build = is_build;
/* Start from the root */
firststack.blkno = GIST_ROOT_BLKNO;
@@ -1251,7 +1268,8 @@ gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
leftchild,
&splitinfo,
true,
- state->heapRel);
+ state->heapRel,
+ state->is_build);
/*
* Before recursing up in case the page was split, release locks on the
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 3652fde5bb..aa05c0a8ee 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -17,6 +17,7 @@
#include <math.h>
#include "access/genam.h"
+#include "access/generic_xlog.h"
#include "access/gist_private.h"
#include "access/gistxlog.h"
#include "access/tableam.h"
@@ -181,18 +182,12 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
MarkBufferDirty(buffer);
- if (RelationNeedsWAL(index))
- {
- XLogRecPtr recptr;
-
- XLogBeginInsert();
- XLogRegisterBuffer(0, buffer, REGBUF_WILL_INIT);
-
- recptr = XLogInsert(RM_GIST_ID, XLOG_GIST_CREATE_INDEX);
- PageSetLSN(page, recptr);
- }
- else
- PageSetLSN(page, gistGetFakeLSN(heap));
+ /*
+ * Do not write index pages to WAL unitl index build is finished.
+ * But we still need increasing LSNs on each page, so use FakeLSN,
+ * even for relations which eventually need WAL.
+ */
+ PageSetLSN(page, gistGetFakeLSN(heap));
UnlockReleaseBuffer(buffer);
@@ -226,6 +221,15 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
freeGISTstate(buildstate.giststate);
+ /*
+ * Create generic wal records for all pages of relation, if necessary.
+ * It seems reasonable not to generate WAL, if we recieved interrupt
+ * signal.
+ */
+ CHECK_FOR_INTERRUPTS();
+ if (RelationNeedsWAL(index))
+ log_relation(index);
+
/*
* Return statistics
*/
@@ -488,7 +492,7 @@ gistBuildCallback(Relation index,
* locked, we call gistdoinsert directly.
*/
gistdoinsert(index, itup, buildstate->freespace,
- buildstate->giststate, buildstate->heaprel);
+ buildstate->giststate, buildstate->heaprel, true);
}
/* Update tuple count and total size. */
@@ -695,7 +699,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
InvalidBuffer,
&splitinfo,
false,
- buildstate->heaprel);
+ buildstate->heaprel, true);
/*
* If this is a root split, update the root path item kept in memory. This
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 2163cc482d..af278e5ded 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -1004,6 +1004,7 @@ gistproperty(Oid index_oid, int attno,
* Temporary and unlogged GiST indexes are not WAL-logged, but we need LSNs
* to detect concurrent page splits anyway. This function provides a fake
* sequence of LSNs for that purpose.
+ * Persistent relations are also not WAL-logged while we build index.
*/
XLogRecPtr
gistGetFakeLSN(Relation rel)
@@ -1024,7 +1025,6 @@ gistGetFakeLSN(Relation rel)
* Unlogged relations are accessible from other backends, and survive
* (clean) restarts. GetFakeLSNForUnloggedRel() handles that for us.
*/
- Assert(rel->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
return GetFakeLSNForUnloggedRel();
}
}
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index cb80ab00cd..4fb1855e89 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -490,25 +490,6 @@ gistRedoPageSplitRecord(XLogReaderState *record)
UnlockReleaseBuffer(firstbuffer);
}
-static void
-gistRedoCreateIndex(XLogReaderState *record)
-{
- XLogRecPtr lsn = record->EndRecPtr;
- Buffer buffer;
- Page page;
-
- buffer = XLogInitBufferForRedo(record, 0);
- Assert(BufferGetBlockNumber(buffer) == GIST_ROOT_BLKNO);
- page = (Page) BufferGetPage(buffer);
-
- GISTInitBuffer(buffer, F_LEAF);
-
- PageSetLSN(page, lsn);
-
- MarkBufferDirty(buffer);
- UnlockReleaseBuffer(buffer);
-}
-
/* redo page deletion */
static void
gistRedoPageDelete(XLogReaderState *record)
@@ -594,9 +575,6 @@ gist_redo(XLogReaderState *record)
case XLOG_GIST_PAGE_SPLIT:
gistRedoPageSplitRecord(record);
break;
- case XLOG_GIST_CREATE_INDEX:
- gistRedoCreateIndex(record);
- break;
case XLOG_GIST_PAGE_DELETE:
gistRedoPageDelete(record);
break;
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index 3ff4f83d38..eb308c72d6 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -71,8 +71,6 @@ gist_desc(StringInfo buf, XLogReaderState *record)
case XLOG_GIST_PAGE_SPLIT:
out_gistxlogPageSplit(buf, (gistxlogPageSplit *) rec);
break;
- case XLOG_GIST_CREATE_INDEX:
- break;
case XLOG_GIST_PAGE_DELETE:
out_gistxlogPageDelete(buf, (gistxlogPageDelete *) rec);
break;
@@ -98,9 +96,6 @@ gist_identify(uint8 info)
case XLOG_GIST_PAGE_SPLIT:
id = "PAGE_SPLIT";
break;
- case XLOG_GIST_CREATE_INDEX:
- id = "CREATE_INDEX";
- break;
case XLOG_GIST_PAGE_DELETE:
id = "PAGE_DELETE";
break;
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 02dc285a78..78e2e3fb31 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -244,6 +244,7 @@ typedef struct
Relation r;
Relation heapRel;
Size freespace; /* free space to be left */
+ bool is_build;
GISTInsertStack *stack;
} GISTInsertState;
@@ -393,7 +394,8 @@ extern void gistdoinsert(Relation r,
IndexTuple itup,
Size freespace,
GISTSTATE *GISTstate,
- Relation heapRel);
+ Relation heapRel,
+ bool is_build);
/* A List of these is returned from gistplacetopage() in *splitinfo */
typedef struct
@@ -409,7 +411,8 @@ extern bool gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
Buffer leftchildbuf,
List **splitinfo,
bool markleftchild,
- Relation heapRel);
+ Relation heapRel,
+ bool is_build);
extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
int len, GISTSTATE *giststate);
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 2f87b67a53..80931497ca 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -23,7 +23,6 @@
* FSM */
#define XLOG_GIST_PAGE_SPLIT 0x30
/* #define XLOG_GIST_INSERT_COMPLETE 0x40 */ /* not used anymore */
-#define XLOG_GIST_CREATE_INDEX 0x50
#define XLOG_GIST_PAGE_DELETE 0x60
/*
--
2.17.1
--------------58E4AA393B5F48ED79D1D18E
Content-Type: text/x-patch;
name="v3_0004-SP-GiST-Optimal-WAL-Usage.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="v3_0004-SP-GiST-Optimal-WAL-Usage.patch"
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
@ 2023-06-28 09:37 David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: David Rowley @ 2023-06-28 09:37 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Melih Mutlu <[email protected]>; pgsql-hackers
On Wed, 28 Jun 2023 at 20:13, Peter Eisentraut <[email protected]> wrote:
> size_t (= Size) is the correct type in C to store the size of an object
> in memory. This is partially a self-documentation issue: If I see
> size_t in a function signature, I know what is intended; if I see
> uint32, I have to wonder what the intent was.
Perhaps it's ok to leave the context creation functions with Size
typed parameters and then just Assert the passed-in sizes are not
larger than 1GB within the context creation function. That way we
could keep this change self contained in the .c file for the given
memory context. That would mean there's no less readability. If we
ever wanted to lift the 1GB limit on block sizes then we'd not need to
switch the function signature again. There's documentation where the
struct's field is declared, so having a smaller type in the struct
itself does not seem like a reduction of documentation quality.
> You could make an argument that using shorter types would save space for
> some internal structs, but then you'd have to show some more information
> where and why that would be beneficial.
I think there's not much need to go proving this speeds something up.
There's just simply no point in the struct fields being changed in
Melih's patch to be bigger than 32 bits as we never need to store more
than 1GB in them. Reducing these down means we may have to touch
fewer cache lines and we'll also have more space on the keeper blocks
to store allocations. Memory allocation performance is fairly
fundamental to Postgres's performance. In my view, we shouldn't have
fields that are twice as large as they need to be in code as hot as
this.
> Absent any strong performance argument, I don't see the benefit of this
> change. People might well want to experiment with MEMORYCHUNK_...
> settings larger than 1GB.
Anyone doing so will be editing C code anyway. They can adjust these
fields then.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
@ 2023-06-28 10:59 ` Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2023-07-10 14:41 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
0 siblings, 2 replies; 20+ messages in thread
From: Tom Lane @ 2023-06-28 10:59 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
David Rowley <[email protected]> writes:
> Perhaps it's ok to leave the context creation functions with Size
> typed parameters and then just Assert the passed-in sizes are not
> larger than 1GB within the context creation function.
Yes, I'm strongly opposed to not using Size/size_t in the mmgr APIs.
If we go that road, we're going to have a problem when someone
inevitably wants to pass a larger-than-GB value for some context
type.
What happens in semi-private structs is a different matter, although
I'm a little dubious that shaving a couple of bytes from context
headers is a useful activity. The self-documentation argument
still has some force there, so I agree with Peter that some positive
benefit has to be shown.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
@ 2023-06-28 21:26 ` Tomas Vondra <[email protected]>
2023-06-28 21:56 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 23:34 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
2023-06-29 03:46 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
1 sibling, 3 replies; 20+ messages in thread
From: Tomas Vondra @ 2023-06-28 21:26 UTC (permalink / raw)
To: Tom Lane <[email protected]>; David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
On 6/28/23 12:59, Tom Lane wrote:
> David Rowley <[email protected]> writes:
>> Perhaps it's ok to leave the context creation functions with Size
>> typed parameters and then just Assert the passed-in sizes are not
>> larger than 1GB within the context creation function.
>
> Yes, I'm strongly opposed to not using Size/size_t in the mmgr APIs.
> If we go that road, we're going to have a problem when someone
> inevitably wants to pass a larger-than-GB value for some context
> type.
+1
> What happens in semi-private structs is a different matter, although
> I'm a little dubious that shaving a couple of bytes from context
> headers is a useful activity. The self-documentation argument
> still has some force there, so I agree with Peter that some positive
> benefit has to be shown.
>
Yeah. FWIW I was interested what the patch does in practice, so I
checked what pahole says about impact on struct sizes:
AllocSetContext 224B -> 208B (4 cachelines)
GenerationContext 152B -> 136B (3 cachelines)
SlabContext 200B -> 200B (no change, adds 4B hole)
Nothing else changes, AFAICS. I find it hard to believe this could have
any sort of positive benefit - I doubt we ever have enough contexts for
this to matter.
When I first saw the patch I was thinking it's probably changing how we
store the per-chunk requested_size. Maybe that'd make a difference,
although 4B is tiny compared to what we waste due to the doubling.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
@ 2023-06-28 21:56 ` Tom Lane <[email protected]>
2023-06-28 23:42 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
2 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2023-06-28 21:56 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
Tomas Vondra <[email protected]> writes:
> ... 4B is tiny compared to what we waste due to the doubling.
Yeah. I've occasionally wondered if we should rethink aset.c's
"only power-of-2 chunk sizes" rule. Haven't had the bandwidth
to pursue the idea though.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2023-06-28 21:56 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
@ 2023-06-28 23:42 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Andres Freund @ 2023-06-28 23:42 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Tomas Vondra <[email protected]>; David Rowley <[email protected]>; Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
Hi,
On 2023-06-28 17:56:55 -0400, Tom Lane wrote:
> Tomas Vondra <[email protected]> writes:
> > ... 4B is tiny compared to what we waste due to the doubling.
>
> Yeah. I've occasionally wondered if we should rethink aset.c's
> "only power-of-2 chunk sizes" rule. Haven't had the bandwidth
> to pursue the idea though.
Me too. It'd not be trivial to do without also incurring performance overhead.
A somewhat easier thing we could try is to carve the "rounding up" space into
smaller chunks, similar to what we do for full blocks. It wouldn't make sense
to do that for the smaller size classes, but above 64-256 bytes or such, I
think the wins might be big enough to outweight the costs?
Of course that doesn't guarantee that that memory in those smaller size
classes is going to be used...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
@ 2023-06-28 23:34 ` Andres Freund <[email protected]>
2023-06-29 09:58 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2 siblings, 1 reply; 20+ messages in thread
From: Andres Freund @ 2023-06-28 23:34 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Tom Lane <[email protected]>; David Rowley <[email protected]>; Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
Hi,
On 2023-06-28 23:26:00 +0200, Tomas Vondra wrote:
> Yeah. FWIW I was interested what the patch does in practice, so I
> checked what pahole says about impact on struct sizes:
>
> AllocSetContext 224B -> 208B (4 cachelines)
> GenerationContext 152B -> 136B (3 cachelines)
> SlabContext 200B -> 200B (no change, adds 4B hole)
>
> Nothing else changes, AFAICS. I find it hard to believe this could have
> any sort of positive benefit - I doubt we ever have enough contexts for
> this to matter.
I don't think it's that hard to believe. We create a lot of memory contexts
that we never or just barely use. Just reducing the number of cachelines
touched for that can't hurt. This does't quite get us to reducing the size to
a lower number of cachelines, but it's a good step.
There are a few other fields that we can get rid of.
- Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
together with the context itself. Saves 8 bytes.
- The set of memory context types isn't runtime extensible. We could replace
MemoryContextData->methods with a small integer index into mcxt_methods. I
think that might actually end up being as-cheap or even cheaper than the
current approach. Saves 8 bytes.
Tthat's sufficient for going to 3 cachelines.
- We could store the power of 2 for initBlockSize, nextBlockSize,
maxBlockSize, instead of the "raw" value. That'd make them one byte
each. Which also would get rid of the concerns around needing a
"mini_size_t" type.
- freeListIndex could be a single byte as well (saving 7 bytes, as right now
we loose 4 trailing bytes due to padding).
That would save another 12 bytes, if I calculate correctly. 25% shrinkage
together ain't bad.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2023-06-28 23:34 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
@ 2023-06-29 09:58 ` Tomas Vondra <[email protected]>
2023-06-30 00:29 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Tomas Vondra @ 2023-06-29 09:58 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; David Rowley <[email protected]>; Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
On 6/29/23 01:34, Andres Freund wrote:
> Hi,
>
> On 2023-06-28 23:26:00 +0200, Tomas Vondra wrote:
>> Yeah. FWIW I was interested what the patch does in practice, so I
>> checked what pahole says about impact on struct sizes:
>>
>> AllocSetContext 224B -> 208B (4 cachelines)
>> GenerationContext 152B -> 136B (3 cachelines)
>> SlabContext 200B -> 200B (no change, adds 4B hole)
>>
>> Nothing else changes, AFAICS. I find it hard to believe this could have
>> any sort of positive benefit - I doubt we ever have enough contexts for
>> this to matter.
>
> I don't think it's that hard to believe. We create a lot of memory contexts
> that we never or just barely use. Just reducing the number of cachelines
> touched for that can't hurt. This does't quite get us to reducing the size to
> a lower number of cachelines, but it's a good step.
>
> There are a few other fields that we can get rid of.
>
> - Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
> together with the context itself. Saves 8 bytes.
>
> - The set of memory context types isn't runtime extensible. We could replace
> MemoryContextData->methods with a small integer index into mcxt_methods. I
> think that might actually end up being as-cheap or even cheaper than the
> current approach. Saves 8 bytes.
>
> Tthat's sufficient for going to 3 cachelines.
>
>
> - We could store the power of 2 for initBlockSize, nextBlockSize,
> maxBlockSize, instead of the "raw" value. That'd make them one byte
> each. Which also would get rid of the concerns around needing a
> "mini_size_t" type.
>
> - freeListIndex could be a single byte as well (saving 7 bytes, as right now
> we loose 4 trailing bytes due to padding).
>
> That would save another 12 bytes, if I calculate correctly. 25% shrinkage
> together ain't bad.
>
I don't oppose these changes, but I still don't quite believe it'll make
a measurable difference (even if we manage to save a cacheline or two).
I'd definitely like to see some measurements demonstrating it's worth
the extra complexity.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2023-06-28 23:34 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
2023-06-29 09:58 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
@ 2023-06-30 00:29 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Andres Freund @ 2023-06-30 00:29 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Tom Lane <[email protected]>; David Rowley <[email protected]>; Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
Hi,
On 2023-06-29 11:58:27 +0200, Tomas Vondra wrote:
> On 6/29/23 01:34, Andres Freund wrote:
> > On 2023-06-28 23:26:00 +0200, Tomas Vondra wrote:
> >> Yeah. FWIW I was interested what the patch does in practice, so I
> >> checked what pahole says about impact on struct sizes:
> >>
> >> AllocSetContext 224B -> 208B (4 cachelines)
> >> GenerationContext 152B -> 136B (3 cachelines)
> >> SlabContext 200B -> 200B (no change, adds 4B hole)
> ...
> > That would save another 12 bytes, if I calculate correctly. 25% shrinkage
> > together ain't bad.
> >
>
> I don't oppose these changes, but I still don't quite believe it'll make
> a measurable difference (even if we manage to save a cacheline or two).
> I'd definitely like to see some measurements demonstrating it's worth
> the extra complexity.
I hacked (emphasis on that) a version together that shrinks AllocSetContext
down to 176 bytes.
There seem to be some minor performance gains, and some not too shabby memory
savings.
E.g. a backend after running readonly pgbench goes from (results repeat
precisely across runs):
pgbench: Grand total: 1361528 bytes in 289 blocks; 367480 free (206 chunks); 994048 used
to:
pgbench: Grand total: 1339000 bytes in 278 blocks; 352352 free (188 chunks); 986648 used
Running a total over all connections in the main regression tests gives less
of a win (best of three):
backends grand blocks free chunks used
690 1046956664 111373 370680728 291436 676275936
to:
backends grand blocks free chunks used
690 1045226056 111099 372972120 297969 672253936
the latter is produced with this beauty:
ninja && m test --suite setup --no-rebuild && m test --no-rebuild --print-errorlogs regress/regress -v && grep "Grand total" testrun/regress/regress/log/postmaster.log|sed -E -e 's/.*Grand total: (.*) bytes in (.*) blocks; (.*) free \((.*) chunks\); (.*) used/\1\t\2\t\3\t\4\t\5/'|awk '{backends += 1; grand += $1; blocks += $2; free += $3; chunks += $4; used += $5} END{print backends, grand, blocks, free, chunks, used}'
There's more to get. The overhead of AllocSetBlock also plays into this. Both
due to the keeper block and obviously separate blocks getting allocated
subsequently. We e.g. don't need AllocBlockData->next,prev as 8 byte pointers
(some trickiness would be required for external blocks, but they could combine
both).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
@ 2023-06-29 03:46 ` David Rowley <[email protected]>
2 siblings, 0 replies; 20+ messages in thread
From: David Rowley @ 2023-06-29 03:46 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Melih Mutlu <[email protected]>; pgsql-hackers
On Thu, 29 Jun 2023 at 09:26, Tomas Vondra
<[email protected]> wrote:
> AllocSetContext 224B -> 208B (4 cachelines)
> GenerationContext 152B -> 136B (3 cachelines)
> SlabContext 200B -> 200B (no change, adds 4B hole)
>
> Nothing else changes, AFAICS.
I don't think a lack of a reduction in the number of cache lines is
the important part. Allowing more space on the keeper block, which is
at the end of the context struct seems more useful. I understand that
the proposal is just to shave off 12 bytes and that's not exactly huge
when it's just once per context, but we do create quite a large number
of contexts with ALLOCSET_SMALL_SIZES which have a 1KB initial block
size. 12 bytes in 1024 is not terrible.
It's not exactly an invasive change. It does not add any complexity
to the code and as far as I can see, about zero risk of it slowing
anything down.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
@ 2023-07-10 14:41 ` Melih Mutlu <[email protected]>
2023-07-13 05:03 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
1 sibling, 1 reply; 20+ messages in thread
From: Melih Mutlu @ 2023-07-10 14:41 UTC (permalink / raw)
To: Tom Lane <[email protected]>; David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>
Hi,
Thanks for your comments.
Tom Lane <[email protected]>, 28 Haz 2023 Çar, 13:59 tarihinde şunu yazdı:
>
> David Rowley <[email protected]> writes:
> > Perhaps it's ok to leave the context creation functions with Size
> > typed parameters and then just Assert the passed-in sizes are not
> > larger than 1GB within the context creation function.
>
> Yes, I'm strongly opposed to not using Size/size_t in the mmgr APIs.
> If we go that road, we're going to have a problem when someone
> inevitably wants to pass a larger-than-GB value for some context
> type.
I reverted changes in the context creation functions and only changed
the types in structs.
I believe there are already lines to assert whether the sizes are less
than 1GB, so we should be safe there.
Andres Freund <[email protected]>, 29 Haz 2023 Per, 02:34 tarihinde şunu yazdı:
> There are a few other fields that we can get rid of.
>
> - Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
> together with the context itself. Saves 8 bytes.
This seemed like a safe change and removed the keeper field in
AllocSet and Generation contexts. It saves an additional 8 bytes.
Best,
--
Melih Mutlu
Microsoft
Attachments:
[application/octet-stream] v2-0001-Change-memory-context-fields-to-uint32.patch (12.1K, ../../CAGPVpCS_+nQQgRzsqcUq_ESaj4MtVZ53G=NR5zYu-g9B5oa4_w@mail.gmail.com/2-v2-0001-Change-memory-context-fields-to-uint32.patch)
download | inline diff:
From b9669a832576ca7fb941c727e6ff46adb3fdcdbf Mon Sep 17 00:00:00 2001
From: Melih Mutlu <[email protected]>
Date: Mon, 12 Jun 2023 08:50:28 +0300
Subject: [PATCH v2] Change memory context fields to uint32
Block sizes and chunk sizes can be upper bounded by
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE respectively.
Values of both bounds correspond to 1 less than 1GB. This allows us
to store the sizes of any block or chunk size limited by those bounds
in 32 bits.
This patch changes types of such fields that represents block or chunk sizes
from 64-bit Size to 32-bit unsigned integers.
Also; keeper fields from AllocSet and Generation structs are removed and
added AllocSetKeeper/GenerationKeeper macros where needed.
---
src/backend/utils/mmgr/aset.c | 52 +++++++++++++++++------------
src/backend/utils/mmgr/generation.c | 51 ++++++++++++++++------------
src/backend/utils/mmgr/slab.c | 6 ++--
3 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0bbbf93672..797abef1b5 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -156,11 +156,10 @@ typedef struct AllocSetContext
AllocBlock blocks; /* head of list of blocks in this set */
MemoryChunk *freelist[ALLOCSET_NUM_FREELISTS]; /* free chunk lists */
/* Allocation parameters for this context: */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
- AllocBlock keeper; /* keep this block over resets */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
} AllocSetContext;
@@ -241,6 +240,14 @@ typedef struct AllocBlockData
*/
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
+/* Obtain the keeper block for an allocation set */
+#define AllocSetKeeper(set) \
+ ((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
+
+/* Check if the block is the keeper block of the given allocation set */
+#define IsKeeperBlock(set, block) \
+ ((block) == (AllocSetKeeper(set)))
+
typedef struct AllocSetFreeList
{
int num_free; /* current list length */
@@ -345,7 +352,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
Size maxBlockSize)
{
int freeListIndex;
- Size firstBlockSize;
+ uint32 firstBlockSize;
AllocSet set;
AllocBlock block;
@@ -417,7 +424,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- set->keeper->endptr - ((char *) set);
+ AllocSetKeeper(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -452,8 +459,11 @@ AllocSetContextCreateInternal(MemoryContext parent,
* we'd leak the header/initial block if we ereport in this stretch.
*/
- /* Fill in the initial block's block header */
- block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = AllocSetKeeper(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -465,15 +475,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Remember block as part of block list */
set->blocks = block;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -544,7 +552,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -552,13 +560,13 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = set->keeper;
+ set->blocks = AllocSetKeeper(set);
while (block != NULL)
{
AllocBlock next = block->next;
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
/* Reset the block, but don't return it to malloc */
char *datastart = ((char *) block) + ALLOC_BLOCKHDRSZ;
@@ -614,7 +622,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
@@ -663,14 +671,14 @@ AllocSetDelete(MemoryContext context)
{
AllocBlock next = block->next;
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
context->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
free(block);
block = next;
@@ -1547,7 +1555,7 @@ AllocSetCheck(MemoryContext context)
long nchunks = 0;
bool has_external_chunk = false;
- if (set->keeper == block)
+ if (IsKeeperBlock(set, block))
total_allocated += block->endptr - ((char *) set);
else
total_allocated += block->endptr - ((char *) block);
@@ -1557,7 +1565,7 @@ AllocSetCheck(MemoryContext context)
*/
if (!blk_used)
{
- if (set->keeper != block)
+ if (!IsKeeperBlock(set, block))
elog(WARNING, "problem in alloc set %s: empty block %p",
name, block);
}
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 4fb8663cd6..d6a0811a72 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -61,17 +61,16 @@ typedef struct GenerationContext
MemoryContextData header; /* Standard memory-context fields */
/* Generational context parameters */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
GenerationBlock *block; /* current (most recently allocated) block, or
* NULL if we've just freed the most recent
* block */
GenerationBlock *freeblock; /* pointer to a block that's being recycled,
* or NULL if there's no such block. */
- GenerationBlock *keeper; /* keep this block over resets */
dlist_head blocks; /* list of blocks */
} GenerationContext;
@@ -120,6 +119,14 @@ struct GenerationBlock
#define ExternalChunkGetBlock(chunk) \
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
+/* Obtain the keeper block for a generation context */
+#define GenerationKeeper(set) \
+ ((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
+
+/* Check if the block is the keeper block of the given generation context */
+#define IsKeeperBlock(set, block) \
+ ((block) == (GenerationKeeper(set)))
+
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
GenerationBlock *block,
@@ -153,8 +160,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- Size firstBlockSize;
- Size allocSize;
+ uint32 firstBlockSize;
+ uint32 allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -213,8 +220,11 @@ GenerationContextCreate(MemoryContext parent,
*/
dlist_init(&set->blocks);
- /* Fill in the initial block's block header */
- block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = GenerationKeeper(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -228,13 +238,10 @@ GenerationContextCreate(MemoryContext parent,
/* No free block, yet */
set->freeblock = NULL;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
-
/* Fill in GenerationContext-specific header fields */
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -294,14 +301,14 @@ GenerationReset(MemoryContext context)
{
GenerationBlock *block = dlist_container(GenerationBlock, node, miter.cur);
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
GenerationBlockMarkEmpty(block);
else
GenerationBlockFree(set, block);
}
/* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
+ set->block = GenerationKeeper(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -440,10 +447,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(set->keeper) &&
- GenerationBlockFreeBytes(set->keeper) >= required_size)
+ else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
+ GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
{
- block = set->keeper;
+ block = GenerationKeeper(set);
}
else
{
@@ -594,7 +601,7 @@ static inline void
GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
{
/* Make sure nobody tries to free the keeper block */
- Assert(block != set->keeper);
+ Assert(!IsKeeperBlock(set, block));
/* We shouldn't be freeing the freeblock either */
Assert(block != set->freeblock);
@@ -691,7 +698,7 @@ GenerationFree(void *pointer)
set = block->context;
/* Don't try to free the keeper block, just mark it empty */
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
GenerationBlockMarkEmpty(block);
return;
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 718dd2ba03..d5e0edbede 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -104,7 +104,7 @@ typedef struct SlabContext
{
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
- Size chunkSize; /* the requested (non-aligned) chunk size */
+ uint32 chunkSize; /* the requested (non-aligned) chunk size */
Size fullChunkSize; /* chunk size with chunk header and alignment */
Size blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
@@ -374,7 +374,7 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = chunkSize;
+ slab->chunkSize = (Size) chunkSize;
slab->fullChunkSize = fullChunkSize;
slab->blockSize = blockSize;
slab->chunksPerBlock = chunksPerBlock;
@@ -506,7 +506,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (unlikely(size != slab->chunkSize))
- elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
size, slab->chunkSize);
/*
--
2.25.1
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-07-10 14:41 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
@ 2023-07-13 05:03 ` David Rowley <[email protected]>
2023-07-14 06:53 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: David Rowley @ 2023-07-13 05:03 UTC (permalink / raw)
To: Melih Mutlu <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>
On Tue, 11 Jul 2023 at 02:41, Melih Mutlu <[email protected]> wrote:
> > - Afaics AllocSet->keeper is unnecessary these days, as it is always allocated
> > together with the context itself. Saves 8 bytes.
>
> This seemed like a safe change and removed the keeper field in
> AllocSet and Generation contexts. It saves an additional 8 bytes.
Seems like a good idea for an additional 8-bytes.
I looked at your v2 patch. The only thing that really looked wrong
were the (Size) casts in the context creation functions. These should
have been casts to uint32 rather than Size. Basically, all the casts
do is say to the compiler "Yes, I know this could cause truncation due
to assigning to a size smaller than the source type's size". Some
compilers will likely warn without that and the cast will stop them.
We know there can't be any truncation due to the Asserts. There's also
the fundamental limitation that MemoryChunk can't store block offsets
larger than 1GBs anyway, so things will go bad if we tried to have
blocks bigger than 1GB.
Aside from that, I thought that a couple of other slab.c fields could
be shrunken to uint32 as the v2 patch just reduces the size of 1 field
which just creates a 4-byte hole in SlabContext. The fullChunkSize
field is just the MAXALIGN(chunkSize) + sizeof(MemoryChunk). We
should never be using slab contexts for any chunks anywhere near that
size. aset.c would be a better context for that, so it seems fine to
me to further restrict the maximum supported chunk size by another 8
bytes.
I've attached your patch again along with a small delta of what I adjusted.
My thoughts on these changes are that it's senseless to have Size
typed fields for storing a value that's never larger than 2^30.
Getting rid of the keeper pointer seems like a cleanup as it's pretty
much a redundant field. For small sized contexts like the ones used
for storing index relcache entries, I think it makes sense to save 20
more bytes. Each backend can have many thousand of those and there
could be many hundred backends. If we can fit more allocations on that
initial 1 kilobyte keeper block without having to allocate any
additional blocks, then that's great.
I feel that Andres's results showing several hundred fewer block
allocations shows this working. Albeit, his patch reduced the size of
the structs even further than what v3 does. I think v3 is enough for
now as the additional changes Andres mentioned require some more
invasive code changes to make work.
If nobody objects or has other ideas about this, modulo commit
message, I plan to push the attached on Monday.
David
From e5e7da6a9880b0cbc7fa96d26cdd73d8bb02c88f Mon Sep 17 00:00:00 2001
From: Melih Mutlu <[email protected]>
Date: Mon, 12 Jun 2023 08:50:28 +0300
Subject: [PATCH v3 1/2] Change memory context fields to uint32
Block sizes and chunk sizes can be upper bounded by
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE respectively.
Values of both bounds correspond to 1 less than 1GB. This allows us
to store the sizes of any block or chunk size limited by those bounds
in 32 bits.
This patch changes types of such fields that represents block or chunk sizes
from 64-bit Size to 32-bit unsigned integers.
Also; keeper fields from AllocSet and Generation structs are removed and
added AllocSetKeeper/GenerationKeeper macros where needed.
---
src/backend/utils/mmgr/aset.c | 52 +++++++++++++++++------------
src/backend/utils/mmgr/generation.c | 51 ++++++++++++++++------------
src/backend/utils/mmgr/slab.c | 6 ++--
3 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0bbbf93672..797abef1b5 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -156,11 +156,10 @@ typedef struct AllocSetContext
AllocBlock blocks; /* head of list of blocks in this set */
MemoryChunk *freelist[ALLOCSET_NUM_FREELISTS]; /* free chunk lists */
/* Allocation parameters for this context: */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
- AllocBlock keeper; /* keep this block over resets */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
} AllocSetContext;
@@ -241,6 +240,14 @@ typedef struct AllocBlockData
*/
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
+/* Obtain the keeper block for an allocation set */
+#define AllocSetKeeper(set) \
+ ((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
+
+/* Check if the block is the keeper block of the given allocation set */
+#define IsKeeperBlock(set, block) \
+ ((block) == (AllocSetKeeper(set)))
+
typedef struct AllocSetFreeList
{
int num_free; /* current list length */
@@ -345,7 +352,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
Size maxBlockSize)
{
int freeListIndex;
- Size firstBlockSize;
+ uint32 firstBlockSize;
AllocSet set;
AllocBlock block;
@@ -417,7 +424,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- set->keeper->endptr - ((char *) set);
+ AllocSetKeeper(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -452,8 +459,11 @@ AllocSetContextCreateInternal(MemoryContext parent,
* we'd leak the header/initial block if we ereport in this stretch.
*/
- /* Fill in the initial block's block header */
- block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = AllocSetKeeper(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -465,15 +475,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Remember block as part of block list */
set->blocks = block;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -544,7 +552,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -552,13 +560,13 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = set->keeper;
+ set->blocks = AllocSetKeeper(set);
while (block != NULL)
{
AllocBlock next = block->next;
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
/* Reset the block, but don't return it to malloc */
char *datastart = ((char *) block) + ALLOC_BLOCKHDRSZ;
@@ -614,7 +622,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
@@ -663,14 +671,14 @@ AllocSetDelete(MemoryContext context)
{
AllocBlock next = block->next;
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
context->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
free(block);
block = next;
@@ -1547,7 +1555,7 @@ AllocSetCheck(MemoryContext context)
long nchunks = 0;
bool has_external_chunk = false;
- if (set->keeper == block)
+ if (IsKeeperBlock(set, block))
total_allocated += block->endptr - ((char *) set);
else
total_allocated += block->endptr - ((char *) block);
@@ -1557,7 +1565,7 @@ AllocSetCheck(MemoryContext context)
*/
if (!blk_used)
{
- if (set->keeper != block)
+ if (!IsKeeperBlock(set, block))
elog(WARNING, "problem in alloc set %s: empty block %p",
name, block);
}
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 4fb8663cd6..d6a0811a72 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -61,17 +61,16 @@ typedef struct GenerationContext
MemoryContextData header; /* Standard memory-context fields */
/* Generational context parameters */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
GenerationBlock *block; /* current (most recently allocated) block, or
* NULL if we've just freed the most recent
* block */
GenerationBlock *freeblock; /* pointer to a block that's being recycled,
* or NULL if there's no such block. */
- GenerationBlock *keeper; /* keep this block over resets */
dlist_head blocks; /* list of blocks */
} GenerationContext;
@@ -120,6 +119,14 @@ struct GenerationBlock
#define ExternalChunkGetBlock(chunk) \
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
+/* Obtain the keeper block for a generation context */
+#define GenerationKeeper(set) \
+ ((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
+
+/* Check if the block is the keeper block of the given generation context */
+#define IsKeeperBlock(set, block) \
+ ((block) == (GenerationKeeper(set)))
+
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
GenerationBlock *block,
@@ -153,8 +160,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- Size firstBlockSize;
- Size allocSize;
+ uint32 firstBlockSize;
+ uint32 allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -213,8 +220,11 @@ GenerationContextCreate(MemoryContext parent,
*/
dlist_init(&set->blocks);
- /* Fill in the initial block's block header */
- block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = GenerationKeeper(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -228,13 +238,10 @@ GenerationContextCreate(MemoryContext parent,
/* No free block, yet */
set->freeblock = NULL;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
-
/* Fill in GenerationContext-specific header fields */
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -294,14 +301,14 @@ GenerationReset(MemoryContext context)
{
GenerationBlock *block = dlist_container(GenerationBlock, node, miter.cur);
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
GenerationBlockMarkEmpty(block);
else
GenerationBlockFree(set, block);
}
/* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
+ set->block = GenerationKeeper(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -440,10 +447,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(set->keeper) &&
- GenerationBlockFreeBytes(set->keeper) >= required_size)
+ else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
+ GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
{
- block = set->keeper;
+ block = GenerationKeeper(set);
}
else
{
@@ -594,7 +601,7 @@ static inline void
GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
{
/* Make sure nobody tries to free the keeper block */
- Assert(block != set->keeper);
+ Assert(!IsKeeperBlock(set, block));
/* We shouldn't be freeing the freeblock either */
Assert(block != set->freeblock);
@@ -691,7 +698,7 @@ GenerationFree(void *pointer)
set = block->context;
/* Don't try to free the keeper block, just mark it empty */
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
GenerationBlockMarkEmpty(block);
return;
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 718dd2ba03..d5e0edbede 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -104,7 +104,7 @@ typedef struct SlabContext
{
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
- Size chunkSize; /* the requested (non-aligned) chunk size */
+ uint32 chunkSize; /* the requested (non-aligned) chunk size */
Size fullChunkSize; /* chunk size with chunk header and alignment */
Size blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
@@ -374,7 +374,7 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = chunkSize;
+ slab->chunkSize = (Size) chunkSize;
slab->fullChunkSize = fullChunkSize;
slab->blockSize = blockSize;
slab->chunksPerBlock = chunksPerBlock;
@@ -506,7 +506,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (unlikely(size != slab->chunkSize))
- elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
size, slab->chunkSize);
/*
--
2.39.2
From 81825c3d3f7d12b5f5d68f3e132e5284f823e393 Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Thu, 13 Jul 2023 16:41:29 +1200
Subject: [PATCH v3 2/2] fixup! Change memory context fields to uint32
---
src/backend/utils/mmgr/aset.c | 25 ++++++++++++-------------
src/backend/utils/mmgr/generation.c | 29 ++++++++++++++---------------
src/backend/utils/mmgr/slab.c | 18 +++++++++++-------
3 files changed, 37 insertions(+), 35 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 797abef1b5..9b556396ee 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -241,12 +241,11 @@ typedef struct AllocBlockData
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
/* Obtain the keeper block for an allocation set */
-#define AllocSetKeeper(set) \
+#define KeeperBlock(set) \
((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
/* Check if the block is the keeper block of the given allocation set */
-#define IsKeeperBlock(set, block) \
- ((block) == (AllocSetKeeper(set)))
+#define IsKeeperBlock(set, block) ((block) == (KeeperBlock(set)))
typedef struct AllocSetFreeList
{
@@ -424,7 +423,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- AllocSetKeeper(set)->endptr - ((char *) set);
+ KeeperBlock(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -460,10 +459,10 @@ AllocSetContextCreateInternal(MemoryContext parent,
*/
/*
- * Fill in the initial block's block header. The initial block is not to be
- * released at reset time, considered as keeper block.
+ * Fill in the initial block's block header. The initial block is not to
+ * be released at reset time, considered as keeper block.
*/
- block = AllocSetKeeper(set);
+ block = KeeperBlock(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -479,9 +478,9 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = (Size) initBlockSize;
- set->maxBlockSize = (Size) maxBlockSize;
- set->nextBlockSize = (Size) initBlockSize;
+ set->initBlockSize = (uint32) initBlockSize;
+ set->maxBlockSize = (uint32) maxBlockSize;
+ set->nextBlockSize = (uint32) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -552,7 +551,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
+ keepersize = KeeperBlock(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -560,7 +559,7 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = AllocSetKeeper(set);
+ set->blocks = KeeperBlock(set);
while (block != NULL)
{
@@ -622,7 +621,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
+ keepersize = KeeperBlock(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index d6a0811a72..a152fbf0c5 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -120,12 +120,11 @@ struct GenerationBlock
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
/* Obtain the keeper block for a generation context */
-#define GenerationKeeper(set) \
+#define KeeperBlock(set) \
((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
/* Check if the block is the keeper block of the given generation context */
-#define IsKeeperBlock(set, block) \
- ((block) == (GenerationKeeper(set)))
+#define IsKeeperBlock(set, block) ((block) == (KeeperBlock(set)))
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
@@ -160,8 +159,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- uint32 firstBlockSize;
- uint32 allocSize;
+ Size firstBlockSize;
+ Size allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -221,10 +220,10 @@ GenerationContextCreate(MemoryContext parent,
dlist_init(&set->blocks);
/*
- * Fill in the initial block's block header. The initial block is not to be
- * released at reset time, considered as keeper block.
+ * Fill in the initial block's block header. The initial block is not to
+ * be released at reset time, considered as keeper block.
*/
- block = GenerationKeeper(set);
+ block = KeeperBlock(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -239,9 +238,9 @@ GenerationContextCreate(MemoryContext parent,
set->freeblock = NULL;
/* Fill in GenerationContext-specific header fields */
- set->initBlockSize = (Size) initBlockSize;
- set->maxBlockSize = (Size) maxBlockSize;
- set->nextBlockSize = (Size) initBlockSize;
+ set->initBlockSize = (uint32) initBlockSize;
+ set->maxBlockSize = (uint32) maxBlockSize;
+ set->nextBlockSize = (uint32) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -308,7 +307,7 @@ GenerationReset(MemoryContext context)
}
/* set it so new allocations to make use of the keeper block */
- set->block = GenerationKeeper(set);
+ set->block = KeeperBlock(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -447,10 +446,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
- GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
+ else if (GenerationBlockIsEmpty(KeeperBlock(set)) &&
+ GenerationBlockFreeBytes(KeeperBlock(set)) >= required_size)
{
- block = GenerationKeeper(set);
+ block = KeeperBlock(set);
}
else
{
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index d5e0edbede..40c1d401c4 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -105,8 +105,8 @@ typedef struct SlabContext
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
uint32 chunkSize; /* the requested (non-aligned) chunk size */
- Size fullChunkSize; /* chunk size with chunk header and alignment */
- Size blockSize; /* the size to make each block of chunks */
+ uint32 fullChunkSize; /* chunk size with chunk header and alignment */
+ uint32 blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
int32 curBlocklistIndex; /* index into the blocklist[] element
* containing the fullest, blocks */
@@ -314,7 +314,9 @@ SlabGetNextFreeChunk(SlabContext *slab, SlabBlock *block)
* blockSize: allocation block size
* chunkSize: allocation chunk size
*
- * The MAXALIGN(chunkSize) may not exceed MEMORYCHUNK_MAX_VALUE
+ * The Slab_CHUNKHDRSZ + MAXALIGN(chunkSize + 1) may not exceed
+ * MEMORYCHUNK_MAX_VALUE.
+ * 'blockSize' may not exceed MEMORYCHUNK_MAX_BLOCKOFFSET.
*/
MemoryContext
SlabContextCreate(MemoryContext parent,
@@ -330,7 +332,7 @@ SlabContextCreate(MemoryContext parent,
/* ensure MemoryChunk's size is properly maxaligned */
StaticAssertDecl(Slab_CHUNKHDRSZ == MAXALIGN(Slab_CHUNKHDRSZ),
"sizeof(MemoryChunk) is not maxaligned");
- Assert(MAXALIGN(chunkSize) <= MEMORYCHUNK_MAX_VALUE);
+ Assert(blockSize <= MEMORYCHUNK_MAX_BLOCKOFFSET);
/*
* Ensure there's enough space to store the pointer to the next free chunk
@@ -347,6 +349,8 @@ SlabContextCreate(MemoryContext parent,
fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize);
#endif
+ Assert(fullChunkSize <= MEMORYCHUNK_MAX_VALUE);
+
/* compute the number of chunks that will fit on each block */
chunksPerBlock = (blockSize - Slab_BLOCKHDRSZ) / fullChunkSize;
@@ -374,9 +378,9 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = (Size) chunkSize;
- slab->fullChunkSize = fullChunkSize;
- slab->blockSize = blockSize;
+ slab->chunkSize = (uint32) chunkSize;
+ slab->fullChunkSize = (uint32) fullChunkSize;
+ slab->blockSize = (uint32) blockSize;
slab->chunksPerBlock = chunksPerBlock;
slab->curBlocklistIndex = 0;
--
2.39.2
Attachments:
[text/plain] v3-0001-Change-memory-context-fields-to-uint32.patch (12.2K, ../../CAApHDvr+QF9CeSCVpZN0GyyvoGwG2-hSHaZ=VYa98vzzmuM_cw@mail.gmail.com/2-v3-0001-Change-memory-context-fields-to-uint32.patch)
download | inline diff:
From e5e7da6a9880b0cbc7fa96d26cdd73d8bb02c88f Mon Sep 17 00:00:00 2001
From: Melih Mutlu <[email protected]>
Date: Mon, 12 Jun 2023 08:50:28 +0300
Subject: [PATCH v3 1/2] Change memory context fields to uint32
Block sizes and chunk sizes can be upper bounded by
MEMORYCHUNK_MAX_BLOCKOFFSET and MEMORYCHUNK_MAX_VALUE respectively.
Values of both bounds correspond to 1 less than 1GB. This allows us
to store the sizes of any block or chunk size limited by those bounds
in 32 bits.
This patch changes types of such fields that represents block or chunk sizes
from 64-bit Size to 32-bit unsigned integers.
Also; keeper fields from AllocSet and Generation structs are removed and
added AllocSetKeeper/GenerationKeeper macros where needed.
---
src/backend/utils/mmgr/aset.c | 52 +++++++++++++++++------------
src/backend/utils/mmgr/generation.c | 51 ++++++++++++++++------------
src/backend/utils/mmgr/slab.c | 6 ++--
3 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 0bbbf93672..797abef1b5 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -156,11 +156,10 @@ typedef struct AllocSetContext
AllocBlock blocks; /* head of list of blocks in this set */
MemoryChunk *freelist[ALLOCSET_NUM_FREELISTS]; /* free chunk lists */
/* Allocation parameters for this context: */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
- AllocBlock keeper; /* keep this block over resets */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
/* freelist this context could be put in, or -1 if not a candidate: */
int freeListIndex; /* index in context_freelists[], or -1 */
} AllocSetContext;
@@ -241,6 +240,14 @@ typedef struct AllocBlockData
*/
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
+/* Obtain the keeper block for an allocation set */
+#define AllocSetKeeper(set) \
+ ((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
+
+/* Check if the block is the keeper block of the given allocation set */
+#define IsKeeperBlock(set, block) \
+ ((block) == (AllocSetKeeper(set)))
+
typedef struct AllocSetFreeList
{
int num_free; /* current list length */
@@ -345,7 +352,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
Size maxBlockSize)
{
int freeListIndex;
- Size firstBlockSize;
+ uint32 firstBlockSize;
AllocSet set;
AllocBlock block;
@@ -417,7 +424,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- set->keeper->endptr - ((char *) set);
+ AllocSetKeeper(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -452,8 +459,11 @@ AllocSetContextCreateInternal(MemoryContext parent,
* we'd leak the header/initial block if we ereport in this stretch.
*/
- /* Fill in the initial block's block header */
- block = (AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = AllocSetKeeper(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -465,15 +475,13 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Remember block as part of block list */
set->blocks = block;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -544,7 +552,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -552,13 +560,13 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = set->keeper;
+ set->blocks = AllocSetKeeper(set);
while (block != NULL)
{
AllocBlock next = block->next;
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
/* Reset the block, but don't return it to malloc */
char *datastart = ((char *) block) + ALLOC_BLOCKHDRSZ;
@@ -614,7 +622,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = set->keeper->endptr - ((char *) set);
+ keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
@@ -663,14 +671,14 @@ AllocSetDelete(MemoryContext context)
{
AllocBlock next = block->next;
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
context->mem_allocated -= block->endptr - ((char *) block);
#ifdef CLOBBER_FREED_MEMORY
wipe_mem(block, block->freeptr - ((char *) block));
#endif
- if (block != set->keeper)
+ if (!IsKeeperBlock(set, block))
free(block);
block = next;
@@ -1547,7 +1555,7 @@ AllocSetCheck(MemoryContext context)
long nchunks = 0;
bool has_external_chunk = false;
- if (set->keeper == block)
+ if (IsKeeperBlock(set, block))
total_allocated += block->endptr - ((char *) set);
else
total_allocated += block->endptr - ((char *) block);
@@ -1557,7 +1565,7 @@ AllocSetCheck(MemoryContext context)
*/
if (!blk_used)
{
- if (set->keeper != block)
+ if (!IsKeeperBlock(set, block))
elog(WARNING, "problem in alloc set %s: empty block %p",
name, block);
}
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index 4fb8663cd6..d6a0811a72 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -61,17 +61,16 @@ typedef struct GenerationContext
MemoryContextData header; /* Standard memory-context fields */
/* Generational context parameters */
- Size initBlockSize; /* initial block size */
- Size maxBlockSize; /* maximum block size */
- Size nextBlockSize; /* next block size to allocate */
- Size allocChunkLimit; /* effective chunk size limit */
+ uint32 initBlockSize; /* initial block size */
+ uint32 maxBlockSize; /* maximum block size */
+ uint32 nextBlockSize; /* next block size to allocate */
+ uint32 allocChunkLimit; /* effective chunk size limit */
GenerationBlock *block; /* current (most recently allocated) block, or
* NULL if we've just freed the most recent
* block */
GenerationBlock *freeblock; /* pointer to a block that's being recycled,
* or NULL if there's no such block. */
- GenerationBlock *keeper; /* keep this block over resets */
dlist_head blocks; /* list of blocks */
} GenerationContext;
@@ -120,6 +119,14 @@ struct GenerationBlock
#define ExternalChunkGetBlock(chunk) \
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
+/* Obtain the keeper block for a generation context */
+#define GenerationKeeper(set) \
+ ((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
+
+/* Check if the block is the keeper block of the given generation context */
+#define IsKeeperBlock(set, block) \
+ ((block) == (GenerationKeeper(set)))
+
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
GenerationBlock *block,
@@ -153,8 +160,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- Size firstBlockSize;
- Size allocSize;
+ uint32 firstBlockSize;
+ uint32 allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -213,8 +220,11 @@ GenerationContextCreate(MemoryContext parent,
*/
dlist_init(&set->blocks);
- /* Fill in the initial block's block header */
- block = (GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext)));
+ /*
+ * Fill in the initial block's block header. The initial block is not to be
+ * released at reset time, considered as keeper block.
+ */
+ block = GenerationKeeper(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -228,13 +238,10 @@ GenerationContextCreate(MemoryContext parent,
/* No free block, yet */
set->freeblock = NULL;
- /* Mark block as not to be released at reset time */
- set->keeper = block;
-
/* Fill in GenerationContext-specific header fields */
- set->initBlockSize = initBlockSize;
- set->maxBlockSize = maxBlockSize;
- set->nextBlockSize = initBlockSize;
+ set->initBlockSize = (Size) initBlockSize;
+ set->maxBlockSize = (Size) maxBlockSize;
+ set->nextBlockSize = (Size) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -294,14 +301,14 @@ GenerationReset(MemoryContext context)
{
GenerationBlock *block = dlist_container(GenerationBlock, node, miter.cur);
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
GenerationBlockMarkEmpty(block);
else
GenerationBlockFree(set, block);
}
/* set it so new allocations to make use of the keeper block */
- set->block = set->keeper;
+ set->block = GenerationKeeper(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -440,10 +447,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(set->keeper) &&
- GenerationBlockFreeBytes(set->keeper) >= required_size)
+ else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
+ GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
{
- block = set->keeper;
+ block = GenerationKeeper(set);
}
else
{
@@ -594,7 +601,7 @@ static inline void
GenerationBlockFree(GenerationContext *set, GenerationBlock *block)
{
/* Make sure nobody tries to free the keeper block */
- Assert(block != set->keeper);
+ Assert(!IsKeeperBlock(set, block));
/* We shouldn't be freeing the freeblock either */
Assert(block != set->freeblock);
@@ -691,7 +698,7 @@ GenerationFree(void *pointer)
set = block->context;
/* Don't try to free the keeper block, just mark it empty */
- if (block == set->keeper)
+ if (IsKeeperBlock(set, block))
{
GenerationBlockMarkEmpty(block);
return;
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index 718dd2ba03..d5e0edbede 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -104,7 +104,7 @@ typedef struct SlabContext
{
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
- Size chunkSize; /* the requested (non-aligned) chunk size */
+ uint32 chunkSize; /* the requested (non-aligned) chunk size */
Size fullChunkSize; /* chunk size with chunk header and alignment */
Size blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
@@ -374,7 +374,7 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = chunkSize;
+ slab->chunkSize = (Size) chunkSize;
slab->fullChunkSize = fullChunkSize;
slab->blockSize = blockSize;
slab->chunksPerBlock = chunksPerBlock;
@@ -506,7 +506,7 @@ SlabAlloc(MemoryContext context, Size size)
/* make sure we only allow correct request size */
if (unlikely(size != slab->chunkSize))
- elog(ERROR, "unexpected alloc chunk size %zu (expected %zu)",
+ elog(ERROR, "unexpected alloc chunk size %zu (expected %u)",
size, slab->chunkSize);
/*
--
2.39.2
[text/plain] v3-0002-fixup-Change-memory-context-fields-to-uint32.patch (8.7K, ../../CAApHDvr+QF9CeSCVpZN0GyyvoGwG2-hSHaZ=VYa98vzzmuM_cw@mail.gmail.com/3-v3-0002-fixup-Change-memory-context-fields-to-uint32.patch)
download | inline diff:
From 81825c3d3f7d12b5f5d68f3e132e5284f823e393 Mon Sep 17 00:00:00 2001
From: David Rowley <[email protected]>
Date: Thu, 13 Jul 2023 16:41:29 +1200
Subject: [PATCH v3 2/2] fixup! Change memory context fields to uint32
---
src/backend/utils/mmgr/aset.c | 25 ++++++++++++-------------
src/backend/utils/mmgr/generation.c | 29 ++++++++++++++---------------
src/backend/utils/mmgr/slab.c | 18 +++++++++++-------
3 files changed, 37 insertions(+), 35 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 797abef1b5..9b556396ee 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -241,12 +241,11 @@ typedef struct AllocBlockData
#define MAX_FREE_CONTEXTS 100 /* arbitrary limit on freelist length */
/* Obtain the keeper block for an allocation set */
-#define AllocSetKeeper(set) \
+#define KeeperBlock(set) \
((AllocBlock) (((char *) set) + MAXALIGN(sizeof(AllocSetContext))))
/* Check if the block is the keeper block of the given allocation set */
-#define IsKeeperBlock(set, block) \
- ((block) == (AllocSetKeeper(set)))
+#define IsKeeperBlock(set, block) ((block) == (KeeperBlock(set)))
typedef struct AllocSetFreeList
{
@@ -424,7 +423,7 @@ AllocSetContextCreateInternal(MemoryContext parent,
name);
((MemoryContext) set)->mem_allocated =
- AllocSetKeeper(set)->endptr - ((char *) set);
+ KeeperBlock(set)->endptr - ((char *) set);
return (MemoryContext) set;
}
@@ -460,10 +459,10 @@ AllocSetContextCreateInternal(MemoryContext parent,
*/
/*
- * Fill in the initial block's block header. The initial block is not to be
- * released at reset time, considered as keeper block.
+ * Fill in the initial block's block header. The initial block is not to
+ * be released at reset time, considered as keeper block.
*/
- block = AllocSetKeeper(set);
+ block = KeeperBlock(set);
block->aset = set;
block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
block->endptr = ((char *) set) + firstBlockSize;
@@ -479,9 +478,9 @@ AllocSetContextCreateInternal(MemoryContext parent,
/* Finish filling in aset-specific parts of the context header */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
- set->initBlockSize = (Size) initBlockSize;
- set->maxBlockSize = (Size) maxBlockSize;
- set->nextBlockSize = (Size) initBlockSize;
+ set->initBlockSize = (uint32) initBlockSize;
+ set->maxBlockSize = (uint32) maxBlockSize;
+ set->nextBlockSize = (uint32) initBlockSize;
set->freeListIndex = freeListIndex;
/*
@@ -552,7 +551,7 @@ AllocSetReset(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
+ keepersize = KeeperBlock(set)->endptr - ((char *) set);
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -560,7 +559,7 @@ AllocSetReset(MemoryContext context)
block = set->blocks;
/* New blocks list will be just the keeper block */
- set->blocks = AllocSetKeeper(set);
+ set->blocks = KeeperBlock(set);
while (block != NULL)
{
@@ -622,7 +621,7 @@ AllocSetDelete(MemoryContext context)
#endif
/* Remember keeper block size for Assert below */
- keepersize = AllocSetKeeper(set)->endptr - ((char *) set);
+ keepersize = KeeperBlock(set)->endptr - ((char *) set);
/*
* If the context is a candidate for a freelist, put it into that freelist
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index d6a0811a72..a152fbf0c5 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -120,12 +120,11 @@ struct GenerationBlock
(GenerationBlock *) ((char *) chunk - Generation_BLOCKHDRSZ)
/* Obtain the keeper block for a generation context */
-#define GenerationKeeper(set) \
+#define KeeperBlock(set) \
((GenerationBlock *) (((char *) set) + MAXALIGN(sizeof(GenerationContext))))
/* Check if the block is the keeper block of the given generation context */
-#define IsKeeperBlock(set, block) \
- ((block) == (GenerationKeeper(set)))
+#define IsKeeperBlock(set, block) ((block) == (KeeperBlock(set)))
/* Inlined helper functions */
static inline void GenerationBlockInit(GenerationContext *context,
@@ -160,8 +159,8 @@ GenerationContextCreate(MemoryContext parent,
Size initBlockSize,
Size maxBlockSize)
{
- uint32 firstBlockSize;
- uint32 allocSize;
+ Size firstBlockSize;
+ Size allocSize;
GenerationContext *set;
GenerationBlock *block;
@@ -221,10 +220,10 @@ GenerationContextCreate(MemoryContext parent,
dlist_init(&set->blocks);
/*
- * Fill in the initial block's block header. The initial block is not to be
- * released at reset time, considered as keeper block.
+ * Fill in the initial block's block header. The initial block is not to
+ * be released at reset time, considered as keeper block.
*/
- block = GenerationKeeper(set);
+ block = KeeperBlock(set);
/* determine the block size and initialize it */
firstBlockSize = allocSize - MAXALIGN(sizeof(GenerationContext));
GenerationBlockInit(set, block, firstBlockSize);
@@ -239,9 +238,9 @@ GenerationContextCreate(MemoryContext parent,
set->freeblock = NULL;
/* Fill in GenerationContext-specific header fields */
- set->initBlockSize = (Size) initBlockSize;
- set->maxBlockSize = (Size) maxBlockSize;
- set->nextBlockSize = (Size) initBlockSize;
+ set->initBlockSize = (uint32) initBlockSize;
+ set->maxBlockSize = (uint32) maxBlockSize;
+ set->nextBlockSize = (uint32) initBlockSize;
/*
* Compute the allocation chunk size limit for this context.
@@ -308,7 +307,7 @@ GenerationReset(MemoryContext context)
}
/* set it so new allocations to make use of the keeper block */
- set->block = GenerationKeeper(set);
+ set->block = KeeperBlock(set);
/* Reset block size allocation sequence, too */
set->nextBlockSize = set->initBlockSize;
@@ -447,10 +446,10 @@ GenerationAlloc(MemoryContext context, Size size)
*/
set->freeblock = NULL;
}
- else if (GenerationBlockIsEmpty(GenerationKeeper(set)) &&
- GenerationBlockFreeBytes(GenerationKeeper(set)) >= required_size)
+ else if (GenerationBlockIsEmpty(KeeperBlock(set)) &&
+ GenerationBlockFreeBytes(KeeperBlock(set)) >= required_size)
{
- block = GenerationKeeper(set);
+ block = KeeperBlock(set);
}
else
{
diff --git a/src/backend/utils/mmgr/slab.c b/src/backend/utils/mmgr/slab.c
index d5e0edbede..40c1d401c4 100644
--- a/src/backend/utils/mmgr/slab.c
+++ b/src/backend/utils/mmgr/slab.c
@@ -105,8 +105,8 @@ typedef struct SlabContext
MemoryContextData header; /* Standard memory-context fields */
/* Allocation parameters for this context: */
uint32 chunkSize; /* the requested (non-aligned) chunk size */
- Size fullChunkSize; /* chunk size with chunk header and alignment */
- Size blockSize; /* the size to make each block of chunks */
+ uint32 fullChunkSize; /* chunk size with chunk header and alignment */
+ uint32 blockSize; /* the size to make each block of chunks */
int32 chunksPerBlock; /* number of chunks that fit in 1 block */
int32 curBlocklistIndex; /* index into the blocklist[] element
* containing the fullest, blocks */
@@ -314,7 +314,9 @@ SlabGetNextFreeChunk(SlabContext *slab, SlabBlock *block)
* blockSize: allocation block size
* chunkSize: allocation chunk size
*
- * The MAXALIGN(chunkSize) may not exceed MEMORYCHUNK_MAX_VALUE
+ * The Slab_CHUNKHDRSZ + MAXALIGN(chunkSize + 1) may not exceed
+ * MEMORYCHUNK_MAX_VALUE.
+ * 'blockSize' may not exceed MEMORYCHUNK_MAX_BLOCKOFFSET.
*/
MemoryContext
SlabContextCreate(MemoryContext parent,
@@ -330,7 +332,7 @@ SlabContextCreate(MemoryContext parent,
/* ensure MemoryChunk's size is properly maxaligned */
StaticAssertDecl(Slab_CHUNKHDRSZ == MAXALIGN(Slab_CHUNKHDRSZ),
"sizeof(MemoryChunk) is not maxaligned");
- Assert(MAXALIGN(chunkSize) <= MEMORYCHUNK_MAX_VALUE);
+ Assert(blockSize <= MEMORYCHUNK_MAX_BLOCKOFFSET);
/*
* Ensure there's enough space to store the pointer to the next free chunk
@@ -347,6 +349,8 @@ SlabContextCreate(MemoryContext parent,
fullChunkSize = Slab_CHUNKHDRSZ + MAXALIGN(chunkSize);
#endif
+ Assert(fullChunkSize <= MEMORYCHUNK_MAX_VALUE);
+
/* compute the number of chunks that will fit on each block */
chunksPerBlock = (blockSize - Slab_BLOCKHDRSZ) / fullChunkSize;
@@ -374,9 +378,9 @@ SlabContextCreate(MemoryContext parent,
*/
/* Fill in SlabContext-specific header fields */
- slab->chunkSize = (Size) chunkSize;
- slab->fullChunkSize = fullChunkSize;
- slab->blockSize = blockSize;
+ slab->chunkSize = (uint32) chunkSize;
+ slab->fullChunkSize = (uint32) fullChunkSize;
+ slab->blockSize = (uint32) blockSize;
slab->chunksPerBlock = chunksPerBlock;
slab->curBlocklistIndex = 0;
--
2.39.2
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-07-10 14:41 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
2023-07-13 05:03 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
@ 2023-07-14 06:53 ` Melih Mutlu <[email protected]>
2023-07-16 23:18 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Melih Mutlu @ 2023-07-14 06:53 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>
Hi David,
David Rowley <[email protected]>, 13 Tem 2023 Per, 08:04 tarihinde şunu
yazdı:
> I looked at your v2 patch. The only thing that really looked wrong
> were the (Size) casts in the context creation functions. These should
> have been casts to uint32 rather than Size. Basically, all the casts
> do is say to the compiler "Yes, I know this could cause truncation due
> to assigning to a size smaller than the source type's size". Some
> compilers will likely warn without that and the cast will stop them.
> We know there can't be any truncation due to the Asserts. There's also
> the fundamental limitation that MemoryChunk can't store block offsets
> larger than 1GBs anyway, so things will go bad if we tried to have
> blocks bigger than 1GB.
>
Right! I don't know why I cast them to Size. Thanks for the fix.
Best,
--
Melih Mutlu
Microsoft
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Changing types of block and chunk sizes in memory contexts
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-07-10 14:41 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
2023-07-13 05:03 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-07-14 06:53 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
@ 2023-07-16 23:18 ` David Rowley <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: David Rowley @ 2023-07-16 23:18 UTC (permalink / raw)
To: Melih Mutlu <[email protected]>; +Cc: Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers; Andres Freund <[email protected]>
On Fri, 14 Jul 2023 at 18:53, Melih Mutlu <[email protected]> wrote:
> David Rowley <[email protected]>, 13 Tem 2023 Per, 08:04 tarihinde şunu yazdı:
>>
>> I looked at your v2 patch. The only thing that really looked wrong
>> were the (Size) casts in the context creation functions. These should
>> have been casts to uint32 rather than Size. Basically, all the casts
>> do is say to the compiler "Yes, I know this could cause truncation due
>> to assigning to a size smaller than the source type's size". Some
>> compilers will likely warn without that and the cast will stop them.
>> We know there can't be any truncation due to the Asserts. There's also
>> the fundamental limitation that MemoryChunk can't store block offsets
>> larger than 1GBs anyway, so things will go bad if we tried to have
>> blocks bigger than 1GB.
>
>
> Right! I don't know why I cast them to Size. Thanks for the fix.
Pushed.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: extension_control_path and "directory"
@ 2025-04-28 20:49 David E. Wheeler <[email protected]>
2025-04-29 14:08 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: David E. Wheeler @ 2025-04-28 20:49 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]
On Apr 25, 2025, at 17:18, Matheus Alcantara <[email protected]> wrote:
> Ok, I was testing using extension_control_path = '$system:/my/custom/path'
> (starting with the macro) and it was working as expected, testing with
> the macro at the end does not work.
Great example of why it’s useful to do as much testing as possible! That’s an entirely reasonable place to start testing :-)
> The problem was on find_extension_control_filename() that was appending
> the /extension at the end of the entire extension_control_path GUC value
> instead of just the custom paths.
Oh yeah, lol, that wouldn’t work.
> To append the /extension at each path on extension_control_path would
> require some changes on find_in_path() that
> find_extension_control_filename() calls, which I think that it would
> make the function more complicated. I've them created a similar
> find_in_paths() function that works in the same way but it receives a
> List of paths instead of the string of paths separated by ":". We can
> get this List of paths using get_extension_control_directories() that
> also handle the macro substitution like find_in_path().
>
> Attached v4 with these fixes. I hope that now you should be able to omit
> the /extension from the GUC value.
Yes! It now works with this configuration:
```ini
extension_control_path = '/Users/david/Downloads/share/postgresql:$system'
dynamic_library_path = '/Users/david/Downloads/lib/postgresql:$libdir’
```
Which is nicely more consistent. Kind of want that first one to be called “share_path” now, though, since it’s not just extensions. Although I guess it’s only extension control file searching that uses it (for now).
If I understand this bit correctly:
```c
/* Substitute the path macro if needed */
mangled = substitute_path_macro(piece, "$system", system_dir);
/*
* Append "extension" suffix in case is a custom extension control
* path.
*/
if (strcmp(piece, "$system") != 0)
mangled = psprintf("%s/extension", mangled);
```
The value of `piece` is a single path from the search path, right? If so, I think it’s either `$system` or something else; I don’t it would ever be that `$system` is a substring of a single path. Is that right?
Other than that, I think this patch is good to go. Thanks!
Best,
David
`
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: extension_control_path and "directory"
2025-04-28 20:49 Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
@ 2025-04-29 14:08 ` David E. Wheeler <[email protected]>
2025-05-02 14:51 ` Re: extension_control_path and "directory" Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: David E. Wheeler @ 2025-04-29 14:08 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: Christoph Berg <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]
On Apr 29, 2025, at 09:49, Matheus Alcantara <[email protected]> wrote:
> Yes, it is a single path from the search path, in your case it will be
> "/Users/david/Downloads/share/postgresql" and "$system". We split these
> paths based on the system path separator and get the next "piece" here:
>
> char *piece = first_path_var_separator(ecp);
>
> The first_path_var_separator() changes the "ecp" parameter on every call,
> it returns the next path on "ecp" and changes it to have the remaining
> paths to iterate over it.
Right. My point is a minor one, but I thin you can use an if/ else there:
```c
if (strcmp(piece, "$system") == 0) {
/* Substitute the path macro if needed */
mangled = substitute_path_macro(piece, "$system", system_dir);
} else {
/*
* Append "extension" suffix in case is a custom extension
* control path.
*/
mangled = psprintf("%s/extension", mangled);
}
```
Best,
David
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: extension_control_path and "directory"
2025-04-28 20:49 Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-29 14:08 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
@ 2025-05-02 14:51 ` Peter Eisentraut <[email protected]>
2025-05-02 15:04 ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Peter Eisentraut @ 2025-05-02 14:51 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; David E. Wheeler <[email protected]>; +Cc: Christoph Berg <[email protected]>; [email protected]
On 29.04.25 17:06, Matheus Alcantara wrote:
> On Tue, Apr 29, 2025 at 11:08 AM David E. Wheeler <[email protected]> wrote:
>> Right. My point is a minor one, but I thin you can use an if/ else there:
>>
>> ```c
>> if (strcmp(piece, "$system") == 0) {
>> /* Substitute the path macro if needed */
>> mangled = substitute_path_macro(piece, "$system", system_dir);
>> } else {
>> /*
>> * Append "extension" suffix in case is a custom extension
>> * control path.
>> */
>> mangled = psprintf("%s/extension", mangled);
>> }
>> ```
>>
>
> The substitute_path_macro() already handles the if/else on "piece" but I
> think that this if/else version looks nicer. Fixed.
>
> I've also included some documentation changes for this v5 version to
> remove the "extension" from the examples and also mention the scenario
> when using the "directory" on the .control file.
Thanks, I have committed this. I did a bit of code reformatting and
adjusted the documentation a bit. It's good to get this in before beta1
so that we don't have to change the valid values of
extension_control_path past beta1.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: extension_control_path and "directory"
2025-04-28 20:49 Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-29 14:08 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-05-02 14:51 ` Re: extension_control_path and "directory" Peter Eisentraut <[email protected]>
@ 2025-05-02 15:04 ` Matheus Alcantara <[email protected]>
2025-05-02 15:53 ` Re: extension_control_path and "directory" Christoph Berg <[email protected]>
0 siblings, 1 reply; 20+ messages in thread
From: Matheus Alcantara @ 2025-05-02 15:04 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Christoph Berg <[email protected]>; [email protected]
On Fri, May 2, 2025 at 11:51 AM Peter Eisentraut <[email protected]> wrote:
>
> Thanks, I have committed this. I did a bit of code reformatting and
> adjusted the documentation a bit. It's good to get this in before beta1
> so that we don't have to change the valid values of
> extension_control_path past beta1.
>
Thanks Peter!
--
Matheus Alcantara
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: extension_control_path and "directory"
2025-04-28 20:49 Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-29 14:08 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-05-02 14:51 ` Re: extension_control_path and "directory" Peter Eisentraut <[email protected]>
2025-05-02 15:04 ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
@ 2025-05-02 15:53 ` Christoph Berg <[email protected]>
0 siblings, 0 replies; 20+ messages in thread
From: Christoph Berg @ 2025-05-02 15:53 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; David E. Wheeler <[email protected]>; [email protected]
Re: Matheus Alcantara
> > Thanks, I have committed this. I did a bit of code reformatting and
> > adjusted the documentation a bit. It's good to get this in before beta1
> > so that we don't have to change the valid values of
> > extension_control_path past beta1.
>
> Thanks Peter!
And thanks everyone!
Christoph
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2025-05-02 15:53 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-04-01 04:07 [PATCH 3/4] GiST-Optimal-WAL-Usage Andrey V. Lepikhov <[email protected]>
2019-04-02 04:43 [PATCH 3/4] GiST-Optimal-WAL-Usage Andrey V. Lepikhov <[email protected]>
2023-06-28 09:37 Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-06-28 10:59 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 21:26 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2023-06-28 21:56 ` Re: Changing types of block and chunk sizes in memory contexts Tom Lane <[email protected]>
2023-06-28 23:42 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
2023-06-28 23:34 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
2023-06-29 09:58 ` Re: Changing types of block and chunk sizes in memory contexts Tomas Vondra <[email protected]>
2023-06-30 00:29 ` Re: Changing types of block and chunk sizes in memory contexts Andres Freund <[email protected]>
2023-06-29 03:46 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-07-10 14:41 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
2023-07-13 05:03 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2023-07-14 06:53 ` Re: Changing types of block and chunk sizes in memory contexts Melih Mutlu <[email protected]>
2023-07-16 23:18 ` Re: Changing types of block and chunk sizes in memory contexts David Rowley <[email protected]>
2025-04-28 20:49 Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-04-29 14:08 ` Re: extension_control_path and "directory" David E. Wheeler <[email protected]>
2025-05-02 14:51 ` Re: extension_control_path and "directory" Peter Eisentraut <[email protected]>
2025-05-02 15:04 ` Re: extension_control_path and "directory" Matheus Alcantara <[email protected]>
2025-05-02 15:53 ` Re: extension_control_path and "directory" Christoph Berg <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox