agora inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 1/2] Move a few ResourceOwnerEnlarge() calls for safety and clarity. 12+ messages / 2 participants [nested] [flat]
* [PATCH v5 1/2] Move a few ResourceOwnerEnlarge() calls for safety and clarity. @ 2021-01-13 10:21 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Heikki Linnakangas @ 2021-01-13 10:21 UTC (permalink / raw) These are functions where quite a lot of things happen between the ResourceOwnerEnlarge and ResourceOwnerRemember calls. It's important that there are no unrelated ResourceOwnerRemember() calls in the code inbetween, otherwise the entry reserved by the ResourceOwnerEnlarge() call might be used up by the intervening ResourceOwnerRemember() and not be available at the intended ResourceOwnerRemember() call anymore. The longer the code path between them is, the harder it is to verify that. In bufmgr.c, there is a function similar to ResourceOwnerEnlarge(), to ensure that the private refcount array has enough space. The ReservePrivateRefCountEntry() calls, analogous to ResourceOwnerEnlarge(), were made at different places than the ResourceOwnerEnlarge() calls. Move the ResourceOwnerEnlarge() calls together with the ReservePrivateRefCountEntry() calls for consistency. --- src/backend/storage/buffer/bufmgr.c | 39 +++++++++++---------------- src/backend/storage/buffer/localbuf.c | 3 +++ src/backend/utils/cache/catcache.c | 13 ++++++--- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 561c212092f..cde869e7d64 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -738,9 +738,6 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, *hit = false; - /* Make sure we will have room to remember the buffer pin */ - ResourceOwnerEnlargeBuffers(CurrentResourceOwner); - isExtend = (blockNum == P_NEW); TRACE_POSTGRESQL_BUFFER_READ_START(forkNum, blockNum, @@ -1093,9 +1090,11 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum, { /* * Ensure, while the spinlock's not yet held, that there's a free - * refcount entry. + * refcount entry and that the resoure owner has room to remember the + * pin. */ ReservePrivateRefCountEntry(); + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); /* * Select a victim buffer. The buffer is returned with its header @@ -1595,8 +1594,6 @@ ReleaseAndReadBuffer(Buffer buffer, * taking the buffer header lock; instead update the state variable in loop of * CAS operations. Hopefully it's just a single CAS. * - * Note that ResourceOwnerEnlargeBuffers must have been done already. - * * Returns true if buffer is BM_VALID, else false. This provision allows * some callers to avoid an extra spinlock cycle. */ @@ -1607,6 +1604,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy) bool result; PrivateRefCountEntry *ref; + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); + ref = GetPrivateRefCountEntry(b, true); if (ref == NULL) @@ -1687,7 +1686,8 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy) * The spinlock is released before return. * * As this function is called with the spinlock held, the caller has to - * previously call ReservePrivateRefCountEntry(). + * previously call ReservePrivateRefCountEntry() and + * ResourceOwnerEnlargeBuffers(CurrentResourceOwner); * * Currently, no callers of this function want to modify the buffer's * usage_count at all, so there's no need for a strategy parameter. @@ -1857,9 +1857,6 @@ BufferSync(int flags) int mask = BM_DIRTY; WritebackContext wb_context; - /* Make sure we can handle the pin inside SyncOneBuffer */ - ResourceOwnerEnlargeBuffers(CurrentResourceOwner); - /* * Unless this is a shutdown checkpoint or we have been explicitly told, * we write only permanent, dirty buffers. But at shutdown or end of @@ -2334,9 +2331,6 @@ BgBufferSync(WritebackContext *wb_context) * requirements, or hit the bgwriter_lru_maxpages limit. */ - /* Make sure we can handle the pin inside SyncOneBuffer */ - ResourceOwnerEnlargeBuffers(CurrentResourceOwner); - num_to_scan = bufs_to_lap; num_written = 0; reusable_buffers = reusable_buffers_est; @@ -2418,8 +2412,6 @@ BgBufferSync(WritebackContext *wb_context) * * (BUF_WRITTEN could be set in error if FlushBuffer finds the buffer clean * after locking it, but we don't care all that much.) - * - * Note: caller must have done ResourceOwnerEnlargeBuffers. */ static int SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context) @@ -2429,7 +2421,9 @@ SyncOneBuffer(int buf_id, bool skip_recently_used, WritebackContext *wb_context) uint32 buf_state; BufferTag tag; + /* Make sure we can handle the pin */ ReservePrivateRefCountEntry(); + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); /* * Check whether buffer needs writing. @@ -3487,9 +3481,6 @@ FlushRelationBuffers(Relation rel) return; } - /* Make sure we can handle the pin inside the loop */ - ResourceOwnerEnlargeBuffers(CurrentResourceOwner); - for (i = 0; i < NBuffers; i++) { uint32 buf_state; @@ -3503,7 +3494,9 @@ FlushRelationBuffers(Relation rel) if (!RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node)) continue; + /* Make sure we can handle the pin */ ReservePrivateRefCountEntry(); + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); buf_state = LockBufHdr(bufHdr); if (RelFileNodeEquals(bufHdr->tag.rnode, rel->rd_node) && @@ -3560,9 +3553,6 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels) if (use_bsearch) pg_qsort(srels, nrels, sizeof(SMgrSortArray), rnode_comparator); - /* Make sure we can handle the pin inside the loop */ - ResourceOwnerEnlargeBuffers(CurrentResourceOwner); - for (i = 0; i < NBuffers; i++) { SMgrSortArray *srelent = NULL; @@ -3599,7 +3589,9 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels) if (srelent == NULL) continue; + /* Make sure we can handle the pin */ ReservePrivateRefCountEntry(); + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); buf_state = LockBufHdr(bufHdr); if (RelFileNodeEquals(bufHdr->tag.rnode, srelent->rnode) && @@ -3639,9 +3631,6 @@ FlushDatabaseBuffers(Oid dbid) int i; BufferDesc *bufHdr; - /* Make sure we can handle the pin inside the loop */ - ResourceOwnerEnlargeBuffers(CurrentResourceOwner); - for (i = 0; i < NBuffers; i++) { uint32 buf_state; @@ -3655,7 +3644,9 @@ FlushDatabaseBuffers(Oid dbid) if (bufHdr->tag.rnode.dbNode != dbid) continue; + /* Make sure we can handle the pin */ ReservePrivateRefCountEntry(); + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); buf_state = LockBufHdr(bufHdr); if (bufHdr->tag.rnode.dbNode == dbid && diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 04b3558ea33..f7c15ea8a44 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -123,6 +123,9 @@ LocalBufferAlloc(SMgrRelation smgr, ForkNumber forkNum, BlockNumber blockNum, if (LocalBufHash == NULL) InitLocalBuffers(); + /* Make sure we will have room to remember the buffer pin */ + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); + /* See if the desired buffer already exists */ hresult = (LocalBufferLookupEnt *) hash_search(LocalBufHash, (void *) &newTag, HASH_FIND, NULL); diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index fa2b49c676e..86fdfc454d2 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -1607,8 +1607,6 @@ SearchCatCacheList(CatCache *cache, * block to ensure we can undo those refcounts if we get an error before * we finish constructing the CatCList. */ - ResourceOwnerEnlargeCatCacheListRefs(CurrentResourceOwner); - ctlist = NIL; PG_TRY(); @@ -1696,13 +1694,22 @@ SearchCatCacheList(CatCache *cache, table_close(relation, AccessShareLock); + /* Make sure the resource owner has room to remember this entry. */ + ResourceOwnerEnlargeCatCacheListRefs(CurrentResourceOwner); + /* Now we can build the CatCList entry. */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); nmembers = list_length(ctlist); cl = (CatCList *) palloc(offsetof(CatCList, members) + nmembers * sizeof(CatCTup *)); - /* Extract key values */ + /* + * Extract key values. + * + * XXX: If we run out of memory while copying the key values, we will + * leak any allocations we had already made in the CacheMemoryContext. + * That is unlikely enough that we just accept the risk. + */ CatCacheCopyKeys(cache->cc_tupdesc, nkeys, cache->cc_keyno, arguments, cl->keys); MemoryContextSwitchTo(oldcxt); -- 2.29.2 --------------1104342304EC6341F5D7C60C Content-Type: text/x-patch; charset=UTF-8; name="v5-0002-Make-resowners-more-easily-extensible.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v5-0002-Make-resowners-more-easily-extensible.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v3 10/13] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 29 +++++++---------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index 0f4850065fb..689a959b467 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,21 +40,12 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "nodes/bitmapset.h" #include "nodes/tidbitmap.h" #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -66,7 +57,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -78,7 +69,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -180,7 +171,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -221,7 +212,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -389,7 +380,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -691,12 +682,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1470,8 +1456,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) * Create the TBMSharedIterator struct, with enough trailing space to * serve the needs of the TBMIterateResult sub-struct. */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639bf..432fae52962 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.37.2 --fa5b4qdlc6zixc7z Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0011-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v4 11/14] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 29 +++++++---------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index 0f4850065fb..689a959b467 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,21 +40,12 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "nodes/bitmapset.h" #include "nodes/tidbitmap.h" #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -66,7 +57,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -78,7 +69,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -180,7 +171,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -221,7 +212,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -389,7 +380,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -691,12 +682,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1470,8 +1456,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) * Create the TBMSharedIterator struct, with enough trailing space to * serve the needs of the TBMIterateResult sub-struct. */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639bf..432fae52962 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.37.2 --5aaqsqqhbq27q3qo Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v4-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v5 11/14] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 29 +++++++---------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fcc..d2bf8f44d50 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) * Create the TBMSharedIterator struct, with enough trailing space to * serve the needs of the TBMIterateResult sub-struct. */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639bf..432fae52962 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.37.2 --6kjpcnqyi64ibp5i Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v13 12/16] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 33 +++++++-------------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fc..1dc4c99bf9 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) TBMSharedIterator *iterator; TBMSharedIteratorState *istate; - /* - * Create the TBMSharedIterator struct, with enough trailing space to - * serve the needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639b..432fae5296 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --cuuqjeyokkhgd736 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v13-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 29 +++++++---------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fcc..d2bf8f44d50 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) * Create the TBMSharedIterator struct, with enough trailing space to * serve the needs of the TBMIterateResult sub-struct. */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639bf..432fae52962 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --w4wcjcocxsm37usi Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 29 +++++++---------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fcc..d2bf8f44d50 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) * Create the TBMSharedIterator struct, with enough trailing space to * serve the needs of the TBMIterateResult sub-struct. */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639bf..432fae52962 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --kqqpqghcwbcc3dt5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v7-0011-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 33 +++++++-------------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fc..1dc4c99bf9 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) TBMSharedIterator *iterator; TBMSharedIteratorState *istate; - /* - * Create the TBMSharedIterator struct, with enough trailing space to - * serve the needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639b..432fae5296 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --xqq4defy3uncu6k6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0011-table_scan_bitmap_next_block-counts-lossy-and-exa.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 33 +++++++-------------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fc..1dc4c99bf9 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) TBMSharedIterator *iterator; TBMSharedIteratorState *istate; - /* - * Create the TBMSharedIterator struct, with enough trailing space to - * serve the needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639b..432fae5296 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --7mdtsjmrzitrgzgx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v10 12/17] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 33 +++++++-------------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fc..1dc4c99bf9 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) TBMSharedIterator *iterator; TBMSharedIteratorState *istate; - /* - * Create the TBMSharedIterator struct, with enough trailing space to - * serve the needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639b..432fae5296 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --3o7pc6dfau5a5hry Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v10-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 33 +++++++-------------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fc..1dc4c99bf9 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) TBMSharedIterator *iterator; TBMSharedIteratorState *istate; - /* - * Create the TBMSharedIterator struct, with enough trailing space to - * serve the needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639b..432fae5296 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --owzzsiozz6hgpp7e Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v11-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
* [PATCH v12 12/17] Hard-code TBMIterateResult offsets array size @ 2024-02-16 01:13 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 12+ messages in thread From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw) TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets but the API always allocated MaxHeapTuplesPerPage OffsetNumbers. Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear for the API user. --- src/backend/nodes/tidbitmap.c | 33 +++++++-------------------------- src/include/nodes/tidbitmap.h | 12 ++++++++++-- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c index e8ab5d78fcc..1dc4c99bf99 100644 --- a/src/backend/nodes/tidbitmap.c +++ b/src/backend/nodes/tidbitmap.c @@ -40,7 +40,6 @@ #include <limits.h> -#include "access/htup_details.h" #include "common/hashfn.h" #include "common/int.h" #include "nodes/bitmapset.h" @@ -48,14 +47,6 @@ #include "storage/lwlock.h" #include "utils/dsa.h" -/* - * The maximum number of tuples per page is not large (typically 256 with - * 8K pages, or 1024 with 32K pages). So there's not much point in making - * the per-page bitmaps variable size. We just legislate that the size - * is this: - */ -#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage - /* * When we have to switch over to lossy storage, we use a data structure * with one bit per page, where all pages having the same number DIV @@ -67,7 +58,7 @@ * table, using identical data structures. (This is because the memory * management for hashtables doesn't easily/efficiently allow space to be * transferred easily from one hashtable to another.) Therefore it's best - * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not + * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not * too different. But we also want PAGES_PER_CHUNK to be a power of 2 to * avoid expensive integer remainder operations. So, define it like this: */ @@ -79,7 +70,7 @@ #define BITNUM(x) ((x) % BITS_PER_BITMAPWORD) /* number of active words for an exact page: */ -#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1) +#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1) /* number of active words for a lossy chunk: */ #define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1) @@ -181,7 +172,7 @@ struct TBMIterator int spageptr; /* next spages index */ int schunkptr; /* next schunks index */ int schunkbit; /* next bit to check in current schunk */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* @@ -222,7 +213,7 @@ struct TBMSharedIterator PTEntryArray *ptbase; /* pagetable element array */ PTIterationArray *ptpages; /* sorted exact page index list */ PTIterationArray *ptchunks; /* sorted lossy page index list */ - TBMIterateResult output; /* MUST BE LAST (because variable-size) */ + TBMIterateResult output; }; /* Local function prototypes */ @@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids, bitnum; /* safety check to ensure we don't overrun bit array bounds */ - if (off < 1 || off > MAX_TUPLES_PER_PAGE) + if (off < 1 || off > MaxHeapTuplesPerPage) elog(ERROR, "tuple offset out of range: %u", off); /* @@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm) Assert(tbm->iterating != TBM_ITERATING_SHARED); - /* - * Create the TBMIterator struct, with enough trailing space to serve the - * needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMIterator *) palloc(sizeof(TBMIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = palloc(sizeof(TBMIterator)); iterator->tbm = tbm; /* @@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp) TBMSharedIterator *iterator; TBMSharedIteratorState *istate; - /* - * Create the TBMSharedIterator struct, with enough trailing space to - * serve the needs of the TBMIterateResult sub-struct. - */ - iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) + - MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber)); + iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator)); istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp); diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h index 1945f0639bf..432fae52962 100644 --- a/src/include/nodes/tidbitmap.h +++ b/src/include/nodes/tidbitmap.h @@ -22,6 +22,7 @@ #ifndef TIDBITMAP_H #define TIDBITMAP_H +#include "access/htup_details.h" #include "storage/itemptr.h" #include "utils/dsa.h" @@ -41,9 +42,16 @@ typedef struct TBMIterateResult { BlockNumber blockno; /* page number containing tuples */ int ntuples; /* -1 indicates lossy result */ - bool recheck; /* should the tuples be rechecked? */ /* Note: recheck is always true if ntuples < 0 */ - OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER]; + bool recheck; /* should the tuples be rechecked? */ + + /* + * The maximum number of tuples per page is not large (typically 256 with + * 8K pages, or 1024 with 32K pages). So there's not much point in making + * the per-page bitmaps variable size. We just legislate that the size is + * this: + */ + OffsetNumber offsets[MaxHeapTuplesPerPage]; } TBMIterateResult; /* function prototypes in nodes/tidbitmap.c */ -- 2.40.1 --6jpz2j246qmht4bt Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v12-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch" ^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2024-02-16 01:13 UTC | newest] Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 10:21 [PATCH v5 1/2] Move a few ResourceOwnerEnlarge() calls for safety and clarity. Heikki Linnakangas <[email protected]> 2024-02-16 01:13 [PATCH v3 10/13] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v5 11/14] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v10 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v4 11/14] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v13 12/16] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]> 2024-02-16 01:13 [PATCH v12 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[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