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
Attachments:
smgr-code-cleanup-1.patchtext/x-patchDownload+168-176
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