Skip to content

Commit b89ee54

Browse files
alvherrealvherre
committed
Fix some coding issues in BRIN
Reported by David Rowley: variadic macros are a problem. Get rid of them using a trick suggested by Tom Lane: add extra parentheses where needed. In the future we might decide we don't need the calls at all and remove them, but it seems appropriate to keep them while this code is still new. Also from David Rowley: brininsert() was trying to use a variable before initializing it. Fix by moving the brin_form_tuple call (which initializes the variable) to within the locked section. Reported by Peter Eisentraut: can't use "new" as a struct member name, because C++ compilers will choke on it, as reported by cpluspluscheck.
1 parent 926f5ce commit b89ee54

File tree

7 files changed

+29
-35
lines changed

7 files changed

+29
-35
lines changed

‎src/backend/access/brin/brin.c

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -247,12 +247,10 @@ brininsert(PG_FUNCTION_ARGS)
247247
* the same page though, so downstream we must be prepared to cope
248248
* if this turns out to not be possible after all.
249249
*/
250+
newtup = brin_form_tuple(bdesc, heapBlk, dtup, &newsz);
250251
samepage = brin_can_do_samepage_update(buf, origsz, newsz);
251-
252252
LockBuffer(buf, BUFFER_LOCK_UNLOCK);
253253

254-
newtup = brin_form_tuple(bdesc, heapBlk, dtup, &newsz);
255-
256254
/*
257255
* Try to update the tuple. If this doesn't work for whatever
258256
* reason, we need to restart from the top; the revmap might be
@@ -589,9 +587,10 @@ brinbuildCallback(Relation index,
589587
while (thisblock > state->bs_currRangeStart + state->bs_pagesPerRange - 1)
590588
{
591589

592-
BRIN_elog(DEBUG2, "brinbuildCallback: completed a range: %u--%u",
593-
state->bs_currRangeStart,
594-
state->bs_currRangeStart + state->bs_pagesPerRange);
590+
BRIN_elog((DEBUG2,
591+
"brinbuildCallback: completed a range: %u--%u",
592+
state->bs_currRangeStart,
593+
state->bs_currRangeStart + state->bs_pagesPerRange));
595594

596595
/* create the index tuple and insert it */
597596
form_and_insert_tuple(state);

‎src/backend/access/brin/brin_pageops.c

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -216,12 +216,12 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
216216

217217
info = XLOG_BRIN_UPDATE | (extended ? XLOG_BRIN_INIT_PAGE : 0);
218218

219-
xlrec.new.node = idxrel->rd_node;
220-
ItemPointerSet(&xlrec.new.tid, BufferGetBlockNumber(newbuf), newoff);
221-
xlrec.new.heapBlk = heapBlk;
222-
xlrec.new.tuplen = newsz;
223-
xlrec.new.revmapBlk = BufferGetBlockNumber(revmapbuf);
224-
xlrec.new.pagesPerRange = pagesPerRange;
219+
xlrec.insert.node = idxrel->rd_node;
220+
ItemPointerSet(&xlrec.insert.tid, BufferGetBlockNumber(newbuf), newoff);
221+
xlrec.insert.heapBlk = heapBlk;
222+
xlrec.insert.tuplen = newsz;
223+
xlrec.insert.revmapBlk = BufferGetBlockNumber(revmapbuf);
224+
xlrec.insert.pagesPerRange = pagesPerRange;
225225
ItemPointerSet(&xlrec.oldtid, BufferGetBlockNumber(oldbuf), oldoff);
226226

227227
rdata[0].data = (char *) &xlrec;
@@ -342,8 +342,8 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
342342
elog(ERROR, "could not insert new index tuple to page");
343343
MarkBufferDirty(*buffer);
344344

345-
BRIN_elog(DEBUG2, "inserted tuple (%u,%u) for range starting at %u",
346-
blk, off, heapBlk);
345+
BRIN_elog((DEBUG2, "inserted tuple (%u,%u) for range starting at %u",
346+
blk, off, heapBlk));
347347

348348
ItemPointerSet(&tid, blk, off);
349349
brinSetHeapBlockItemptr(revmapbuf, pagesPerRange, heapBlk, tid);
@@ -593,8 +593,8 @@ brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
593593
newblk = BufferGetBlockNumber(buf);
594594
*was_extended = extended = true;
595595

