[PATCH] Phrase search ported to 9.6

On Mon, Feb 1, 2016 at 8:21 PM, Dmitry Ivanov <d.ivanov@postgrespro.ru>
wrote:

This patch was originally developed by Teodor Sigaev and Oleg Bartunov in
2009, so all credit goes to them. Any feedback is welcome.

This is not a small patch:
28 files changed, 2441 insertions(+), 380 deletions(-)
And the last CF of 9.6 should not contain rather large patches.
--
Michael

På tirsdag 02. februar 2016 kl. 04:22:57, skrev Michael Paquier <
michael.paquier@gmail.com <mailto:michael.paquier@gmail.com>>:
    On Mon, Feb 1, 2016 at 8:21 PM, Dmitry Ivanov <d.ivanov@postgrespro.ru
<mailto:d.ivanov@postgrespro.ru>> wrote: This patch was originally developed by
Teodor Sigaev and Oleg Bartunov in
2009, so all credit goes to them. Any feedback is welcome.

This is not a small patch:
28 files changed, 2441 insertions(+), 380 deletions(-)
And the last CF of 9.6 should not contain rather large patches.
-- Michael

 
 
OTOH; It would be extremely nice to get this into 9.6.
 
 
-- Andreas Joseph Krogh

On Tue, Feb 2, 2016 at 10:18 AM, Andreas Joseph Krogh <andreas@visena.com>
wrote:

På tirsdag 02. februar 2016 kl. 04:22:57, skrev Michael Paquier <
michael.paquier@gmail.com>:

On Mon, Feb 1, 2016 at 8:21 PM, Dmitry Ivanov <d.ivanov@postgrespro.ru>
wrote:

This patch was originally developed by Teodor Sigaev and Oleg Bartunov in
2009, so all credit goes to them. Any feedback is welcome.

This is not a small patch:
28 files changed, 2441 insertions(+), 380 deletions(-)
And the last CF of 9.6 should not contain rather large patches.
--
Michael

OTOH; It would be extremely nice to get this into 9.6.

will see how community decided.
anyway, it's already in our distribution.

På tirsdag 02. februar 2016 kl. 09:20:06, skrev Oleg Bartunov <
obartunov@gmail.com <mailto:obartunov@gmail.com>>:
    On Tue, Feb 2, 2016 at 10:18 AM, Andreas Joseph Krogh <andreas@visena.com
<mailto:andreas@visena.com>> wrote: På tirsdag 02. februar 2016 kl. 04:22:57,
skrev Michael Paquier <michael.paquier@gmail.com
<mailto:michael.paquier@gmail.com>>:
    On Mon, Feb 1, 2016 at 8:21 PM, Dmitry Ivanov <d.ivanov@postgrespro.ru
<mailto:d.ivanov@postgrespro.ru>> wrote: This patch was originally developed by
Teodor Sigaev and Oleg Bartunov in
2009, so all credit goes to them. Any feedback is welcome.

This is not a small patch:
28 files changed, 2441 insertions(+), 380 deletions(-)
And the last CF of 9.6 should not contain rather large patches.
-- Michael

OTOH; It would be extremely nice to get this into 9.6.
 
will see how community decided.
anyway, it's already in our distribution.

 
 
Which seems to indicate it has received a fair amount of testing and is quite
stable.
Hopefully it integrates into the 9.6 codebase without too much risk.
Thanks for contributing this.
 
-- Andreas Joseph Krogh

Andreas Joseph Krogh wrote:
�

Which seems to indicate it has received a fair amount of testing and is quite
stable.
Hopefully it integrates into the 9.6 codebase without too much risk.

Yes, yes, that's all very good, but we're nearing the closure of the 9.6
development cycle and we only have one commitfest left. If someone had
lots of community brownie points because of doing lots of reviews of
other people's patches, they might push their luck by posting this patch
to the final commitfest. But if that someone didn't, then it wouldn't
be fair, and if I were the commitfest manager of that commitfest I would
boot their patch to the 9.7-First commitfest.

The current commitfest which I'm trying to close still has 24 patches in
needs-review state and 11 patches ready-for-committer; the next one (not
closed yet) has 40 patches that will need review. That means a total of
75 patches, and those should all be processed ahead of this one. The
effort needed to process each of those patches is not trivial, and I'm
sorry I have to say this but I don't see PostgresPro contributing enough
reviews, even though I pinged a number of people there, so putting one
more patch on the rest of the community's shoulders doesn't seem fair to
me.

