Обсуждение: match_clause_to_indexcol()
I was looking at KNNGIST some more today and found myself trying to disentangle what match_clause_to_indexcol() is actually doing. It appears to me that the opfamily passed to that function is always the same as index->opfamily[indexcol], which seems like needless notational complexity. Maybe I'm missing something, but the attached patch seems to make things simpler and clearer. Thoughts? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
Robert Haas <robertmhaas@gmail.com> writes:
> I was looking at KNNGIST some more today and found myself trying to
> disentangle what match_clause_to_indexcol() is actually doing. It
> appears to me that the opfamily passed to that function is always the
> same as index->opfamily[indexcol], which seems like needless
> notational complexity. Maybe I'm missing something, but the attached
> patch seems to make things simpler and clearer. Thoughts?
+1. I think the existing coding dates from a time when we didn't have
IndexOptInfo at all, or at least didn't pass it around to all these
sub-functions, so there was no other path for getting at the info.
But if you're going to do that, get rid of DoneMatchingIndexKeys
altogether, along with the extra zero that plancat.c adds to the
opfamily array. We don't need to be using more than one way to
iterate over those arrays.
regards, tom lane
On Sat, Nov 20, 2010 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I was looking at KNNGIST some more today and found myself trying to >> disentangle what match_clause_to_indexcol() is actually doing. It >> appears to me that the opfamily passed to that function is always the >> same as index->opfamily[indexcol], which seems like needless >> notational complexity. Maybe I'm missing something, but the attached >> patch seems to make things simpler and clearer. Thoughts? > > +1. I think the existing coding dates from a time when we didn't have > IndexOptInfo at all, or at least didn't pass it around to all these > sub-functions, so there was no other path for getting at the info. > > But if you're going to do that, get rid of DoneMatchingIndexKeys > altogether, Sure. That's a giant crock. > along with the extra zero that plancat.c adds to the > opfamily array. We don't need to be using more than one way to > iterate over those arrays. I am slightly worried that this might break third-party code, or even some part of the core code that's still using the old system. I don't mind if you want to rip it out, but personally, I'd rather leave that part well enough alone. Revised patch attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Nov 20, 2010 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> But if you're going to do that, get rid of DoneMatchingIndexKeys
>> altogether,
> Sure. That's a giant crock.
>> along with the extra zero that plancat.c adds to the
>> opfamily array. �We don't need to be using more than one way to
>> iterate over those arrays.
> I am slightly worried that this might break third-party code, or even
> some part of the core code that's still using the old system. I don't
> mind if you want to rip it out, but personally, I'd rather leave that
> part well enough alone. Revised patch attached.
I'll take the responsibility if you don't want to ;-)
I think your revised patch is incorrect, or at least not terribly safe,
to just remove the last DoneMatchingIndexKeys test and not replace it
with anything else. Other than that it looks okay as far as it goes.
Do you want to commit it yourself, or shall I incorporate it in a more
extensive cleanup?
regards, tom lane
On Sat, Nov 20, 2010 at 1:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sat, Nov 20, 2010 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> But if you're going to do that, get rid of DoneMatchingIndexKeys >>> altogether, > >> Sure. That's a giant crock. > >>> along with the extra zero that plancat.c adds to the >>> opfamily array. We don't need to be using more than one way to >>> iterate over those arrays. > >> I am slightly worried that this might break third-party code, or even >> some part of the core code that's still using the old system. I don't >> mind if you want to rip it out, but personally, I'd rather leave that >> part well enough alone. Revised patch attached. > > I'll take the responsibility if you don't want to ;-) Sold! :-) > I think your revised patch is incorrect, or at least not terribly safe, > to just remove the last DoneMatchingIndexKeys test and not replace it > with anything else. Other than that it looks okay as far as it goes. > Do you want to commit it yourself, or shall I incorporate it in a more > extensive cleanup? If it's all right with you, I'll go ahead and commit this and then you can break as much more stuff as you like. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Nov 20, 2010 at 1:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think your revised patch is incorrect, or at least not terribly safe,
>> to just remove the last DoneMatchingIndexKeys test and not replace it
>> with anything else.
Oh, now that I look at it I notice the after-the-fact Assert claiming
that the clausegroup list isn't too long. That can definitely be done
better.
>> Do you want to commit it yourself, or shall I incorporate it in a more
>> extensive cleanup?
> If it's all right with you, I'll go ahead and commit this and then you
> can break as much more stuff as you like. :-)
Go for it.
regards, tom lane
On Sat, Nov 20, 2010 at 1:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> If it's all right with you, I'll go ahead and commit this and then you >> can break as much more stuff as you like. :-) > > Go for it. Your turn. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company