596-
BRIN_elog(DEBUG2, "brin_getinsertbuffer: extending to page %u",
597-
BufferGetBlockNumber(buf));
596+
BRIN_elog((DEBUG2, "brin_getinsertbuffer: extending to page %u",
597+
BufferGetBlockNumber(buf)));
598598
}
599599
else if (newblk == oldblk)
600600
{

‎src/backend/access/brin/brin_revmap.c

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -331,10 +331,6 @@ revmap_get_buffer(BrinRevmap *revmap, BlockNumber heapBlk)
331331
Assert(mapBlk != BRIN_METAPAGE_BLKNO &&
332332
mapBlk <= revmap->rm_lastRevmapPage);
333333

334-
BRIN_elog(DEBUG2, "getting revmap page for logical page %lu (physical %u) for heap %u",
335-
HEAPBLK_TO_REVMAP_BLK(revmap->rm_pagesPerRange, heapBlk),
336-
mapBlk, heapBlk);
337-
338334
/*
339335
* Obtain the buffer from which we need to read. If we already have the
340336
* correct buffer in our access struct, use that; otherwise, release that,

‎src/backend/access/brin/brin_xlog.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ brin_xlog_update(XLogRecPtr lsn, XLogRecord *record)
144144

145145
/* First remove the old tuple */
146146
blkno = ItemPointerGetBlockNumber(&(xlrec->oldtid));
147-
action = XLogReadBufferForRedo(lsn, record, 2, xlrec->new.node,
147+
action = XLogReadBufferForRedo(lsn, record, 2, xlrec->insert.node,
148148
blkno, &buffer);
149149
if (action == BLK_NEEDS_REDO)
150150
{
@@ -164,7 +164,7 @@ brin_xlog_update(XLogRecPtr lsn, XLogRecord *record)
164164
}
165165

166166
/* Then insert the new tuple and update revmap, like in an insertion. */
167-
brin_xlog_insert_update(lsn, record, &xlrec->new, newtup);
167+
brin_xlog_insert_update(lsn, record, &xlrec->insert, newtup);
168168

169169
if (BufferIsValid(buffer))
170170
UnlockReleaseBuffer(buffer);

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,14 @@ brin_desc(StringInfo buf, XLogRecord *record)
4949
xl_brin_update *xlrec = (xl_brin_update *) rec;
5050

5151
appendStringInfo(buf, "rel %u/%u/%u heapBlk %u revmapBlk %u pagesPerRange %u old TID (%u,%u) TID (%u,%u)",
52-
xlrec->new.node.spcNode, xlrec->new.node.dbNode,
53-
xlrec->new.node.relNode,
54-
xlrec->new.heapBlk, xlrec->new.revmapBlk,
55-
xlrec->new.pagesPerRange,
52+
xlrec->insert.node.spcNode, xlrec->insert.node.dbNode,
53+
xlrec->insert.node.relNode,
54+
xlrec->insert.heapBlk, xlrec->insert.revmapBlk,
55+
xlrec->insert.pagesPerRange,
5656
ItemPointerGetBlockNumber(&xlrec->oldtid),
5757
ItemPointerGetOffsetNumber(&xlrec->oldtid),
58-
ItemPointerGetBlockNumber(&xlrec->new.tid),
59-
ItemPointerGetOffsetNumber(&xlrec->new.tid));
58+
ItemPointerGetBlockNumber(&xlrec->insert.tid),
59+
ItemPointerGetOffsetNumber(&xlrec->insert.tid));
6060
}
6161
else if (info == XLOG_BRIN_SAMEPAGE_UPDATE)
6262
{

‎src/include/access/brin_internal.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,12 @@ typedef struct BrinDesc
7272
#define BRIN_PROCNUM_UNION 4
7373
/* procedure numbers up to 10 are reserved for BRIN future expansion */
7474

75-
#define BRIN_DEBUG
75+
#undef BRIN_DEBUG
7676

77-
/* we allow debug if using GCC; otherwise don't bother */
78-
#if defined(BRIN_DEBUG) && defined(__GNUC__)
79-
#define BRIN_elog(level, ...) elog(level, __VA_ARGS__)
77+
#ifdef BRIN_DEBUG
78+
#define BRIN_elog(args) elog args
8079
#else
81-
#define BRIN_elog(a) void(0)
80+
#define BRIN_elog(args) ((void) 0)
8281
#endif
8382

8483
/* brin.c */

‎src/include/access/brin_xlog.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,10 @@ typedef struct xl_brin_insert
7676
typedef struct xl_brin_update
7777
{
7878
ItemPointerData oldtid;
79-
xl_brin_insert new;
79+
xl_brin_insert insert;
8080
} xl_brin_update;
8181

82-
#define SizeOfBrinUpdate (offsetof(xl_brin_update, new) + SizeOfBrinInsert)
82+
#define SizeOfBrinUpdate (offsetof(xl_brin_update, insert) + SizeOfBrinInsert)
8383

8484
/* This is what we need to know about a BRIN tuple samepage update */
8585
typedef struct xl_brin_samepage_update

0 commit comments

Comments
 (0)