Everybody has their favorite patch that they want in the next release,
but we only have so much manpower to review and integrate those patches.
All review help is welcome.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

På tirsdag 02. februar 2016 kl. 12:04:21, skrev Alvaro Herrera <
alvherre@2ndquadrant.com <mailto:alvherre@2ndquadrant.com>>:
Andreas Joseph Krogh wrote:
  

Which seems to indicate it has received a fair amount of testing and is

quite

stable.
Hopefully it integrates into the 9.6 codebase without too much risk.

Yes, yes, that's all very good, but we're nearing the closure of the 9.6
development cycle and we only have one commitfest left.  If someone had
lots of community brownie points because of doing lots of reviews of
other people's patches, they might push their luck by posting this patch
to the final commitfest.  But if that someone didn't, then it wouldn't
be fair, and if I were the commitfest manager of that commitfest I would
boot their patch to the 9.7-First commitfest.

The current commitfest which I'm trying to close still has 24 patches in
needs-review state and 11 patches ready-for-committer; the next one (not
closed yet) has 40 patches that will need review.  That means a total of
75 patches, and those should all be processed ahead of this one.  The
effort needed to process each of those patches is not trivial, and I'm
sorry I have to say this but I don't see PostgresPro contributing enough
reviews, even though I pinged a number of people there, so putting one
more patch on the rest of the community's shoulders doesn't seem fair to
me.

Everybody has their favorite patch that they want in the next release,
but we only have so much manpower to review and integrate those patches.
All review help is welcome.
 
I understand completely.
 
-- Andreas Joseph Krogh

On Tue, Feb 2, 2016 at 2:04 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Andreas Joseph Krogh wrote:

Which seems to indicate it has received a fair amount of testing and is

quite

stable.
Hopefully it integrates into the 9.6 codebase without too much risk.

Yes, yes, that's all very good, but we're nearing the closure of the 9.6
development cycle and we only have one commitfest left. If someone had
lots of community brownie points because of doing lots of reviews of
other people's patches, they might push their luck by posting this patch
to the final commitfest. But if that someone didn't, then it wouldn't
be fair, and if I were the commitfest manager of that commitfest I would
boot their patch to the 9.7-First commitfest.

The current commitfest which I'm trying to close still has 24 patches in
needs-review state and 11 patches ready-for-committer; the next one (not
closed yet) has 40 patches that will need review. That means a total of
75 patches, and those should all be processed ahead of this one. The
effort needed to process each of those patches is not trivial, and I'm
sorry I have to say this but I don't see PostgresPro contributing enough
reviews, even though I pinged a number of people there, so putting one
more patch on the rest of the community's shoulders doesn't seem fair to
me.

I'll talk about this.

On Tue, Feb 2, 2016 at 6:04 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Andreas Joseph Krogh wrote:

Which seems to indicate it has received a fair amount of testing and is quite
stable.
Hopefully it integrates into the 9.6 codebase without too much risk.

Yes, yes, that's all very good, but we're nearing the closure of the 9.6
development cycle and we only have one commitfest left. If someone had
lots of community brownie points because of doing lots of reviews of
other people's patches, they might push their luck by posting this patch
to the final commitfest. But if that someone didn't, then it wouldn't
be fair, and if I were the commitfest manager of that commitfest I would
boot their patch to the 9.7-First commitfest.

Completely agreed. Also, to be frank, these text search patches are
often in need of quite a LOT of work per line compared to some others.
For example, consider the percentage of this patch that is comments.
It's a pretty low percentage. And it's going into surrounding code
that is also very low in comments. Giving this a good review will
take somebody a lot of time, and bringing it up to PostgreSQL's normal
standards will probably take quite a lot of work. I don't understand
why the community should agree to do that for anyone, and as you say,
it's not like PostgresPro is leading the pack in terms of review
contributions. We're not going to get this release out on time if
everybody insists that every patch has to go in.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello, Dmitry

This is my initial review for you patch. Below are my comments.

Introduction
------------

This patch introduce new operator and new functions.
New operator:
- ??
New functions:
- phraseto_tsquery([ config regconfig , ] query text)
- setweight(tsquery, "char")
- tsquery_phrase(query1 tsquery, query2 tsquery)
- tsquery_phrase(query1 tsquery, query2 tsquery, distance integer)

