> Two comments:
>
> - I notice you getting rid of { } pairs eg:
>
> if (condition)
> {
> dosomething();
> }
>
> becomes
>
> if (condition)
> dosomething();
>
> Is this policy? I prefer to have the braces almost always as I find
> it easier to read, and less error prone if someone adds a statement or an
> else clause, so in most of my patches, I would tend to put braces in.
> If you are busy taking them out simaltaniously, this could get silly.
I think several developers agreed that they were just wasting screen
space. The code is large enough without brace noise. I have considered
writing a script to remove the single-statement braces, but have not
done it yet.
If people would like to re-vote on this issue, I would be glad to hear
about it.
>
> Btw, I have been badly bit by:
>
> if (condition);
> dosomething();
>
> which turned out to be very hard to see indeed.
Sure, but braces don't help you either. This is just as legal:
if (condition);
{
dosomething();
}
>
>
> - I think the bit at line 1295-1309 might want to do all the work before
> the elog. Otherwise the elog leave the buffer cache polluted with buffers
> belonging to a mostly deleted index. Eg:
>
> + ReleaseRelationBuffers(indexRelation);
> +
> fname = relpath(indexRelation->rd_rel->relname.data);
> status = FileNameUnlink(fname);
>
> index_close(indexRelation);
> + RelationForgetRelation(indexRelation->rd_id);
>
> if (status < 0)
> elog(ERROR, "amdestroyr: unlink: %m");
Yes, that is true, but I kept the order as used in the rest of the code,
figuring the original coder knew better than I do. IMHO, if we get that
"amdestroyr" error, we have bigger problems than an invalid relation
cache
--
Bruce Momjian | 830 Blythe Avenue
maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)
