On 2014-05-05 18:50:59 -0400, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.com> writes:
> > On 2014-05-05 15:41:22 -0400, Tom Lane wrote:
> >> Looks all right to me. Yeah, the right shift might have undefined
> >> high-order bits, but we don't care because we're storing the result
> >> into an int16.
>
> > Doesn't at the very least
> > rnode.backend = (msg->sm.backend_hi << 16) | (int)
> > msg->sm.backend_lo;
> > have to be
> > rnode.backend = ((int) msg->sm.backend_hi << 16) | (int)
> > msg->sm.backend_lo;
>
> A promotion to int (or wider) is implicit in any arithmetic operation,
> last I checked the C standard. The "(int)" on the other side isn't
> necessary either.
Done in the attached patch.
> We might actually be better off casting both sides to unsigned int,
> just to enforce that the left shifting is done with unsigned semantics.
>
> >>> c) The ProcessMessageList() calls access msg->rc.id to test for the type
> >>> of the existing message. That's not nice.
>
> >> Huh?
>
> > The checks should access msg->id, not msg->rc.id...
>
> Meh. If they're not the same thing, we've got big trouble anyway.
> But if you want to change it as a cosmetic thing, no objection.
Changed as well.
On 2014-05-05 15:41:22 -0400, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.com> writes:
> > a) SICleanupQueue() sometimes releases and reacquires the write lock
> > held on the outside. That's pretty damn fragile, not to mention
> > ugly. Even slight reformulations of the code in SIInsertDataEntries()
> > can break this... Can we please extend the comment in the latter that
> > to mention the lock dropping explicitly?
>
> Want to write a better comment?
Edited the comment and, in a perhaps debatable fashion, moved some
variable declarations + volatile into the retry loop for some added
cozyness. If the compiler inlines the cleanup function it could very
well decide to fetch some of the variables only once.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From a24973508710c965035fca45e933823ce49a5a4f Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Thu, 8 May 2014 16:13:44 +0200
Subject: [PATCH] Minor cleanups and comment improvements in the cache
invalidation code.
---
src/backend/storage/ipc/sinvaladt.c | 11 ++++++++---
src/backend/utils/cache/inval.c | 9 +++++----
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index 0328660..3966ccc 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -453,7 +453,6 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n)
while (n > 0)
{
int nthistime = Min(n, WRITE_QUANTUM);
- int numMsgs;
int max;
int i;
@@ -466,11 +465,17 @@ SIInsertDataEntries(const SharedInvalidationMessage *data, int n)
* queue and reset anyone who is preventing space from being freed.
* Otherwise, clean the queue only when it's exceeded the next
* fullness threshold. We have to loop and recheck the buffer state
- * after any call of SICleanupQueue.
+ * after any call of SICleanupQueue because the cleanup sometimes
+ * require releasing the write lock acquired above.
*/
for (;;)
{
- numMsgs = segP->maxMsgNum - segP->minMsgNum;
+ int numMsgs;
+ /* use volatile pointer to prevent code rearrangement */
+ volatile SISeg *vsegP = segP;
+
+ numMsgs = vsegP->maxMsgNum - vsegP->minMsgNum;
+
if (numMsgs + nthistime > MAXNUMMESSAGES ||
numMsgs >= segP->nextThreshold)
SICleanupQueue(true, nthistime);
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index dd46e18..4c3197b 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -374,7 +374,7 @@ AddRelcacheInvalidationMessage(InvalidationListHeader *hdr,
/* Don't add a duplicate item */
/* We assume dbId need not be checked because it will never change */
ProcessMessageList(hdr->rclist,
- if (msg->rc.id == SHAREDINVALRELCACHE_ID &&
+ if (msg->id == SHAREDINVALRELCACHE_ID &&
msg->rc.relId == relId)
return);
@@ -400,7 +400,7 @@ AddSnapshotInvalidationMessage(InvalidationListHeader *hdr,
/* Don't add a duplicate item */
/* We assume dbId need not be checked because it will never change */
ProcessMessageList(hdr->rclist,
- if (msg->sn.id == SHAREDINVALSNAPSHOT_ID &&
+ if (msg->id == SHAREDINVALSNAPSHOT_ID &&
msg->sn.relId == relId)
return);
@@ -580,7 +580,8 @@ LocalExecuteInvalidationMessage(SharedInvalidationMessage *msg)
RelFileNodeBackend rnode;
rnode.node = msg->sm.rnode;
- rnode.backend = (msg->sm.backend_hi << 16) | (int) msg->sm.backend_lo;
+ rnode.backend = (BackendId) (((uint32) msg->sm.backend_hi << 16)
+ |(uint32) msg->sm.backend_lo);
smgrclosenode(rnode);
}
else if (msg->id == SHAREDINVALRELMAP_ID)
@@ -1257,7 +1258,7 @@ CacheInvalidateSmgr(RelFileNodeBackend rnode)
SharedInvalidationMessage msg;
msg.sm.id = SHAREDINVALSMGR_ID;
- msg.sm.backend_hi = rnode.backend >> 16;
+ msg.sm.backend_hi = (uint32) rnode.backend >> 16;
msg.sm.backend_lo = rnode.backend & 0xffff;
msg.sm.rnode = rnode.node;
/* check AddCatcacheInvalidationMessage() for an explanation */
--
1.8.5.rc2.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