New regression tests are included in the patch. Information about new
features is provided in the documentation.

Initial Run
-----------

The patch applies correctly to HEAD. Regression tests pass successfully,
without errors. It seems that the patch work as you expected.

Performance
-----------

I have not tested possible performance issues yet. Maybe later I will
prepare some data to test performance.

Coding
------

I agree with others that there is a lack of comments for new functions.
Most of them without comments.

../pg_patches/phrase_search.patch:1086: trailing whitespace.
* QI_VALSTOP nodes should be cleaned and
../pg_patches/phrase_search.patch:1124: trailing whitespace.
if (priority < parentPriority)
../pg_patches/phrase_search.patch:1140: trailing whitespace.
if (priority < parentPriority)
../pg_patches/phrase_search.patch:1591: space before tab in indent.
/* (a|b) ? c => (a?c) | (b?c) */
../pg_patches/phrase_search.patch:1603: space before tab in indent.
/* !a ? b => b & !(a?b) */

It is git apply output. There are trailing whitespaces in the code.

+typedef struct MorphOpaque
+{
+	Oid		cfg_id;
+	int		qoperator;	/* query operator */
+} MorphOpaque;

Maybe you should move structure definition to the top of the to_tsany.c

+					pushValue(state,
+							  prs.words[count].word,
+							  prs.words[count].len,
+							  weight,
+							  ((prs.words[count].flags & TSL_PREFIX) || prefix) ?
+									true :
+									false);

There is not need in ternary operator here. You can write:

pushValue(state,
prs.words[count].word,
prs.words[count].len,
weight,
(prs.words[count].flags & TSL_PREFIX) || prefix);

/*
+ * Wrapper of check condition function for TS_execute.
+ * We are using the fact GIN_FALSE = 0 and GIN_MAYBE state
+ * should be treated as true, so, any non-zero value should be
+ * converted to boolean true.
+ */
+static bool
+checkcondition_gin(void *checkval, QueryOperand *val, ExecPhraseData *data)
+{
+	return !!checkcondition_gin_internal((GinChkVal *) checkval, val, data);
+}

Maybe write like this?

static bool
checkcondition_gin(void *checkval, QueryOperand *val, ExecPhraseData *data)
{
return checkcondition_gin_internal((GinChkVal *) checkval, val, data)
!= GIN_FALSE;
}

Then you don't need in the comment above of the function.

+#define PRINT_PRIORITY(x) ( (((QueryOperator*)(x))->oper == OP_NOT) ? 5 : QO_PRIORITY(x) )

What is mean "5" here? You can define a new value near the:

#define OP_OR 1
#define OP_AND 2
#define OP_NOT 3
#define OP_PHRASE 4

+Datum
+tsquery_setweight(PG_FUNCTION_ARGS)
+{
+	TSQuery    	in = PG_GETARG_TSQUERY(0);
+	char        cw = PG_GETARG_CHAR(1);
+	TSQuery 	out;
+	QueryItem  *item;
+	int         w = 0;
+
+	switch (cw)
+	{
+		case 'A':
+		case 'a':
+			w = 3;
+			break;
+		case 'B':
+		case 'b':
+			w = 2;
+			break;
+		case 'C':
+		case 'c':
+			w = 1;
+			break;
+		case 'D':
+		case 'd':
+			w = 0;
+			break;
+		default:
+			/* internal error */
+			elog(ERROR, "unrecognized weight: %d", cw);
+	}
+
+	out = (TSQuery) palloc(VARSIZE(in));
+	memcpy(out, in, VARSIZE(in));
+	item = GETQUERY(out);
+
+	while(item - GETQUERY(out) < out->size)	
+	{
+		if (item->type == QI_VAL)
+			item->qoperand.weight |= (1 << w);
+
+		item++;
+	}
+
+	PG_FREE_IF_COPY(in, 0);
+	PG_RETURN_POINTER(out);
+}

This function has the duplicated piece from the tsvector_setweight()
from tsvector_op.c. You can define new function for it.

