Chris Bandy <bandy.chris@gmail.com> writes:
> On Tue, Oct 7, 2014 at 5:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> + /* use strncmp so that we match truncated tokens */
>> result = strncmp(key, position->token, TOKMAXLEN);
> In your proposal you wanted to remove "crufty code that deals with
> non-null-terminated token strings". Is this some of that crufty code? Can
> it be removed?
Yeah, I had hoped to simplify these things to just strcmp, but on closer
inspection that didn't work, because some of the keywords in the table are
truncated at TOKMAXLEN (10 characters). If we just made these strcmp then
the code would stop recognizing e.g. "milliseconds".
I thought briefly about widening TOKMAXLEN so that there were no truncated
keywords in the table, but that seems risky from a backwards-compatibility
standpoint: as it stands, the code will accept "milliseconds",
"millisecond", or "millisecon", and there might possibly be applications
out there that depend on that. In any case I'd much rather keep the array
stride at 16 bytes for speed reasons; and who's to say we might not put in
some even-longer keywords in the future?
Another alternative we should maybe consider is leaving the definition
of the token field alone (ie, still not guaranteed null terminated)
which'd leave us with one free byte per datetkn entry. I can't think
of a likely reason to need another 1-byte field though, and the existing
definition is not without risk. That comment that was there about "don't
change this to strlcpy" was there because somebody broke it awhile back,
or at least submitted a patch that would've broken it if it'd been
accepted. People are too used to null-terminated strings in C; a field
definition that violates that norm is just trouble waiting to happen.
regards, tom lane