Re: Prefix operator for text and spgist support

From: Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>
To: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Prefix operator for text and spgist support
Date: 2018-02-19 14:19:15
Message-ID: 20180219171915.65821d58@wp.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, 19 Feb 2018 15:06:51 +0300
Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote:

> Hello,
>
> On Fri, Feb 02, 2018 at 06:03:27PM +0300, Ildus Kurbangaliev wrote:
> > Hi,
> >
> > Attached patch introduces prefix operator ^@ for text type. For 'a
> > ^@ b' it returns true if 'a' starts with 'b'. Also there is spgist
> > index support for this operator.
> >
> > It could be useful as an alternative for LIKE for 'something%'
> > templates. Or even used in LIKE queries instead of ~>=~ and ~<~ in
> > the future. But it would require new strategy for btree.
>
> I've looked at the patch. It is applied and tests pass.

Hi, thanks for the review.

>
> I have a couple comments:
>
> > + if (ptype == Pattern_Type_Prefix)
> > + {
> > + char *s = TextDatumGetCString(constval);
> > + prefix = string_to_const(s, vartype);
> > + pstatus = Pattern_Prefix_Partial;
> > + rest_selec = 1.0; /* all */
> > + pfree(s);
> > + }
> > + else
> > + pstatus = pattern_fixed_prefix(patt, ptype,
> > collation,
> > +
> > &prefix, &rest_selec);
>
> I think it is better to put Pattern_Type_Prefix processing into
> pattern_fixed_prefix() as another case entry.

At brief look at this place seems better to move this block into
pattern_fixed_prefix function. But there is also `vartype` variable
which used to in prefix construction, and it would require pass this
variable too. And since pattern_fixed_prefix called in 10 other places
and vartype is required only for this ptype it seems better just keep
this block outside of this function.

>
> Secondly, it is worth to fix the documentation. At least here [1].
> Maybe there are another places where documentation should be fixed.
>
>
> 1 -
> https://www.postgresql.org/docs/current/static/spgist-builtin-opclasses.html
>

I've added documentation in current version of the patch.

--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Attachment Content-Type Size
prefix_operator_and_spgist_v2.patch text/x-patch 15.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Anastasia Lubennikova 2018-02-19 14:36:55 Re: CURRENT OF causes an error when IndexOnlyScan is used
Previous Message Joe Conway 2018-02-19 14:11:05 Re: SHA-2 functions