+/*
+ * Check if datatype is the specified type or equivalent to it.
+ *
+ * Note: we could just do getBaseType() unconditionally, but since that's
+ * a relatively expensive catalog lookup that most users won't need, we
+ * try the straight comparison first.
+ */
+static bool
+is_expected_type(Oid typid, Oid expected_type)
+{
+	if (typid == expected_type)
+		return true;
+	typid = getBaseType(typid);
+	if (typid == expected_type)
+		return true;
+	return false;
+}
+
+/* Check if datatype is TEXT or binary-equivalent to it */
+static bool
+is_text_type(Oid typid)
+{
+	/* varchar(n) and char(n) are binary-compatible with text */
+	if (typid == TEXTOID || typid == VARCHAROID || typid == BPCHAROID)
+		return true;
+	/* Allow domains over these types, too */
+	typid = getBaseType(typid);
+	if (typid == TEXTOID || typid == VARCHAROID || typid == BPCHAROID)
+		return true;
+	return false;
+}

These functions was removed in 9acb9007de30b3daaa9efc16763c3bc6e3e0a92d.
It seems that tsvector_op.c was not synchronized with the master.

Conclusion
----------

This patch is large and it needs more research. I will be reviewing it
and will give another notes later.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/29/16 10:33 AM, Artur Zakirov wrote:

Conclusion
----------

This patch is large and it needs more research. I will be reviewing it
and will give another notes later.

This thread has not had a response from the authors or new patch in more
than two weeks. Please post something soon or it will be marked
"returned with feedback".

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi, Artur

I've made an attempt to fix some of the issues you've listed, although there's
still much work to be done. I'll add some comments later.

This function has the duplicated piece from the tsvector_setweight()
from tsvector_op.c. You can define new function for it.

I'm not sure it's worth the trouble. IMO these functions are relatively small
and we won't benefit from extracting the duplicate code.

These functions was removed in 9acb9007de30b3daaa9efc16763c3bc6e3e0a92d.
It seems that tsvector_op.c was not synchronized with the master.

Got it, thanks!

--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

I tried to find some bugs in the code. I can't find them. But it does
not mean that there are not bugs.

Still there are a lack of comments and trailing whitespaces.

On 16.03.2016 19:38, Dmitry Ivanov wrote:

Hi, Artur

I've made an attempt to fix some of the issues you've listed, although there's
still much work to be done. I'll add some comments later.

This function has the duplicated piece from the tsvector_setweight()
from tsvector_op.c. You can define new function for it.

I'm not sure it's worth the trouble. IMO these functions are relatively small
and we won't benefit from extracting the duplicate code.

I think that a separate function would be better. These weights are
occurred in other functions. But I might be wrong and maybe I cavil at
the code too much.

These functions was removed in 9acb9007de30b3daaa9efc16763c3bc6e3e0a92d.
It seems that tsvector_op.c was not synchronized with the master.

Got it, thanks!

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi!

I see that patch changes existing regression tests in tsearch2.out.

*** a/contrib/tsearch2/expected/tsearch2.out
--- b/contrib/tsearch2/expected/tsearch2.out
*************** SELECT '(!1|2)&3'::tsquery;
*** 278,292 ****
  (1 row)

SELECT '1|(2|(4|(5|6)))'::tsquery;
! tsquery
! -----------------------------------------
! '1' | ( '2' | ( '4' | ( '5' | '6' ) ) )
(1 row)

SELECT '1|2|4|5|6'::tsquery;
! tsquery
! -----------------------------------------
! ( ( ( '1' | '2' ) | '4' ) | '5' ) | '6'
(1 row)

  SELECT '1&(2&(4&(5&6)))'::tsquery;
--- 278,292 ----
  (1 row)

SELECT '1|(2|(4|(5|6)))'::tsquery;
! tsquery
! -----------------------------
! '1' | '2' | '4' | '5' | '6'
(1 row)

SELECT '1|2|4|5|6'::tsquery;
! tsquery
! -----------------------------
! '1' | '2' | '4' | '5' | '6'
(1 row)

This change looks like improvement, without braces tsquery readability is
much better.

