Re: btree_gist (was: CommitFest progress - or lack thereof)

On Fri, Feb 4, 2011 at 12:02 PM, Oleg Bartunov <oleg(at)sai(dot)msu(dot)su> wrote:
> Aha,
>
> Teodor sent it to the list Dec 28, see
> http://archives.postgresql.org/message-id/4D1A1677.80300%40sigaev.ru
>
> After a month I didn't see any activity on this patch, so I I added it to
> https://commitfest.postgresql.org/action/patch_view?id=350 Jan 21
>
> Now, I realised it was too late. Added to current commitfest.

I think this patch missed the deadline for the current CommitFest.
It's true that it was posted to the list in time, but it's just
madness to think we can do review in a meaningful way and get done in
a reasonable time if every patch that's ever been posted is fair game
to be added to the CommitFest at any point. I believe it's a
generally accepted principle that adding things to the CommitFest
properly is the submitter's responsibility.

That having been said, this looks like a fairly mechanical change to a
contrib module that you and Teodor wrote. So I'd say if you guys are
confident that it's correct, go ahead and commit. If you need it
reviewed, or if you can't commit it in the next week or so, I think
it's going to have to wait for 9.2.

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

Robert Haas wrote:
> On Fri, Feb 4, 2011 at 12:02 PM, Oleg Bartunov <oleg(at)sai(dot)msu(dot)su> wrote:
> > Aha,
> >
> > Teodor sent it to the list Dec 28, see
> > http://archives.postgresql.org/message-id/4D1A1677.80300%40sigaev.ru
> >
> > After a month I didn't see any activity on this patch, so I I added it to
> > https://commitfest.postgresql.org/action/patch_view?id=350 Jan 21
> >
> > Now, I realised it was too late. Added to current commitfest.
>
> I think this patch missed the deadline for the current CommitFest.
> It's true that it was posted to the list in time, but it's just
> madness to think we can do review in a meaningful way and get done in
> a reasonable time if every patch that's ever been posted is fair game
> to be added to the CommitFest at any point. I believe it's a
> generally accepted principle that adding things to the CommitFest
> properly is the submitter's responsibility.
>
> That having been said, this looks like a fairly mechanical change to a
> contrib module that you and Teodor wrote. So I'd say if you guys are
> confident that it's correct, go ahead and commit. If you need it
> reviewed, or if you can't commit it in the next week or so, I think
> it's going to have to wait for 9.2.

If you need a +1, agreed.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > Teodor sent it to the list Dec 28, see
> > http://archives.postgresql.org/message-id/4D1A1677.80300%40sigaev.ru
[...]
> That having been said, this looks like a fairly mechanical change to a
> contrib module that you and Teodor wrote. So I'd say if you guys are
> confident that it's correct, go ahead and commit. If you need it
> reviewed, or if you can't commit it in the next week or so, I think
> it's going to have to wait for 9.2.

Alright, I've gone through this patch and the main thing it's missing is
documentation, as far as I can tell. It passes all the regression tests
(and adds a number of them which are then tested with, which is always
nice) and while there are quite a few changes, they're all pretty
mechanical and simple. There are some really minor whitespace issues
too, but overall I think this is ready to be committed, so long as we
have a promise that someone will write up the documentation for it.

I'd write the docs, but I'm not 100% sure that I know what's going on
enough to really write them correctly. :) I'm also hoping that someone
else is already working on them. If not, feel free to ping me and I'll
work on writing up *something*, at least.

Thanks,

Stephen

Stephen,

what do you need for documentation ? From users point of view we add just
knn support and all examples are available in btree_gist.sql and sql
subdirectory. Contact me directly, if you have questions.

Oleg
On Fri, 11 Feb 2011, Stephen Frost wrote:

> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>>> Teodor sent it to the list Dec 28, see
>>> http://archives.postgresql.org/message-id/4D1A1677.80300%40sigaev.ru
> [...]
>> That having been said, this looks like a fairly mechanical change to a
>> contrib module that you and Teodor wrote. So I'd say if you guys are
>> confident that it's correct, go ahead and commit. If you need it
>> reviewed, or if you can't commit it in the next week or so, I think
>> it's going to have to wait for 9.2.
>
> Alright, I've gone through this patch and the main thing it's missing is
> documentation, as far as I can tell. It passes all the regression tests
> (and adds a number of them which are then tested with, which is always
> nice) and while there are quite a few changes, they're all pretty
> mechanical and simple. There are some really minor whitespace issues
> too, but overall I think this is ready to be committed, so long as we
> have a promise that someone will write up the documentation for it.
>
> I'd write the docs, but I'm not 100% sure that I know what's going on
> enough to really write them correctly. :) I'm also hoping that someone
> else is already working on them. If not, feel free to ping me and I'll
> work on writing up *something*, at least.
>
> Thanks,
>
> Stephen
>

