Re: PATCH: optimized DROP of multiple tables within a transaction

Hi Tomas,

On Tue, Jan 8, 2013 at 7:08 AM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
>> * I found another extra space after asterisk.
>>
>> + RelFileNode * nodes;
>
> Thanks, fixed.

check

>> * Curly bracket which starts code block should be at the head of next line.
>>
>> + /* extend the array if needed (double the size) */
>> + if (maxrels <= nrels) {
>
> Fixed.

check

>> * There are several trailing tabs in src/backend/catalog/storage.c.
>
> Fixed (I balieve).

check

>> * naming of DROP_RELATIONS_BSEARCH_LIMIT (or off-by-one bug?)
>> IIUC bsearch is used when # of relations to be dropped is *more* than
>> the value of DROP_RELATIONS_BSEARCH_LIMIT (10). IMO this behavior is
>> not what the macro name implies; I thought that bsearch is used for 10
>> relations. Besides, the word "LIMIT" is used as *upper limit* in
>> other macros. How about MIN_DROP_REL[ATION]S_BSEARCH or
>> DROP_REL[ATION]S_LINEAR_SEARCH_LIMIT?
>> # using RELS instead of RELATIONS would be better to shorten the name
>>
>
> I've changed the name to DROP_RELS_BSEARCH_THRESHOLD. I believe this
> naming is consistent with options like "geqo_threshold" - when the
> number of relations reaches the specified value, the bsearch is used.
>
> I've also increased the value from 10 to 20, in accordance with the
> previous discussion.

New name sounds good to me, but the #define says that the value is 15.
Should it be fixed to 20?

>> * +1 for Alvaro's suggestion about avoiding palloc traffic by starting
>> with 8 elements or so.
>>
>
> Done.

Not yet. Initial size of srels array is still 1 element.

Regards,
--
Shigeru HANADA