*************** select rewrite('moscow & hotel', 'select
*** 461,469 ****
(1 row)

select rewrite('bar & new & qq & foo & york', 'select keyword, sample
from test_tsquery'::text );
! rewrite
!
-------------------------------------------------------------------------------------
! 'citi' & 'foo' & ( 'bar' | 'qq' ) & ( 'nyc' | ( 'big' & 'appl' | 'new' &
'york' ) )
(1 row)

  select rewrite( ARRAY['moscow', keyword, sample] ) from test_tsquery;
--- 461,469 ----
  (1 row)

select rewrite('bar & new & qq & foo & york', 'select keyword, sample
from test_tsquery'::text );
! rewrite
!
---------------------------------------------------------------------------------
! ( 'nyc' | 'big' & 'appl' | 'new' & 'york' ) & 'citi' & 'foo' & ( 'bar' |
'qq' )
(1 row)

select rewrite( ARRAY['moscow', keyword, sample] ) from test_tsquery;

However, such reorderings look unclear and need motivation.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi Dmitry,

On 3/16/16 12:38 PM, Dmitry Ivanov wrote:

I've made an attempt to fix some of the issues you've listed, although there's
still much work to be done. I'll add some comments later.

Do you know when you'll have a chance to respond to reviews and provide
a new patch?

Time is short and it's not encouraging that you say there is "still much
work to be done". Perhaps it would be best to mark this "returned with
feedback"?

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Mar 25, 2016 at 6:42 PM, David Steele <david@pgmasters.net> wrote:

On 3/16/16 12:38 PM, Dmitry Ivanov wrote:

I've made an attempt to fix some of the issues you've listed, although

there's
still much work to be done. I'll add some comments later.

Do you know when you'll have a chance to respond to reviews and provide a
new patch?

Time is short and it's not encouraging that you say there is "still much
work to be done". Perhaps it would be best to mark this "returned with
feedback"?

Dmitry will provide a new version of patch today.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Sorry for the delay, I desperately needed some time to finish a bunch of
dangling tasks.

I've added some new comments and clarified the ones that were obscure.
Moreover, I felt an urge to recheck most parts of the code since apparently
nobody (besides myself) has gone so far yet.

On 25.03.16 18:42 MSK, David Steele wrote:

Time is short and it's not encouraging that you say there is "still much

work to be done".

Indeed, I was inaccurate. I am more than interested in the positive outcome.

--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On 3/25/16 2:42 PM, Dmitry Ivanov wrote:

Sorry for the delay, I desperately needed some time to finish a bunch of
dangling tasks.

I've added some new comments and clarified the ones that were obscure.
Moreover, I felt an urge to recheck most parts of the code since apparently
nobody (besides myself) has gone so far yet.

On 25.03.16 18:42 MSK, David Steele wrote:

Time is short and it's not encouraging that you say there is "still much

work to be done".

Indeed, I was inaccurate. I am more than interested in the positive outcome.

Me, too! I have set this to "needs review".

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 25.03.2016 21:42, Dmitry Ivanov wrote:

Sorry for the delay, I desperately needed some time to finish a bunch of
dangling tasks.

I've added some new comments and clarified the ones that were obscure.
Moreover, I felt an urge to recheck most parts of the code since apparently
nobody (besides myself) has gone so far yet.

On 25.03.16 18:42 MSK, David Steele wrote:

Time is short and it's not encouraging that you say there is "still much

work to be done".

Indeed, I was inaccurate. I am more than interested in the positive outcome.

Hi,

Thank you for your work!

I tested the patch and take a look on it. All regression tests passed.
The code looks good and the patch introduce a great functionality.

I think the patch can be marked as "Ready for Commiter". But I do not
feel the force to do that myself.

Also I agree with you about tsvector_setweight(). There is not a problem
with it because this weights are immutable and so there is not benefits
from new function.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/25/16 3:54 PM, Artur Zakirov wrote:

On 25.03.2016 21:42, Dmitry Ivanov wrote:

Sorry for the delay, I desperately needed some time to finish a bunch of
dangling tasks.

I've added some new comments and clarified the ones that were obscure.
Moreover, I felt an urge to recheck most parts of the code since
apparently
nobody (besides myself) has gone so far yet.

On 25.03.16 18:42 MSK, David Steele wrote:

Time is short and it's not encouraging that you say there is "still much

work to be done".

Indeed, I was inaccurate. I am more than interested in the positive
outcome.

I tested the patch and take a look on it. All regression tests passed.
The code looks good and the patch introduce a great functionality.

I think the patch can be marked as "Ready for Commiter". But I do not
feel the force to do that myself.

Also I agree with you about tsvector_setweight(). There is not a problem
with it because this weights are immutable and so there is not benefits
from new function.

Ideally Alexander can also look at it. If not, then you should mark it
"ready for committer" since I doubt any more reviewers will sign on this
late in the CF.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers