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

On 7.1.2013 10:07, Shigeru Hanada wrote:
> Hi Tomas,
>
> I reviewed v6 patch, and found that several places need fix.
> Sorry for extra nitpickings.
>
> * I found another extra space after asterisk.
>
> + RelFileNode * nodes;

Thanks, fixed.

>
> * 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.

>
> * There are several trailing tabs in src/backend/catalog/storage.c.

Fixed (I balieve).

> * 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.

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

Done.

regards
Tomas