Date: Tue, 22 Nov 2022 06:34:21 +0000
From: sebastian.patino-lang@posteo.net
To: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org
Message-ID: <6702b4c1-457f-4e34-a844-bb9cfe4c7e6a@Spark>
In-Reply-To: <2885495.1668972591@sss.pgh.pa.us>
References: <17691-93cef39a14663963@postgresql.org>
 <2885495.1668972591@sss.pgh.pa.us>
Subject: Re: BUG #17691: Unexpected behaviour using ts_headline()
MIME-Version: 1.0
Content-Type: multipart/alternative; boundary="637c6d72_3d1b58ba_89ae"
Archived-At: 
 <https://www.postgresql.org/message-id/6702b4c1-457f-4e34-a844-bb9cfe4c7e6a%40Spark>
Precedence: bulk

--637c6d72_3d1b58ba_89ae
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

Hi Tom,

thanks for the in-depth explanation. Makes a bit more sense now. Not sure what to do with the attached patch though. Should I use that to compile my own version of PG? Since I use a managed version thats not an option currently.

Regards

Sebastian
On 20. Nov 2022, 20:30 +0100, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > I experience unexpected behaviour when using ts_headline() in general, but
> > especially when changing MaxFragments. Given the data in
> > ts_headline_report.sql [1]
>
> Thanks for sending the sample data. After poking at this a bit, it
> seems the question boils down to what ts_headline should do when there
> simply aren't any short matches to the query. Part of the issue there
> is what qualifies as "short". The definition I installed in 78e73e875
> was
>
> + /*
> + * We might eventually make max_cover a user-settable parameter, but for
> + * now, just compute a reasonable value based on max_words and
> + * max_fragments.
> + */
> + max_cover = Max(max_words * 10, 100);
> + if (max_fragments > 0)
> + max_cover *= max_fragments;
>
> Looking at this again, I think the dependency on max_fragments is just
> wrong. That is what is causing your different results for different
> settings of MaxFragments: larger values allow recognition of longer
> candidate covers ("cover" being jargon in this code for "a substring
> satisfying the query"). But there's no good reason for that. I'd misread
> the logic in mark_hl_fragments, which "breaks the cover into smaller
> fragments such that each fragment has at most max_words", to think that
> it needed to see longer covers that it could divide into fragments.
> But actually max_fragments limits the number of fragments it will return
> across all covers, and there doesn't seem to be a reason to connect that
> parameter to the acceptable length of any particular cover. So dropping
> those two lines is enough to make the strange behavior across differing
> MaxFragments go away.
>
> The other issue is what to do after finding there's no match of max_cover
> or fewer tokens. As the code now stands, it'll just give up and return
> the initial bit of the text, likely with no query words. That's not
> great. I propose that we look for any match and return (highlight) just
> its first word.
>
> > SELECT id,
> > ts_headline('english', "texts"."fulltext", to_tsquery('english', 'amazon &
> > world'), 'StartSel=<<, StopSel=>>') AS "preview"
> > FROM texts;
>
> > id=1: Highlight word is the first one in the result. Expectation: highlight
> > word is somewhere in the middle.
>
> Meh --- I don't agree with that expectation. The fragment-based code does
> act that way, but the non-fragment path never has, so I think we'd make
> more people unhappy than happy if we change its behavior.
>
> > id=2: No highlight word at all.
>
> Right, because there are no short matches to "amazon & world" anywhere.
> With the attached proposed patch, we'll highlight whichever one of those
> words appears first.
>
> > id=3: Highlight words are the first and last one in the result. Not ideal
> > but ok-ish.
>
> As I said, that's how non-fragment highlighting has always worked. It
> will extend the quoted text, but not beyond min_words, which is a bit
> different from fragment highlighting.
>
> > SELECT id,
> > ts_headline('english', "texts"."fulltext", to_tsquery('english', 'amazon &
> > world'), 'MaxFragments=3, StartSel=<<, StopSel=>>') AS "preview"
> > FROM texts;
>
> > id=1: Wrong number of fragments (2) with highlight words are returned.
>
> I don't agree with this expectation either. There's only one reasonably
> sane match to 'amazon & world' in that text, and it's near the beginning.
> As the code stands, increasing max_fragments eventually allows it to find
> some less-sane matches, but that doesn't seem like an improvement.
>
>
> In short, I suggest something about like the attached. For your id=2
> example, which has only very far-apart occurrences of 'amazon' and
> 'world', this produces
>
> <<world>> where fast runtimes make or break the popularity of research
> fields, this oversight has effectively
>
> without fragmenting, and
>
> researchers make the effort of understanding the inner workings of
> these convenient black-boxes. In a <<world>> where fast runtimes make
> or break the popularity of research fields, this oversight has
> effectively surrendered most
>
> with fragmenting, which is okay by me given their historically different
> behaviors. Otherwise, it gets rid of the strange changes in matching
> behavior for different MaxFragment values.
>
> regards, tom lane
>

--637c6d72_3d1b58ba_89ae
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

<html xmlns=3D=22http://www.w3.org/1999/xhtml=22>
<head>
<title></title>
</head>
<body>
<div name=3D=22messageBodySection=22>
<div dir=3D=22auto=22>Hi Tom,<br />
<br />
thanks for the in-depth explanation. Makes a bit more sense now. Not sure=
 what to do with the attached patch though. Should I use that to compile =
my own version of PG=3F Since I use a managed version thats not an option=
 currently.<br />
<br />
Regards</div>
</div>
<div name=3D=22messageSignatureSection=22><br />
<div class=3D=22match=46ont=22>Sebastian</div>
</div>
<div name=3D=22messageReplySection=22>On 20. Nov 2022, 20:30 +0100, Tom L=
ane &lt;tgl=40sss.pgh.pa.us&gt;, wrote:<br />
<blockquote type=3D=22cite=22 style=3D=22border-left-color: grey; border-=
left-width: thin; border-left-style: solid; margin: 5px 5px;padding-left:=
 10px;=22>PG Bug reporting form &lt;noreply=40postgresql.org&gt; writes:<=
br />
<blockquote type=3D=22cite=22>I experience unexpected behaviour when usin=
g ts=5Fheadline() in general, but<br />
especially when changing Max=46ragments. Given the data in<br />
ts=5Fheadline=5Freport.sql =5B1=5D<br /></blockquote>
<br />
Thanks for sending the sample data. After poking at this a bit, it<br />
seems the question boils down to what ts=5Fheadline should do when there<=
br />
simply aren't any short matches to the query. Part of the issue there<br =
/>
is what qualifies as =22short=22. The definition I installed in 78e73e875=
<br />
was<br />
<br />
+ /*<br />
+ * We might eventually make max=5Fcover a user-settable parameter, but f=
or<br />
+ * now, just compute a reasonable value based on max=5Fwords and<br />
+ * max=5Ffragments.<br />
+ */<br />
+ max=5Fcover =3D Max(max=5Fwords * 10, 100);<br />
+ if (max=5Ffragments &gt; 0)<br />
+ max=5Fcover *=3D max=5Ffragments;<br />
<br />
Looking at this again, I think the dependency on max=5Ffragments is just<=
br />
wrong. That is what is causing your different results for different<br />=

settings of Max=46ragments: larger values allow recognition of longer<br =
/>
candidate covers (=22cover=22 being jargon in this code for =22a substrin=
g<br />
satisfying the query=22). But there's no good reason for that. I'd misrea=
d<br />
the logic in mark=5Fhl=5Ffragments, which =22breaks the cover into smalle=
r<br />
fragments such that each fragment has at most max=5Fwords=22, to think th=
at<br />
it needed to see longer covers that it could divide into fragments.<br />=

But actually max=5Ffragments limits the number of fragments it will retur=
n<br />
across all covers, and there doesn't seem to be a reason to connect that<=
br />
parameter to the acceptable length of any particular cover. So dropping<b=
r />
those two lines is enough to make the strange behavior across differing<b=
r />
Max=46ragments go away.<br />
<br />
The other issue is what to do after finding there's no match of max=5Fcov=
er<br />
or fewer tokens. As the code now stands, it'll just give up and return<br=
 />
the initial bit of the text, likely with no query words. That's not<br />=

great. I propose that we look for any match and return (highlight) just<b=
r />
its first word.<br />
<br />
<blockquote type=3D=22cite=22>SELECT id,<br />
ts=5Fheadline('english', =22texts=22.=22fulltext=22, to=5Ftsquery('englis=
h', 'amazon &amp;<br />
world'), 'StartSel=3D&lt;&lt;, StopSel=3D&gt;&gt;') AS =22preview=22<br /=
>
=46ROM texts;<br /></blockquote>
<br />
<blockquote type=3D=22cite=22>id=3D1: Highlight word is the first one in =
the result. Expectation: highlight<br />
word is somewhere in the middle.<br /></blockquote>
<br />
Meh --- I don't agree with that expectation. The fragment-based code does=
<br />
act that way, but the non-fragment path never has, so I think we'd make<b=
r />
more people unhappy than happy if we change its behavior.<br />
<br />
<blockquote type=3D=22cite=22>id=3D2: No highlight word at all.<br /></bl=
ockquote>
<br />
Right, because there are no short matches to =22amazon &amp; world=22 any=
where.<br />
With the attached proposed patch, we'll highlight whichever one of those<=
br />
words appears first.<br />
<br />
<blockquote type=3D=22cite=22>id=3D3: Highlight words are the first and l=
ast one in the result. Not ideal<br />
but ok-ish.<br /></blockquote>
<br />
As I said, that's how non-fragment highlighting has always worked. It<br =
/>
will extend the quoted text, but not beyond min=5Fwords, which is a bit<b=
r />
different from fragment highlighting.<br />
<br />
<blockquote type=3D=22cite=22>SELECT id,<br />
ts=5Fheadline('english', =22texts=22.=22fulltext=22, to=5Ftsquery('englis=
h', 'amazon &amp;<br />
world'), 'Max=46ragments=3D3, StartSel=3D&lt;&lt;, StopSel=3D&gt;&gt;') A=
S =22preview=22<br />
=46ROM texts;<br /></blockquote>
<br />
<blockquote type=3D=22cite=22>id=3D1: Wrong number of fragments (2) with =
highlight words are returned.<br /></blockquote>
<br />
I don't agree with this expectation either. There's only one reasonably<b=
r />
sane match to 'amazon &amp; world' in that text, and it's near the beginn=
ing.<br />
As the code stands, increasing max=5Ffragments eventually allows it to fi=
nd<br />
some less-sane matches, but that doesn't seem like an improvement.<br />
<br />
<br />
In short, I suggest something about like the attached. =46or your id=3D2<=
br />
example, which has only very far-apart occurrences of 'amazon' and<br />
'world', this produces<br />
<br />
&lt;&lt;world&gt;&gt; where fast runtimes make or break the popularity of=
 research<br />
fields, this oversight has effectively<br />
<br />
without fragmenting, and<br />
<br />
researchers make the effort of understanding the inner workings of<br />
these convenient black-boxes. In a &lt;&lt;world&gt;&gt; where fast runti=
mes make<br />
or break the popularity of research fields, this oversight has<br />
effectively surrendered most<br />
<br />
with fragmenting, which is okay by me given their historically different<=
br />
behaviors. Otherwise, it gets rid of the strange changes in matching<br /=
>
behavior for different Max=46ragment values.<br />
<br />
regards, tom lane<br />
<br /></blockquote>
</div>
</body>
</html>

--637c6d72_3d1b58ba_89ae--