Subject: Re: [HACKERS] new function for tsquery creartion
To: Victor Drobny <v.drobny@postgrespro.ru>,
 Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
References: <fe931111ff7e9ad79196486ada79e268@postgrespro.ru>
 <CA+TgmoYSBLFAq14ePMWC=JBQ4D4Crt08emkcDPa65SQ6sbVAoQ@mail.gmail.com>
 <CAEepm=2S-rXC-G3iGSJLKwThF2v9zWG0pJB4R6v5LVVphwQkRw@mail.gmail.com>
 <69234700d26674fc7c171e48b850824c@postgrespro.ru>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <bff75704-a832-e240-d216-5ba6a8db5d13@2ndquadrant.com>
Date: Sun, 19 Nov 2017 02:30:57 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <69234700d26674fc7c171e48b850824c@postgrespro.ru>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 09/13/2017 10:57 AM, Victor Drobny wrote:
> On 2017-09-09 06:03, Thomas Munro wrote:
>> Please send a rebased version of the patch for people to review and
>> test as that one has bit-rotted.
> Hello,
> Thank you for interest. In the attachment you can find rebased
> version(based on 69835bc8988812c960f4ed5aeee86b62ac73602a commit)
> 

I did a quick review of the patch today. The patch unfortunately no
longer applies, so I had to use an older commit from September. Please
rebase to current master.

I've only looked on the diff at this point, will do more testing once
the rebase happens.

Some comments:

1) This seems to mix multiple improvements into one patch. There's the
support for alternative query syntax, and then there are the new
operators (AROUND and <m,n>). I propose to split the patch into two or
more parts, each addressing one of those bits.

I guess there will be two or three parts - first adding the syntax,
second adding <m,n> and third adding the AROUND(n). Seems reasonable?

2) I don't think we should mention Google in the docs explicitly. Not
that I'm somehow anti-google, but this syntax was certainly not invented
by Google - I vividly remember using something like that on Altavista
(yeah, I'm old). And it's used by pretty much every other web search
engine out there ...

3) In the SGML docs, please use <literal></literal> instead of just
quoting the values. So it should be <literal>|</literal> instead of '|'
etc. Just like in the parts describing plainto_tsquery, for example.

4) Also, I recommend adding a brief explanation what the examples do.
Right now there's just a bunch of queryto_tsquery, and the reader is
expected to understand the output. I suggest adding a sentence or two,
explaining what's happening (just like for plainto_tsquery examples).

5) I'm not sure about negative values in the <n,m> operator. I don't
find it particularly confusing - once you understand that (a <n,m> b)
means "there are 'k' words between 'a' and 'b' (n <= k <= m)", then
negative values seem like a fairly straightforward extension.

But I guess the main question is "Is there really a demand for the new
<n,m> operator, or have we just invented if because we can?"

6) There seem to be some new constants defined, with names not following
the naming convention. I mean this

    #define WAITOPERAND			1
    #define WAITOPERATOR		2
    #define WAITFIRSTOPERAND	3
    #define WAITSINGLEOPERAND	4
    #define INSIDEQUOTES		5   <-- the new one

and

    #define TSPO_L_ONLY            0x01
    #define TSPO_R_ONLY            0x02
    #define TSPO_BOTH              0x04
    #define TS_NOT_EXAC            0x08 <-- the new one

Perhaps that's OK, but it seems a bit inconsistent.


regards

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