totalSpace not properly initialized in AllocPage

• Key: DERBY-3116
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Knut Anders Hatlen
• Reporter: Knut Anders Hatlen
• Component/s: Store
• Affects Version/s: 10.4.1.3
• Fix Version/s: 10.4.1.3, 10.5.1.1

Description

There are some problems with the initialization of totalSpace in AllocPage. It is initialized in StoredPage.initSpace() which is again called from StoredPage.usePageBuffer(), and it is set to the value returned from AllocPage.getMaxFreeSpace(). The problems are:

  1) The calculation in getMaxFreeSpace() uses borrowedSpace, but when createIdentity() is called on an AllocPage, borrowedSpace has not been initialized when getMaxFreeSpace() is called and the calculated size is wrong.

  2) When a page object is reused, usePageBuffer() is only called if a new byte array must be allocated (because the new page has a different size than the old page). This means that the totalSize field gets the same value as in the old page if their sizes are equal, which is not necessarily correct.

Comments

[Knut Anders Hatlen]

I haven't run any tests on it yet, but this patch seems to make totalSpace have the correct value for all pages created in unit/T_RawStoreFactory.unit (verified by printing and comparing totalSpace to getMaxFreeSpace()). Will start a full run of regression tests and report back. I'll also see if I can add some asserts (since I know of no other way to expose the bug).

What this patch does, is:

  - in AllocPage.createPage(), set borrowedSpace before super.createPage() is called so that getMaxFreeSpace() returns the correct value when totalSpace is initialized

  - in CachedPage.setPageArray(), call usePageBuffer() also when the old buffer is reused. This ensures that totalSpace is recalculated when a page object is reused.

[Knut Anders Hatlen]

suites.All and derbyall ran cleanly with the d3116-1 patch.

[Jørgen Løland]

Hi Knut Anders.

Patch d3116-1 looks correct to me, but it seems a little overkill to always reinitiate the byte[] in StoredPage. Could you achieve the same thing by moving the initSpace call from StoredPage#usePageBuffer to StoredPage#createPage? Alternatively make initSpace protected and call StoredPage#initSpace from CachedPage#setPageArray when the page is reused?

Another bug? In StoredPage#initSpace, slotEntrySize is used when setting maxFieldSize. However, initSpace is called before slotEntrySize has been updated in usePageBuffer.

Not related to your patch: There is a problem with the Javadoc in AllocPage (unclosed paragraph "<p"). Fixing that would show more javadoc in the html files.

[Knut Anders Hatlen]

Thanks for looking at the patch, Jørgen!

> it seems a little overkill to always reinitiate the byte[] in StoredPage.

Note that setPageBuffer() doesn't reallocate the byte array, it only sets the reference to the byte array and refreshes some of the instance variables. I think the cost of these operations is negligible compared to the cost of evicting the old page and initializing the new one.

> Could you achieve the same thing by moving the initSpace call from StoredPage#usePageBuffer to StoredPage#createPage?

That might be possible, but then I think we would also have to move it into StoredPage.initFromData() as the same problem may occur if you read an alloc page from disk instead of creating a new one. It also seems like initSpace() depends on other variables initialized in usePageBuffer() (slotEntrySize in particular) so we'd probably end up with moving most of the code in usePageBuffer() into initSpace() anyway.

> Another bug? In StoredPage#initSpace, slotEntrySize is used when setting maxFieldSize. However, initSpace is called before slotEntrySize has been updated in usePageBuffer.

This is supposed to be fixed in the current trunk (see DERBY-3099). But thanks for verifiying that it in fact is a bug! :)

> Not related to your patch: There is a problem with the Javadoc in AllocPage (unclosed paragraph "<p"). Fixing that would show more javadoc in the html files.

Good catch! I fixed the tag and checked it in. Now I see one more line in the javadoc, but still there's much of it that doesn't show up. Perhaps there's a problem with the custom javadoc tags (format_id, purpose, etc)?

[Knut Anders Hatlen]

Uploading a new patch which resolves a conflict with a recent commit. The new patch also contains an assert which exposes the bug in the lack of a test for it.

 - Without the fixes, the assert will cause database creation to fail.
 - With the fix in AllocPage.createPage(), unit/T_RawStoreFactory.unit will fail when an AllocPage is evicted from the page cache to make room for an AllocPage with a different borrowedSpace value.
 - With the fix in CachedPage.setPageArray(), unit/T_RawStoreFactory.unit passes.

I'm re-running derbyall and suites.All now. Since there were no more comments on the previous patch, I intend to commit the updated patch to trunk and 10.4 if all the tests pass.

[Knut Anders Hatlen]

Committed to trunk with revision 644698.

[Knut Anders Hatlen]

Committed to 10.4 with revision 644967.