Thread: minor smgr code cleanup

This patch cleans up some code in the smgr and elsewhere:

     - Update comment in IsReservedName() to the present day, remove
       "ugly coding for speed" as this is no longer a performance
       critical function

     - Improve some variable & function names in commands/vacuum.c. I
       was planning to rewrite this to avoid lappend(), but since I
       still intend to do the list rewrite, there's no need for that.

     - Update some smgr comments which seemed to imply that we still
       forced all dirty pages to disk at commit-time.

     - Replace some #ifdef DIAGNOSTIC code with assertions.

     - Make the distinction between OS-level file descriptors and
       virtual file descriptors a little clearer in a few comments

     - Other minor comment improvements in the smgr code

Unless anyone objects, I intend to apply this code in about 48 hours.

-Neil

Neil Conway <neilc@samurai.com> writes:
>   bool
>   IsReservedName(const char *name)
>   {
> !     /* ugly coding for speed */
> !     return (name[0] == 'p' &&
> !             name[1] == 'g' &&
> !             name[2] == '_');
>   }
> --- 160,178 ----
>   bool
>   IsReservedName(const char *name)
>   {
> !     return strncmp(name, "pg_", 3);
>   }

This change is actually wrong (backwards), no?  You want a true result
on equality.

In any case I don't think this is a step forward in readability, and it
also poses a portability risk.  You should always write such tests as

    return strncmp(name, "pg_", 3) == 0;

(or != 0 as appropriate).  Pretending that the result of strcmp is a
bool is a type pun, and one that can rise up to bite you.  In the case
at hand, strncmp is allowed to return (say) 256 to indicate a nonzero
result --- which would be lost when the value is squeezed into a bool
(char).  See the archives; we've had at least one bug of this ilk.

            regards, tom lane

Neil Conway <neilc@samurai.com> writes:
>        I was planning to rewrite this to avoid lappend(), but since I
>        still intend to do the list rewrite, there's no need for that.

BTW, I was planning to ask you where that stood.  I've been writing new
code on the assumption that I didn't have to bother with FastList, so
if we don't get the List rewrite done, I'm gonna have to revisit some
things ...

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> BTW, I was planning to ask you where that stood.  I've been writing
> new code on the assumption that I didn't have to bother with
> FastList

Yeah, I definitely plan to have the list rewrite finished in time for
7.5. After the bufmgr work, it's the next thing on my plate.

-Neil

Tom Lane <tgl@sss.pgh.pa.us> writes:
> This change is actually wrong (backwards), no?  You want a true result
> on equality.

*sigh*, I'm an idiot. Thanks for catching my mistake!

> Pretending that the result of strcmp is a bool is a type pun, and
> one that can rise up to bite you.  In the case at hand, strncmp is
> allowed to return (say) 256 to indicate a nonzero result --- which
> would be lost when the value is squeezed into a bool (char).  See
> the archives; we've had at least one bug of this ilk.

Good point, and good to know. I just left the code as it previously
was (but updated the comment), and applied the patch.

Thanks again.

-Neil