Changeset: e582a038df7a for MonetDB
URL: https://dev.monetdb.org/hg/MonetDB/rev/e582a038df7a
Modified Files:
gdk/gdk_bat.c
gdk/gdk_batop.c
gdk/gdk_bbp.c
gdk/gdk_hash.c
gdk/gdk_private.h
gdk/gdk_storage.c
Branch: default
Log Message:
Merge with Sep2022 branch.
diffs (truncated from 302 to 300 lines):
diff --git a/gdk/ChangeLog.Sep2022 b/gdk/ChangeLog.Sep2022
--- a/gdk/ChangeLog.Sep2022
+++ b/gdk/ChangeLog.Sep2022
@@ -1,6 +1,12 @@
# ChangeLog file for GDK
# This file is updated with Maddlog
+* Wed Dec 14 2022 Sjoerd Mullender <sjo...@acm.org>
+- Fixed a race condition where a hash could have been created on a
+ bat using the old bat count while in another thread the bat count
+ got updated. This would make the hash be based on too small a size,
+ causing failures later on.
+
* Thu Dec 8 2022 Sjoerd Mullender <sjo...@acm.org>
- When extending a bat failed, the capacity had been updated already and
was therefore too large. This could then later cause a crash. This has
diff --git a/gdk/gdk_bat.c b/gdk/gdk_bat.c
--- a/gdk/gdk_bat.c
+++ b/gdk/gdk_bat.c
@@ -1175,6 +1175,7 @@ BUNappendmulti(BAT *b, const void *value
}
BATiter bi = bat_iterator_nolock(b);
MT_lock_unset(&b->theaplock);
+ MT_rwlock_wrlock(&b->thashlock);
if (values && b->ttype) {
int (*atomcmp) (const void *, const void *) =
ATOMcompare(b->ttype);
const void *atomnil = ATOMnilptr(b->ttype);
@@ -1189,6 +1190,7 @@ BUNappendmulti(BAT *b, const void *value
t = ((void **) values)[i];
gdk_return rc = tfastins_nocheckVAR(b, p, t);
if (rc != GDK_SUCCEED) {
+ MT_rwlock_wrunlock(&b->thashlock);
return rc;
}
if (vbase != b->tvheap->base) {
@@ -1229,7 +1231,6 @@ BUNappendmulti(BAT *b, const void *value
}
p++;
}
- MT_rwlock_wrlock(&b->thashlock);
if (b->thash) {
p -= count;
for (BUN i = 0; i < count; i++) {
@@ -1239,7 +1240,6 @@ BUNappendmulti(BAT *b, const void *value
}
nunique = b->thash ? b->thash->nunique : 0;
}
- MT_rwlock_wrunlock(&b->thashlock);
} else if (ATOMstorage(b->ttype) == TYPE_msk) {
bi.minpos = bi.maxpos = BUN_NONE;
minvalp = maxvalp = NULL;
@@ -1253,7 +1253,6 @@ BUNappendmulti(BAT *b, const void *value
minvalp = BUNtloc(bi, bi.minpos);
if (bi.maxpos != BUN_NONE)
maxvalp = BUNtloc(bi, bi.maxpos);
- MT_rwlock_wrlock(&b->thashlock);
for (BUN i = 0; i < count; i++) {
t = (void *) ((char *) values + (i <<
b->tshift));
gdk_return rc = tfastins_nocheckFIX(b, p, t);
@@ -1284,14 +1283,12 @@ BUNappendmulti(BAT *b, const void *value
p++;
}
nunique = b->thash ? b->thash->nunique : 0;
- MT_rwlock_wrunlock(&b->thashlock);
}
MT_lock_set(&b->theaplock);
b->tminpos = bi.minpos;
b->tmaxpos = bi.maxpos;
MT_lock_unset(&b->theaplock);
} else {
- MT_rwlock_wrlock(&b->thashlock);
for (BUN i = 0; i < count; i++) {
gdk_return rc = tfastins_nocheck(b, p, t);
if (rc != GDK_SUCCEED) {
@@ -1304,13 +1301,13 @@ BUNappendmulti(BAT *b, const void *value
p++;
}
nunique = b->thash ? b->thash->nunique : 0;
- MT_rwlock_wrunlock(&b->thashlock);
}
MT_lock_set(&b->theaplock);
BATsetcount(b, p);
if (nunique != 0)
b->tunique_est = (double) nunique;
MT_lock_unset(&b->theaplock);
+ MT_rwlock_wrunlock(&b->thashlock);
IMPSdestroy(b); /* no support for inserts in imprints yet */
OIDXdestroy(b);
@@ -1588,14 +1585,11 @@ BUNinplacemulti(BAT *b, const oid *posit
}
if (b->twidth < SIZEOF_VAR_T &&
(b->twidth <= 2 ? _d - GDK_VAROFFSET : _d) >=
((size_t) 1 << (8 << b->tshift))) {
- /* doesn't fit in current heap, upgrade
- * it, can't keep hashlock while doing
- * so */
- MT_rwlock_wrunlock(&b->thashlock);
+ /* doesn't fit in current heap, upgrade it */
if (GDKupgradevarheap(b, _d, 0, bi.count) !=
GDK_SUCCEED) {
+ MT_rwlock_wrunlock(&b->thashlock);
return GDK_FAIL;
}
- MT_rwlock_wrlock(&b->thashlock);
}
/* reinitialize iterator after possible heap upgrade */
{
diff --git a/gdk/gdk_batop.c b/gdk/gdk_batop.c
--- a/gdk/gdk_batop.c
+++ b/gdk/gdk_batop.c
@@ -304,12 +304,12 @@ insert_string_bat(BAT *b, BATiter *ni, s
}
}
TIMEOUT_CHECK(timeoffset, TIMEOUT_HANDLER(GDK_FAIL));
+ MT_rwlock_wrlock(&b->thashlock);
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++) {
HASHappend_locked(b, r, b->tvheap->base + VarHeapVal(Tloc(b,
0), r, b->twidth));
}
@@ -382,11 +382,11 @@ append_varsized_bat(BAT *b, BATiter *ni,
*dst++ = src[canditer_next(ci) - hseq];
}
}
+ MT_rwlock_wrlock(&b->thashlock);
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;
b->thash && i < BATcount(b);
i++) {
@@ -477,12 +477,12 @@ append_varsized_bat(BAT *b, BATiter *ni,
}
}
BUN nunique = b->thash ? b->thash->nunique : 0;
- MT_rwlock_wrunlock(&b->thashlock);
MT_lock_set(&b->theaplock);
BATsetcount(b, r);
if (nunique != 0)
b->tunique_est = (double) nunique;
MT_lock_unset(&b->theaplock);
+ MT_rwlock_wrunlock(&b->thashlock);
return GDK_SUCCEED;
}
@@ -904,12 +904,12 @@ BATappend2(BAT *b, BAT *n, BAT *s, bool
}
BUN nunique;
nunique = b->thash ? b->thash->nunique : 0;
- MT_rwlock_wrunlock(&b->thashlock);
MT_lock_set(&b->theaplock);
BATsetcount(b, b->batCount + ci.ncand);
if (nunique != 0)
b->tunique_est = (double) nunique;
MT_lock_unset(&b->theaplock);
+ MT_rwlock_wrunlock(&b->thashlock);
}
doreturn:
@@ -1320,16 +1320,10 @@ BATappend_or_update(BAT *b, BAT *p, cons
}
if (b->twidth < SIZEOF_VAR_T &&
(b->twidth <= 2 ? d - GDK_VAROFFSET : d) >=
((size_t) 1 << (8 << b->tshift))) {
- /* doesn't fit in current heap, upgrade
- * it, can't keep hashlock while doing
- * so */
- MT_rwlock_wrunlock(&b->thashlock);
- locked = false;
+ /* doesn't fit in current heap, upgrade it */
if (GDKupgradevarheap(b, d, 0, MAX(updid,
b->batCount)) != GDK_SUCCEED) {
goto bailout;
}
- MT_rwlock_wrlock(&b->thashlock);
- locked = true;
}
/* in case ATOMreplaceVAR and/or
* GDKupgradevarheap replaces a heap, we need to
@@ -1373,6 +1367,7 @@ BATappend_or_update(BAT *b, BAT *p, cons
b->tvheap->dirty = true;
MT_lock_unset(&b->theaplock);
} else if (ATOMstorage(b->ttype) == TYPE_msk) {
+ assert(b->thash == NULL);
HASHdestroy(b); /* hash doesn't make sense for msk */
for (BUN i = 0; i < ni.count; i++) {
oid updid;
diff --git a/gdk/gdk_bbp.c b/gdk/gdk_bbp.c
--- a/gdk/gdk_bbp.c
+++ b/gdk/gdk_bbp.c
@@ -3786,9 +3786,7 @@ BBPsync(int cnt, bat *restrict subcommit
BBP_status_on(i, BBPSAVING);
if (lock)
MT_lock_unset(&GDKswapLock(i));
- MT_rwlock_rdlock(&b->thashlock);
- ret = BATsave_locked(b, &bi, size);
- MT_rwlock_rdunlock(&b->thashlock);
+ ret = BATsave_iter(b, &bi, size);
BBP_status_off(i, BBPSAVING);
}
} else {
diff --git a/gdk/gdk_hash.c b/gdk/gdk_hash.c
--- a/gdk/gdk_hash.c
+++ b/gdk/gdk_hash.c
@@ -609,6 +609,7 @@ BAThashsave_intern(BAT *b, bool dosync)
/* only persist if parent BAT hasn't changed in the
* mean time */
if (!b->theap->dirty &&
+ ((size_t *) h->heapbckt.base)[1] == BATcount(b) &&
((size_t *) h->heapbckt.base)[4] == BATcount(b) &&
HEAPsave(&h->heaplink, h->heaplink.filename, NULL, dosync,
h->heaplink.free, NULL) == GDK_SUCCEED &&
HEAPsave(&h->heapbckt, h->heapbckt.filename, NULL, dosync,
h->heapbckt.free, NULL) == GDK_SUCCEED) {
diff --git a/gdk/gdk_private.h b/gdk/gdk_private.h
--- a/gdk/gdk_private.h
+++ b/gdk/gdk_private.h
@@ -86,7 +86,7 @@ void BATrmprop(BAT *b, enum prop_t idx)
__attribute__((__visibility__("hidden")));
void BATrmprop_nolock(BAT *b, enum prop_t idx)
__attribute__((__visibility__("hidden")));
-gdk_return BATsave_locked(BAT *bd, BATiter *bi, BUN size)
+gdk_return BATsave_iter(BAT *bd, BATiter *bi, BUN size)
__attribute__((__visibility__("hidden")));
void BATsetdims(BAT *b, uint16_t width)
__attribute__((__visibility__("hidden")));
diff --git a/gdk/gdk_storage.c b/gdk/gdk_storage.c
--- a/gdk/gdk_storage.c
+++ b/gdk/gdk_storage.c
@@ -661,23 +661,31 @@ DESCload(int i)
}
gdk_return
-BATsave_locked(BAT *b, BATiter *bi, BUN size)
+BATsave_iter(BAT *b, BATiter *bi, BUN size)
{
gdk_return err = GDK_SUCCEED;
const char *nme;
bool dosync;
+ bool locked = false;
BATcheck(b, GDK_FAIL);
+ if (MT_rwlock_rdtry(&b->thashlock))
+ locked = true;
+
dosync = (BBP_status(b->batCacheid) & BBPPERSISTENT) != 0;
assert(!GDKinmemory(bi->h->farmid));
/* views cannot be saved, but make an exception for
* force-remapped views */
if (isVIEW(b)) {
+ if (locked)
+ MT_rwlock_rdunlock(&b->thashlock);
GDKerror("%s is a view on %s; cannot be saved\n", BATgetId(b),
BBP_logical(VIEWtparent(b)));
return GDK_FAIL;
}
if (!BATdirtybi(*bi)) {
+ if (locked)
+ MT_rwlock_rdunlock(&b->thashlock);
return GDK_SUCCEED;
}
@@ -760,9 +768,11 @@ BATsave_locked(BAT *b, BATiter *bi, BUN
b->batCopiedtodisk = true;
}
MT_lock_unset(&b->theaplock);
- if (b->thash && b->thash != (Hash *) 1)
+ if (locked && b->thash && b->thash != (Hash *) 1)
BAThashsave(b, dosync);
}
+ if (locked)
+ MT_rwlock_rdunlock(&b->thashlock);
return err;
}
@@ -772,9 +782,7 @@ BATsave(BAT *b)
gdk_return rc;
BATiter bi = bat_iterator(b);
- MT_rwlock_rdlock(&b->thashlock);
- rc = BATsave_locked(b, &bi, bi.count);
- MT_rwlock_rdunlock(&b->thashlock);
+ rc = BATsave_iter(b, &bi, bi.count);
bat_iterator_end(&bi);
return rc;
}
diff --git a/sql/ChangeLog.Sep2022 b/sql/ChangeLog.Sep2022
--- a/sql/ChangeLog.Sep2022
+++ b/sql/ChangeLog.Sep2022
@@ -1,6 +1,11 @@
# ChangeLog file for sql
# This file is updated with Maddlog
+* Wed Dec 14 2022 Sjoerd Mullender <sjo...@acm.org>
+- Fixed cleanup after a failed allocation where the data being cleaned
+ up was unitialized but still used as pointers to memory that also had
+ to be freed.
+
* Wed Dec 7 2022 Sjoerd Mullender <sjo...@acm.org>
_______________________________________________
checkin-list mailing list -- checkin-list@monetdb.org
To unsubscribe send an email to checkin-list-le...@monetdb.org
