Re: new function for tsquery creartion

From: Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Victor Drobny <v(dot)drobny(at)postgrespro(dot)ru>
Subject: Re: new function for tsquery creartion
Date: 2017-11-28 14:39:16
Message-ID: 20171128143916.1377.5847.pgcf@coridan.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi Victor,

I like the idea and I think it's a great patch. However in current shape it
requires some amount of reworking to meet PostgreSQL standards of code quality.

Particularly:

1. Many new procedures don't have a comment with a brief description. Ideally
every procedure should have not only a brief description but also a description
of every argument, return value and changes of global state if applicable.

2. I believe you could implement the has_prefix procedure just as a wrapper of
strstr().

3. I suggest to use snprintf instead of sprintf in a new code whenever
possible, especially if you are using %s - just to be on a safe side.

4. I noticed that your code affects the catalog. Did you check that your
changes will not cause problems during the migration from the older version of
PostgreSQL to the never one?

5. Tests for queryto_tsquery use only ASCII strings. I suggest to add a few
test that use non-ASCII characters as well, and a few corner cases like empty
string, string that contains only the stop-words, etc.

6. `make check-world` doesn't pass:

```
***************
*** 1672,1678 ****
(1 row)

set enable_seqscan = on;
-
--test queryto_tsquery function
select queryto_tsquery('My brand new smartphone');
queryto_tsquery
--- 1672,1677 ----
***************
*** 1784,1786 ****
--- 1783,1786 ----
---------------------------
'fat-rat' & 'fat' & 'rat'
(1 row)
+
```

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2017-11-28 14:45:09 Re: ERROR: too many dynamic shared memory segments
Previous Message Robert Haas 2017-11-28 13:43:45 Re: ERROR: too many dynamic shared memory segments