Thread: [PATCH] inet << indexability
This is second take at indexability of << operator for inet types. Please take a look at it. Also, I have a question: I put in a regression test to check that the type can be indexed, by doing 'explain select ...'. However, the expected result may vary when the optimizer is tweaked. I am not sure if its a good idea to check for that, so feel free to not commit the regression test part of this patch...If there's a better way to check that the query will use the index in regression test, I'd like to know too. -alex
Augh. Previous patch had some garbage changes in it. Sorry. This one is clean...I promise, I'll get better at this. -alex On Fri, 15 Jun 2001, Alex Pilosov wrote: > This is second take at indexability of << operator for inet types. > > Please take a look at it. > > Also, I have a question: I put in a regression test to check that the type > can be indexed, by doing 'explain select ...'. However, the expected > result may vary when the optimizer is tweaked. > > I am not sure if its a good idea to check for that, so feel free to not > commit the regression test part of this patch...If there's a better way to > check that the query will use the index in regression test, I'd like to > know too. > > -alex >
Alex Pilosov <alex@pilosoft.com> writes:
> Also, I have a question: I put in a regression test to check that the type
> can be indexed, by doing 'explain select ...'. However, the expected
> result may vary when the optimizer is tweaked.
Yes, I'd noted that already in looking at your prior version. I think
it's best not to do an EXPLAIN in the regress test, because I don't want
to have to touch the tests every time the cost functions are tweaked.
However, we can certainly check to make sure that the results of an
indexscan are what we expect. Is the table set up so that this is a
useful test case? For example, it'd be nice to check boundary
conditions (eg, try both << and <<= on a case where they should give
different results).
Do you have any thought of making network_scan_first and
network_scan_last user-visible functions? (Offhand I see no use for
them to a user, but maybe you do.) If not, I'd suggest not using the
fmgr call protocol for them, but just making them pass and return inet*,
or possibly Datum. No need for the extra notational cruft of
DirectFunctionCall.
Another minor stylistic gripe is that you should use bool/true/false
where appropriate, not int/1/0. Otherwise it looks pretty good.
Oh, one more thing: those dynloader/openbsd.h and psql/tab-complete.c
changes don't belong in this patch...
regards, tom lane
On Sat, 16 Jun 2001, Tom Lane wrote: > Alex Pilosov <alex@pilosoft.com> writes: > > Also, I have a question: I put in a regression test to check that the type > > can be indexed, by doing 'explain select ...'. However, the expected > > result may vary when the optimizer is tweaked. > > Yes, I'd noted that already in looking at your prior version. I think > it's best not to do an EXPLAIN in the regress test, because I don't want > to have to touch the tests every time the cost functions are tweaked. I'll remove it with resubmitted patch. > However, we can certainly check to make sure that the results of an > indexscan are what we expect. Is the table set up so that this is a > useful test case? For example, it'd be nice to check boundary > conditions (eg, try both << and <<= on a case where they should give > different results). I'll do that too. > Do you have any thought of making network_scan_first and > network_scan_last user-visible functions? (Offhand I see no use for > them to a user, but maybe you do.) If not, I'd suggest not using the > fmgr call protocol for them, but just making them pass and return inet*, > or possibly Datum. No need for the extra notational cruft of > DirectFunctionCall. I didn't want to make them user-visible, however, the alternative, IMHO, is worse, since these functions rely on network_broadcast and network_network to do the work, calling sequence would be: a) indxpath casts Datum to inet, passes to network_scan* b) network_scan will create new Datum, pass it to network_broadcast c) network_scan will extract inet from Datum returned d) indxpath will then cast inet back to Datum :) Which, I think, is pretty messy :) > Another minor stylistic gripe is that you should use bool/true/false > where appropriate, not int/1/0. Otherwise it looks pretty good. I'll clean it up and resubmit. > Oh, one more thing: those dynloader/openbsd.h and psql/tab-complete.c > changes don't belong in this patch... Sorry, my fault -alex
Alex Pilosov <alex@pilosoft.com> writes:
> I didn't want to make them user-visible, however, the alternative, IMHO,
> is worse, since these functions rely on network_broadcast and
> network_network to do the work, calling sequence would be:
> a) indxpath casts Datum to inet, passes to network_scan*
> b) network_scan will create new Datum, pass it to network_broadcast
> c) network_scan will extract inet from Datum returned
> d) indxpath will then cast inet back to Datum :)
> Which, I think, is pretty messy :)
Sure, but you could make them look like
Datum network_scan_first(Datum networkaddress)
without incurring any of that overhead. (Anyway, Datum <-> inet* is
only a cast.)
regards, tom lane
On Sat, 16 Jun 2001, Tom Lane wrote: > Alex Pilosov <alex@pilosoft.com> writes: > > I didn't want to make them user-visible, however, the alternative, IMHO, > > is worse, since these functions rely on network_broadcast and > > network_network to do the work, calling sequence would be: > > a) indxpath casts Datum to inet, passes to network_scan* > > b) network_scan will create new Datum, pass it to network_broadcast > > c) network_scan will extract inet from Datum returned > > d) indxpath will then cast inet back to Datum :) > > Which, I think, is pretty messy :) > > Sure, but you could make them look like > > Datum network_scan_first(Datum networkaddress) > > without incurring any of that overhead. (Anyway, Datum <-> inet* is > only a cast.) Gotcha, I misunderstood you the first time. Thanks -alex
Your patch has been added to the PostgreSQL unapplied patches list at: http://candle.pha.pa.us/cgi-bin/pgpatches I will try to apply it within the next 48 hours. > Augh. Previous patch had some garbage changes in it. Sorry. This one is > clean...I promise, I'll get better at this. > > -alex > > On Fri, 15 Jun 2001, Alex Pilosov wrote: > > > This is second take at indexability of << operator for inet types. > > > > Please take a look at it. > > > > Also, I have a question: I put in a regression test to check that the type > > can be indexed, by doing 'explain select ...'. However, the expected > > result may vary when the optimizer is tweaked. > > > > I am not sure if its a good idea to check for that, so feel free to not > > commit the regression test part of this patch...If there's a better way to > > check that the query will use the index in regression test, I'd like to > > know too. > > > > -alex > > Content-Description: [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Tom already merged [latest version of this patch] it in, so you can delete this one from patch list. Thanks -alex On Mon, 18 Jun 2001, Bruce Momjian wrote: > Your patch has been added to the PostgreSQL unapplied patches list at: > > http://candle.pha.pa.us/cgi-bin/pgpatches > > I will try to apply it within the next 48 hours. > > > Augh. Previous patch had some garbage changes in it. Sorry. This one is > > clean...I promise, I'll get better at this. > > > > -alex > > > > On Fri, 15 Jun 2001, Alex Pilosov wrote: > > > > > This is second take at indexability of << operator for inet types. > > > > > > Please take a look at it. > > > > > > Also, I have a question: I put in a regression test to check that the type > > > can be indexed, by doing 'explain select ...'. However, the expected > > > result may vary when the optimizer is tweaked. > > > > > > I am not sure if its a good idea to check for that, so feel free to not > > > commit the regression test part of this patch...If there's a better way to > > > check that the query will use the index in regression test, I'd like to > > > know too. > > > > > > -alex > > > > > Content-Description: > > [ Attachment, skipping... ] > > > > > ---------------------------(end of broadcast)--------------------------- > > TIP 4: Don't 'kill -9' the postmaster > >
> Tom already merged [latest version of this patch] it in, so you can delete > this one from patch list. Oh, OK. I thought he had done that but I couldn't find the commit message in my mailbox. > > Thanks > -alex > > > On Mon, 18 Jun 2001, Bruce Momjian wrote: > > > Your patch has been added to the PostgreSQL unapplied patches list at: > > > > http://candle.pha.pa.us/cgi-bin/pgpatches > > > > I will try to apply it within the next 48 hours. > > > > > Augh. Previous patch had some garbage changes in it. Sorry. This one is > > > clean...I promise, I'll get better at this. > > > > > > -alex > > > > > > On Fri, 15 Jun 2001, Alex Pilosov wrote: > > > > > > > This is second take at indexability of << operator for inet types. > > > > > > > > Please take a look at it. > > > > > > > > Also, I have a question: I put in a regression test to check that the type > > > > can be indexed, by doing 'explain select ...'. However, the expected > > > > result may vary when the optimizer is tweaked. > > > > > > > > I am not sure if its a good idea to check for that, so feel free to not > > > > commit the regression test part of this patch...If there's a better way to > > > > check that the query will use the index in regression test, I'd like to > > > > know too. > > > > > > > > -alex > > > > > > > > Content-Description: > > > > [ Attachment, skipping... ] > > > > > > > > ---------------------------(end of broadcast)--------------------------- > > > TIP 4: Don't 'kill -9' the postmaster > > > > > > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> ... best not to do an EXPLAIN in the regress test, because I don't want
> to have to touch the tests every time the cost functions are tweaked...
At some point we really should have an "optimizer" regression test, so
y'all *do* have to touch some regression test when the cost functions
are tweaked. If it were isolated into a single test, with appropriate
comments to keep it easy to remember *why* a result should be a certain
way, then it should be manageable and make it easier to proactively
evaluate changes.
It likely would have have full coverage, but at least some over/under
test cases would help...
- Thomas