Regards,
Oleg
_____________________________________________________________
Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
Sternberg Astronomical Institute, Moscow University, Russia
Internet: oleg(at)sai(dot)msu(dot)su, http://www.sai.msu.su/~megera/
phone: +007(495)939-16-83, +007(495)939-23-83

Oleg,

* Oleg Bartunov (oleg(at)sai(dot)msu(dot)su) wrote:
> what do you need for documentation ? From users point of view we add just
> knn support and all examples are available in btree_gist.sql and sql
> subdirectory. Contact me directly, if you have questions.

It sure seems like
http://www.postgresql.org/docs/9.0/static/btree-gist.html could be and
should be improved, in general.. If this module is really still just a
test bed for GiST, then perhaps it's not a big deal..

Thanks,

Stephen

On Sat, Feb 12, 2011 at 7:47 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Oleg,
>
> * Oleg Bartunov (oleg(at)sai(dot)msu(dot)su) wrote:
>> what do you need for documentation ? From users point of view we add just
>> knn support and all examples are available in btree_gist.sql and sql
>> subdirectory. Contact me directly, if you have questions.
>
> It sure seems like
> http://www.postgresql.org/docs/9.0/static/btree-gist.html could be and
> should be improved, in general..  If this module is really still just a
> test bed for GiST, then perhaps it's not a big deal..

I agree that the documentation there could be a lot better, but I
don't think that's a commit-blocker for this patch. However, "us
reaching beta" will be a commit-blocker.

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

Stephen,

On Sat, 12 Feb 2011, Stephen Frost wrote:

> Oleg,
>
> * Oleg Bartunov (oleg(at)sai(dot)msu(dot)su) wrote:
>> what do you need for documentation ? From users point of view we add just
>> knn support and all examples are available in btree_gist.sql and sql
>> subdirectory. Contact me directly, if you have questions.
>
> It sure seems like
> http://www.postgresql.org/docs/9.0/static/btree-gist.html could be and
> should be improved, in general.. If this module is really still just a
> test bed for GiST, then perhaps it's not a big deal..

No, it's quite useful and used in many projects, since it's the only way
to create multicolumn gist indexes like (tsvector,date).

Regards,
Oleg
_____________________________________________________________
Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
Sternberg Astronomical Institute, Moscow University, Russia
Internet: oleg(at)sai(dot)msu(dot)su, http://www.sai.msu.su/~megera/
phone: +007(495)939-16-83, +007(495)939-23-83

On Sat, 2011-02-12 at 18:53 +0300, Oleg Bartunov wrote:
> > It sure seems like
> > http://www.postgresql.org/docs/9.0/static/btree-gist.html could be and
> > should be improved, in general.. If this module is really still just a
> > test bed for GiST, then perhaps it's not a big deal..
>
> No, it's quite useful and used in many projects, since it's the only way
> to create multicolumn gist indexes like (tsvector,date).

+1

btree_gist is essentially required for exclusion constraints to be
useful in a practical way.

In fact, can you submit it for the next commitfest to be included in
core? That would allow range types and exclusion constraints to be used
out-of-the-box in 9.2.

Only if you think it's reasonable to put it in core, of course. If
extensions are easy enough to install, maybe that's not really
necessary.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> btree_gist is essentially required for exclusion constraints to be
> useful in a practical way.

> In fact, can you submit it for the next commitfest to be included in
> core? That would allow range types and exclusion constraints to be used
> out-of-the-box in 9.2.

> Only if you think it's reasonable to put it in core, of course. If
> extensions are easy enough to install, maybe that's not really
> necessary.

btree_gist is entirely unready to be included in core.
http://archives.postgresql.org/pgsql-bugs/2010-10/msg00104.php

regards, tom lane

Oops, thank for remind !

Oleg
On Sat, 12 Feb 2011, Tom Lane wrote:

