Changeset: 5323d7b6d951 for MonetDB
URL: https://dev.monetdb.org/hg/MonetDB/rev/5323d7b6d951
Modified Files:
gdk/gdk.h
gdk/gdk_aggr.c
gdk/gdk_align.c
gdk/gdk_bat.c
gdk/gdk_batop.c
gdk/gdk_bbp.c
gdk/gdk_delta.c
gdk/gdk_group.c
gdk/gdk_orderidx.c
gdk/gdk_select.c
gdk/gdk_storage.c
gdk/gdk_strimps.c
gdk/gdk_tm.c
gdk/gdk_unique.c
monetdb5/mal/mal_dataflow.c
monetdb5/mal/mal_debugger.c
monetdb5/mal/mal_profiler.c
monetdb5/modules/kernel/bat5.c
sql/storage/bat/bat_storage.c
Branch: default
Log Message:
Fix data races. Add more values to the BAT iterator.
diffs (truncated from 1100 to 300 lines):
diff --git a/gdk/gdk.h b/gdk/gdk.h
--- a/gdk/gdk.h
+++ b/gdk/gdk.h
@@ -774,6 +774,19 @@ typedef enum {
BAT_APPEND, /* only reads and appends allowed */
} restrict_t;
+/* batDirtydesc: should be set (true) if any of the following fields
+ * have changed since the bat was last saved: hseqbase, batRestricted,
+ * batTransient, batCount, and the theap properties tkey, tseqbase,
+ * tsorted, trevsorted, twidth, tshift, tnonil, tnil, tnokey, tnosorted,
+ * tnorevsorted, tminpos, tmaxpos, and tunique_est; in addition, the
+ * value should be set if the BBP field BBP_logical(bid) is changed.
+ *
+ * theaplock: this lock should be held when reading or writing any of
+ * the fields mentioned above for batDirtydesc, and also when reading or
+ * writing any of the following fields: batDirtydesc, theap, tvheap,
+ * batInserted, batCapacity. There is no need for the lock if the bat
+ * cannot possibly be modified concurrently, e.g. when it is new and not
+ * yet returned to the interpreter or during system initialization. */
typedef struct BAT {
/* static bat properties */
oid hseqbase; /* head seq base */
@@ -961,7 +974,11 @@ typedef struct BATiter {
sorted:1,
revsorted:1,
hdirty:1,
- vhdirty:1;
+ vhdirty:1,
+ dirtydesc:1,
+ copiedtodisk:1,
+ transient:1;
+ restrict_t restricted:2;
union {
oid tvid;
bool tmsk;
@@ -1003,8 +1020,14 @@ bat_iterator_nolock(BAT *b)
.nil = b->tnil,
.sorted = b->tsorted,
.revsorted = b->trevsorted,
- .hdirty = b->theap->dirty,
- .vhdirty = b->tvheap && b->tvheap->dirty,
+ /* only look at heap dirty flag if we own it */
+ .hdirty = b->theap->parentid == b->batCacheid &&
b->theap->dirty,
+ /* also, if there is no vheap, it's not dirty */
+ .vhdirty = b->tvheap && b->tvheap->parentid ==
b->batCacheid && b->tvheap->dirty,
+ .dirtydesc = b->batDirtydesc,
+ .copiedtodisk = b->batCopiedtodisk,
+ .transient = b->batTransient,
+ .restricted = b->batRestricted,
#ifndef NDEBUG
.locked = false,
#endif
@@ -1282,6 +1305,10 @@ gdk_export restrict_t BATgetaccess(BAT *
(b)->batDirtydesc || \
(b)->theap->dirty || \
((b)->tvheap != NULL && (b)->tvheap->dirty))
+#define BATdirtybi(bi) (!(bi).copiedtodisk || \
+ (bi).dirtydesc || \
+ (bi).hdirty || \
+ (bi).vhdirty)
#define BATdirtydata(b) (!(b)->batCopiedtodisk ||
\
(b)->theap->dirty || \
((b)->tvheap != NULL && (b)->tvheap->dirty))
@@ -1396,7 +1423,8 @@ gdk_export void GDKqsort(void *restrict
#define BATtvoid(b) (BATtdense(b) || (b)->ttype==TYPE_void)
#define BATtkey(b) ((b)->tkey || BATtdense(b))
-/* set some properties that are trivial to deduce */
+/* set some properties that are trivial to deduce; called with theaplock
+ * held */
static inline void
BATsettrivprop(BAT *b)
{
diff --git a/gdk/gdk_aggr.c b/gdk/gdk_aggr.c
--- a/gdk/gdk_aggr.c
+++ b/gdk/gdk_aggr.c
@@ -3746,8 +3746,11 @@ BATmin_skipnil(BAT *b, void *aggr, bit s
bi.width, 0, bi.count,
ATOMnilptr(bi.type), 1, 1);
if (r == 0) {
- bi.nonil = true;
+ /* there are no nils, record that */
+ MT_lock_set(&b->theaplock);
+ b->tnonil = true;
b->batDirtydesc = true;
+ MT_lock_unset(&b->theaplock);
}
} else {
r = 0;
diff --git a/gdk/gdk_align.c b/gdk/gdk_align.c
--- a/gdk/gdk_align.c
+++ b/gdk/gdk_align.c
@@ -262,11 +262,11 @@ BATmaterialize(BAT *b)
b->tbaseoff = 0;
b->theap->dirty = true;
b->tunique_est = is_oid_nil(t) ? 1.0 : (double) b->batCount;
- MT_lock_unset(&b->theaplock);
b->ttype = TYPE_oid;
BATsetdims(b, 0);
b->batDirtydesc = true;
BATsetcount(b, b->batCount);
+ MT_lock_unset(&b->theaplock);
HEAPdecref(h, false);
if (vh)
HEAPdecref(vh, true);
diff --git a/gdk/gdk_bat.c b/gdk/gdk_bat.c
--- a/gdk/gdk_bat.c
+++ b/gdk/gdk_bat.c
@@ -1033,9 +1033,11 @@ BUNappendmulti(BAT *b, const void *value
}
}
if (dense) {
+ MT_lock_set(&b->theaplock);
if (b->batCount == 0)
b->tseqbase = ovals ? ovals[0] : oid_nil;
BATsetcount(b, BATcount(b) + count);
+ MT_lock_unset(&b->theaplock);
return GDK_SUCCEED;
} else {
/* we need to materialize b; allocate enough capacity */
@@ -1273,7 +1275,9 @@ BUNappendmulti(BAT *b, const void *value
}
}
MT_rwlock_wrunlock(&b->thashlock);
+ MT_lock_set(&b->theaplock);
BATsetcount(b, p);
+ MT_lock_unset(&b->theaplock);
IMPSdestroy(b); /* no support for inserts in imprints yet */
OIDXdestroy(b);
@@ -1307,12 +1311,12 @@ BUNdelete(BAT *b, oid o)
return GDK_FAIL;
}
TRC_DEBUG(ALGO, ALGOBATFMT " deleting oid " OIDFMT "\n", ALGOBATPAR(b),
o);
- b->batDirtydesc = true;
val = BUNtail(bi, p);
/* writing the values should be locked, reading could be done
* unlocked (since we're the only thread that should be changing
* anything) */
MT_lock_set(&b->theaplock);
+ b->batDirtydesc = true;
if (b->tmaxpos == p)
b->tmaxpos = BUN_NONE;
if (b->tminpos == p)
@@ -1348,18 +1352,19 @@ BUNdelete(BAT *b, oid o)
MT_lock_unset(&b->theaplock);
}
/* no longer sorted */
+ MT_lock_set(&b->theaplock);
b->tsorted = b->trevsorted = false;
b->theap->dirty = true;
+ MT_lock_unset(&b->theaplock);
}
+ MT_lock_set(&b->theaplock);
if (b->tnosorted >= p)
b->tnosorted = 0;
if (b->tnorevsorted >= p)
b->tnorevsorted = 0;
- MT_lock_set(&b->theaplock);
b->batCount--;
if (BATcount(b) < gdk_unique_estimate_keep_fraction)
b->tunique_est = 0;
- MT_lock_unset(&b->theaplock);
if (b->batCount <= 1) {
/* some trivial properties */
b->tkey = true;
@@ -1370,6 +1375,7 @@ BUNdelete(BAT *b, oid o)
b->tnonil = true;
}
}
+ MT_lock_unset(&b->theaplock);
IMPSdestroy(b);
OIDXdestroy(b);
return GDK_SUCCEED;
@@ -1612,6 +1618,7 @@ BUNinplacemulti(BAT *b, const oid *posit
prv = p > 0 ? p - 1 : BUN_NONE;
nxt = p < last ? p + 1 : BUN_NONE;
+ MT_lock_set(&b->theaplock);
if (b->tsorted) {
if (prv != BUN_NONE &&
ATOMcmp(tt, t, BUNtail(bi, prv)) < 0) {
@@ -1653,6 +1660,7 @@ BUNinplacemulti(BAT *b, const oid *posit
b->tnokey[0] = b->tnokey[1] = 0;
if (b->tnonil && ATOMstorage(b->ttype) != TYPE_msk)
b->tnonil = t && ATOMcmp(b->ttype, t,
ATOMnilptr(b->ttype)) != 0;
+ MT_lock_unset(&b->theaplock);
}
MT_rwlock_wrunlock(&b->thashlock);
MT_lock_set(&b->theaplock);
@@ -1864,6 +1872,9 @@ BATsetcapacity(BAT *b, BUN cnt)
assert(b->batCount <= cnt);
}
+/* Set the batCount value for the bat and also set some dependent
+ * properties. This function should be called only when it is save from
+ * concurrent use (e.g. when theaplock is being held). */
void
BATsetcount(BAT *b, BUN cnt)
{
@@ -1871,7 +1882,6 @@ BATsetcount(BAT *b, BUN cnt)
assert(!is_oid_nil(b->hseqbase));
assert(cnt <= BUN_MAX);
- MT_lock_set(&b->theaplock);
b->batCount = cnt;
b->batDirtydesc = true;
if (b->theap->parentid == b->batCacheid) {
@@ -1908,7 +1918,6 @@ BATsetcount(BAT *b, BUN cnt)
}
}
assert(b->batCapacity >= cnt);
- MT_lock_unset(&b->theaplock);
}
/*
@@ -2258,7 +2267,6 @@ BATcheckmodes(BAT *b, bool existing)
return GDK_FAIL;
if (dirty) {
- b->batDirtydesc = true;
b->theap->newstorage = m1;
if (b->tvheap)
b->tvheap->newstorage = m3;
@@ -2417,6 +2425,7 @@ BATmode(BAT *b, bool transient)
}
/* session bats or persistent bats that did not
* witness a commit yet may have been saved */
+ MT_lock_set(&b->theaplock);
if (b->batCopiedtodisk) {
if (!transient) {
BBP_status_off(bid, BBPTMP);
@@ -2427,6 +2436,7 @@ BATmode(BAT *b, bool transient)
}
}
b->batTransient = transient;
+ MT_lock_unset(&b->theaplock);
MT_lock_unset(&GDKswapLock(bid));
}
return GDK_SUCCEED;
diff --git a/gdk/gdk_batop.c b/gdk/gdk_batop.c
--- a/gdk/gdk_batop.c
+++ b/gdk/gdk_batop.c
@@ -304,8 +304,10 @@ insert_string_bat(BAT *b, BATiter *ni, s
r++;
}
}
+ MT_lock_set(&b->theaplock);
BATsetcount(b, oldcnt + ci->ncand);
assert(b->batCapacity >= b->batCount);
+ MT_lock_unset(&b->theaplock);
/* maintain hash */
MT_rwlock_wrlock(&b->thashlock);
for (r = oldcnt, cnt = BATcount(b); b->thash && r < cnt; r++) {
@@ -376,7 +378,9 @@ append_varsized_bat(BAT *b, BATiter *ni,
*dst++ = src[canditer_next(ci) - hseq];
}
}
+ MT_lock_set(&b->theaplock);
BATsetcount(b, BATcount(b) + ci->ncand);
+ MT_lock_unset(&b->theaplock);
/* maintain hash table */
MT_rwlock_wrlock(&b->thashlock);
for (BUN i = BATcount(b) - ci->ncand;
@@ -429,7 +433,9 @@ append_varsized_bat(BAT *b, BATiter *ni,
r++;
}
MT_rwlock_wrunlock(&b->thashlock);
+ MT_lock_set(&b->theaplock);
BATsetcount(b, r);
+ MT_lock_unset(&b->theaplock);
return GDK_SUCCEED;
}
@@ -656,12 +662,12 @@ BATappend2(BAT *b, BAT *n, BAT *s, bool
return GDK_FAIL;
}
- b->batDirtydesc = true;
-
IMPSdestroy(b); /* imprints do not support updates yet */
OIDXdestroy(b);
STRMPdestroy(b); /* TODO: use STRMPappendBitString */
MT_lock_set(&b->theaplock);
+ b->batDirtydesc = true;
+
if (BATcount(b) == 0 || b->tmaxpos != BUN_NONE) {
if (ni.maxpos != BUN_NONE) {
BATiter bi = bat_iterator_nolock(b);
@@ -700,6 +706,7 @@ BATappend2(BAT *b, BAT *n, BAT *s, bool
if (b->ttype == TYPE_void) {
/* b does not have storage, keep it that way if we can */
HASHdestroy(b); /* we're not maintaining the hash here */
+ MT_lock_set(&b->theaplock);
if (BATtdense(n) && ci.tpe == cand_dense &&
(BATcount(b) == 0 ||
(BATtdense(b) &&
@@ -708,6 +715,7 @@ BATappend2(BAT *b, BAT *n, BAT *s, bool
if (BATcount(b) == 0)
_______________________________________________
checkin-list mailing list -- [email protected]
To unsubscribe send an email to [email protected]
