Hi Manu,
ciccettino@stripped wrote:
> Hello to everybody,
> I realized a mod of the heap engine to support gis data (via blobs).
This looks like great work!
Unfortunately I could only take a quick look so far. I will do a deeper
review later. But I need to move it to a newer tree first. And I will
need to change the C++ style comments ("//") to C style comments
("/**/") in C files. Also I am used to look at BitKeeper diffs.
I can do all this myself. However, if you want to help, it would be
great if you could pull the latest 5.1 bk tree and install your changes
on it. Then do "bk -r diffs -u" and post the result. If this doesn't
work with a free bk license, then a tar archive is also welcome.
However, when sending diffs, you don't need to have comments like
"//manu: modded for blob support". The diffs show exactly what you
changed. They will be bundled with changeset comments eventually so that
their purpose remains clear.
Another thing is that I would highly appreciate to see a testcase. It
should at least show the new abilities of the server.
If we decide to accept your patch, then we would need to develop tests
that do also try to address all possible errors. We use "gcov" to see if
we managed to test all testable code paths. But it may be too early to
do this now. It would be lost effort in the case that we reject the patch.
And finally there is a conceptual problem. You allocate memory for the
blobs with my_malloc(). This means that blobs add to
max_heap_table_size. I think this is not acceptable. IMHO you need to
copy the blobs somewhere into the table buffer. So that we have a fixed
buffer of max_heap_table_size. But this can also be done later after we
made an opinion if we want to go with your patch.
More to come later. Thank you for your great work!
Regards
Ingo
--
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Hans von Bell, Kaj Arnö - HRB München 162140
