Bunch of tsearch fixes and cleanup

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

- readstopwords calls recode_and_lowerstr directly, instead of using the
"wordop" function pointer in StopList struct. All callers used
recode_and_lowerstr anyway, so this simplifies the code a little bit. Is
there any external dictionary implementations that would require
different behavior?

I don't think eliminating wordop altogether is such a hot idea; some
dictionary could possibly want to do different processing than that.

Something that was annoying me yesterday was that it was not clear
whether we had fixed every single place that uses a tsearch config file
to assume that the file is in UTF8 and should be converted to database
encoding. So I was thinking of hardwiring the "recode" part into
readstopwords, and using wordop just for the "lowercase" part, which
seemed to me like a saner division of labor. That is, UTF8 is a policy
that we want to enforce globally, but lowercasing maybe not, and this
still leaves the door open for more processing besides lowercasing.

Oleg, Teodor, what do you think about this?

regards, tom lane

Tom Lane wrote:

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

- readstopwords calls recode_and_lowerstr directly, instead of using the
"wordop" function pointer in StopList struct. All callers used
recode_and_lowerstr anyway, so this simplifies the code a little bit. Is
there any external dictionary implementations that would require
different behavior?

I don't think eliminating wordop altogether is such a hot idea; some
dictionary could possibly want to do different processing than that.

Ok.

Something that was annoying me yesterday was that it was not clear
whether we had fixed every single place that uses a tsearch config file
to assume that the file is in UTF8 and should be converted to database
encoding.

I'm afraid there's still a lot of inconsistencies in that. I'm just
looking at dict_synonym, and it looks like it has the same problem I
patched in readstopwords; it's using pg_verifymbstr, with database
encoding, to verify the input file. It also seems to be calling
pg_mblen, which depends on database encoding, against UTF-8 encoded
strings. I'll look at those more closely..

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Thu, 23 Aug 2007, Tom Lane wrote:

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

- readstopwords calls recode_and_lowerstr directly, instead of using the
"wordop" function pointer in StopList struct. All callers used
recode_and_lowerstr anyway, so this simplifies the code a little bit. Is
there any external dictionary implementations that would require
different behavior?

I don't think eliminating wordop altogether is such a hot idea; some
dictionary could possibly want to do different processing than that.

Something that was annoying me yesterday was that it was not clear
whether we had fixed every single place that uses a tsearch config file
to assume that the file is in UTF8 and should be converted to database
encoding. So I was thinking of hardwiring the "recode" part into
readstopwords, and using wordop just for the "lowercase" part, which
seemed to me like a saner division of labor. That is, UTF8 is a policy
that we want to enforce globally, but lowercasing maybe not, and this
still leaves the door open for more processing besides lowercasing.

Oleg, Teodor, what do you think about this?

I agrre with utf-8 recoding and please, don't lowercase. Dictionaries
are very different.

regards, tom lane

Regards,
Oleg
_____________________________________________________________
Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
Sternberg Astronomical Institute, Moscow University, Russia
Internet: oleg@sai.msu.su, http://www.sai.msu.su/~megera/
phone: +007(495)939-16-83, +007(495)939-23-83

Tom Lane wrote:

Something that was annoying me yesterday was that it was not clear
whether we had fixed every single place that uses a tsearch config file
to assume that the file is in UTF8 and should be converted to database
encoding. So I was thinking of hardwiring the "recode" part into
readstopwords, and using wordop just for the "lowercase" part, which
seemed to me like a saner division of labor. That is, UTF8 is a policy
that we want to enforce globally, but lowercasing maybe not, and this
still leaves the door open for more processing besides lowercasing.

I think we also want to always run input files through pg_verify_mbstr.
We do it for stopwords, and synonym files (though incorrectly), but not
for thesaurus files or ispell files. It's probably best to do that
within the recode-function as well.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:

Tom Lane wrote:

Something that was annoying me yesterday was that it was not clear
whether we had fixed every single place that uses a tsearch config file
to assume that the file is in UTF8 and should be converted to database
encoding. So I was thinking of hardwiring the "recode" part into
readstopwords, and using wordop just for the "lowercase" part, which
seemed to me like a saner division of labor. That is, UTF8 is a policy
that we want to enforce globally, but lowercasing maybe not, and this
still leaves the door open for more processing besides lowercasing.

I think we also want to always run input files through pg_verify_mbstr.
We do it for stopwords, and synonym files (though incorrectly), but not
for thesaurus files or ispell files. It's probably best to do that
within the recode-function as well.

Ok, here's an updated version of the patch.

- ispell initialization crashed on empty dictionary file
- ispell initialization crashed on affix file with prefixes but no suffixes
- stop words file was ran through pg_verify_mbstr, with database
encoding, but it's later interpreted as being UTF-8. Now verifies that
it's UTF-8, regardless of database encoding.

- introduces new t_readline function that reads a line from a file,
verifies that it's valid UTF-8, and converts it to database encoding.
Modified all places that read tsearch config files to use this function
instead of fgets directly.

- readstopwords now sorts the stop words after loading them. Removed the
separate sortstopwords function.

- moved the wordop-input parameter from StopList struct to a direct
argument to readstopwords. Seems cleaner to me that way, the struct is
now purely an output of readstopwords, not mixed input/output.
readstopwords now recodes the input implicitly using t_readline.

- bunch of comments added, typos fixed, and other cleanup

PS. It's bank holiday here in the UK on Monday, so I won't be around
until Tuesday if something comes up.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

And here's the attachment I forgot.

Heikki Linnakangas wrote:

Heikki Linnakangas wrote:

Tom Lane wrote:

Something that was annoying me yesterday was that it was not clear
whether we had fixed every single place that uses a tsearch config file
to assume that the file is in UTF8 and should be converted to database
encoding. So I was thinking of hardwiring the "recode" part into
readstopwords, and using wordop just for the "lowercase" part, which
seemed to me like a saner division of labor. That is, UTF8 is a policy
that we want to enforce globally, but lowercasing maybe not, and this
still leaves the door open for more processing besides lowercasing.

I think we also want to always run input files through pg_verify_mbstr.
We do it for stopwords, and synonym files (though incorrectly), but not
for thesaurus files or ispell files. It's probably best to do that
within the recode-function as well.

Ok, here's an updated version of the patch.

- ispell initialization crashed on empty dictionary file
- ispell initialization crashed on affix file with prefixes but no suffixes
- stop words file was ran through pg_verify_mbstr, with database
encoding, but it's later interpreted as being UTF-8. Now verifies that
it's UTF-8, regardless of database encoding.

- introduces new t_readline function that reads a line from a file,
verifies that it's valid UTF-8, and converts it to database encoding.
Modified all places that read tsearch config files to use this function
instead of fgets directly.

- readstopwords now sorts the stop words after loading them. Removed the
separate sortstopwords function.

- moved the wordop-input parameter from StopList struct to a direct
argument to readstopwords. Seems cleaner to me that way, the struct is
now purely an output of readstopwords, not mixed input/output.
readstopwords now recodes the input implicitly using t_readline.

- bunch of comments added, typos fixed, and other cleanup

PS. It's bank holiday here in the UK on Monday, so I won't be around
until Tuesday if something comes up.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

Ok, here's an updated version of the patch.

I haven't actually read this patch yet, but the description all sounds
like the Right Thing now. Will review and commit today.

Also, I believe there's consensus to rename the standard Snowball
dictionaries to "english_stem" etc, so I'll make that happen too.

regards, tom lane

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

Ok, here's an updated version of the patch.

Applied, with a few trivial additional cleanups I noticed while reading
the patch. I included your HeadlineText de-duplication too.

regards, tom lane