Re: Tsvector editing functions

Hi,

On 12/07/2015 03:05 PM, Stas Kelvich wrote:
> Hello.
>
> Done with the list of suggestions. Also heavily edit delete function.
>

I did a quick review of the updated patch. I'm not a tsvector-expert so
hopefully my comments won't be entirely bogus.

1) It's a bit difficult to judge the usefulness of the API, as I've
always been a mere user of full-text search, and I never had a need
(or courage) to mess with the tsvectors. OTOH I don't see a good
reason no to have such API, when there's a need for it.

The API seems to be reasonably complete, with one exception - when
looking at editing function we have for 'hstore', we do have these
variants for delete()

delete(hstore,text)
delete(hstore,text[])
delete(hstore,hstore)

while this patch only adds delete(tsvector,text). Would it make
sense to add variants similar to hstore? It probably does not make
much sense to add delete(tsvector,tsvector), right? But being able
to delete a bunch of lexemes in one go seems like a good thing.

What do you think?

2) tsvector_op.c needs a bit of love, to eliminate the two warnings it
currently triggers:

tsvector_op.c:211:2: warning: ISO C90 forbids mixed ...
tsvector_op.c:635:9: warning: variable ‘lexeme2copy’ set but ...

3) the patch also touches tsvector_setweight(), only to do change:

elog(ERROR, "unrecognized weight: %d", cw);

to

elog(ERROR, "unrecognized weight: %c", cw);

That should probably go get committed separately, as a bugfix.

4) I find it rather annoying that there are pretty much no comments in
the code. Granted, there are pretty much no comments in the
surrounding code, but I doubt that's a good reason for not having
any comments in new code. It makes reviews unnecessarily difficult.

5) tsvector_concat() is not mentioned in docs at all

6) Docs don't mention names of the new parameters in function
signatures, just data types. The functions with a single parameter
probably don't need to do that, but multi-parameter ones should.

7) Some of the functions use intexterm that does not match the function
name. I see two such cases - to_tsvector and setweight. Is there a
reason for that?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services