> Jeff Davis <pgsql(at)j-davis(dot)com> writes:
>> btree_gist is essentially required for exclusion constraints to be
>> useful in a practical way.
>
>> In fact, can you submit it for the next commitfest to be included in
>> core? That would allow range types and exclusion constraints to be used
>> out-of-the-box in 9.2.
>
>> Only if you think it's reasonable to put it in core, of course. If
>> extensions are easy enough to install, maybe that's not really
>> necessary.
>
> btree_gist is entirely unready to be included in core.
> http://archives.postgresql.org/pgsql-bugs/2010-10/msg00104.php
>
> regards, tom lane
>
>

Regards,
Oleg
_____________________________________________________________
Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
Sternberg Astronomical Institute, Moscow University, Russia
Internet: oleg(at)sai(dot)msu(dot)su, http://www.sai.msu.su/~megera/
phone: +007(495)939-16-83, +007(495)939-23-83

On Sat, Feb 12, 2011 at 8:19 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Feb 12, 2011 at 7:47 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Oleg,
>>
>> * Oleg Bartunov (oleg(at)sai(dot)msu(dot)su) wrote:
>>> what do you need for documentation ? From users point of view we add just
>>> knn support and all examples are available in btree_gist.sql and sql
>>> subdirectory. Contact me directly, if you have questions.
>>
>> It sure seems like
>> http://www.postgresql.org/docs/9.0/static/btree-gist.html could be and
>> should be improved, in general..  If this module is really still just a
>> test bed for GiST, then perhaps it's not a big deal..
>
> I agree that the documentation there could be a lot better, but I
> don't think that's a commit-blocker for this patch.  However, "us
> reaching beta" will be a commit-blocker.

Teodor, are you intending to commit this? If so, it needs to be soon.

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

We need to fix inet support as Tom complained and then will commit.

Oleg
On Wed, 16 Feb 2011, Robert Haas wrote:

> On Sat, Feb 12, 2011 at 8:19 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sat, Feb 12, 2011 at 7:47 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>>> Oleg,
>>>
>>> * Oleg Bartunov (oleg(at)sai(dot)msu(dot)su) wrote:
>>>> what do you need for documentation ? From users point of view we add just
>>>> knn support and all examples are available in btree_gist.sql and sql
>>>> subdirectory. Contact me directly, if you have questions.
>>>
>>> It sure seems like
>>> http://www.postgresql.org/docs/9.0/static/btree-gist.html could be and
>>> should be improved, in general..  If this module is really still just a
>>> test bed for GiST, then perhaps it's not a big deal..
>>
>> I agree that the documentation there could be a lot better, but I
>> don't think that's a commit-blocker for this patch.  However, "us
>> reaching beta" will be a commit-blocker.
>
> Teodor, are you intending to commit this? If so, it needs to be soon.
>
>

Regards,
Oleg
_____________________________________________________________
Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
Sternberg Astronomical Institute, Moscow University, Russia
Internet: oleg(at)sai(dot)msu(dot)su, http://www.sai.msu.su/~megera/
phone: +007(495)939-16-83, +007(495)939-23-83

2011/2/17 Oleg Bartunov <oleg(at)sai(dot)msu(dot)su>:
> We need to fix inet support as Tom complained and then will commit.

I think those are two separate issues.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 2011/2/17 Oleg Bartunov <oleg(at)sai(dot)msu(dot)su>:
>> We need to fix inet support as Tom complained and then will commit.

> I think those are two separate issues.

Yeah ... also, fixing the inet problem looked to me like it'd probably
be a major compatibility break (change in on-disk index contents).
Not something to rush into at the end of a development cycle.

In any case, I was pointing to that as a reason that btree_gist wasn't
ready to be in core. It has nothing to do with KNN-ifying the module.
I would like to see that happen before 9.1, else KNN will go out with
not very many actual use-cases supported.

regards, tom lane

I wrote:
> In any case, I was pointing to that as a reason that btree_gist wasn't
> ready to be in core. It has nothing to do with KNN-ifying the module.
> I would like to see that happen before 9.1, else KNN will go out with
> not very many actual use-cases supported.

However, a larger reason for not applying that patch in a rush is that
it's almost certainly not up to speed for the recent extensions-related
changes. Please note in particular that we are now expecting the
update-from-unpackaged script to produce the same catalog state as the
install script. I just committed fixes to btree_gist's scripts to make
that actually true. Any new additions to the opclasses will need to be
done in the same style.

regards, tom lane