Small improvement to get_base_rel_indexes()

Started by David Rowleyover 10 years ago2 messageshackers
Jump to latest
#1David Rowley
dgrowleyml@gmail.com

Attached is a small patch which improves the way get_base_rel_indexes()
works.

The current version creates a new bitmapset on each recursion level then
bms_joins() to the one on the next level up each time. I understand that
this will patch will have about a 0 net performance improvement, but I
thought I'd post anyway as:

1. It removes 5 lines of code.
2. It's a better example to leave in the code.

Is it worth applying?

Regards

David Rowley
--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

get_base_rel_indexes_cleanup.difftext/plain; charset=US-ASCII; name=get_base_rel_indexes_cleanup.diffDownload+9-14
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: David Rowley (#1)
Re: Small improvement to get_base_rel_indexes()

David Rowley <david.rowley@2ndquadrant.com> writes:

Attached is a small patch which improves the way get_base_rel_indexes()
works.

The current version creates a new bitmapset on each recursion level then
bms_joins() to the one on the next level up each time. I understand that
this will patch will have about a 0 net performance improvement, but I
thought I'd post anyway as:

1. It removes 5 lines of code.
2. It's a better example to leave in the code.

I don't know that it's a better example: it seems substantially uglier.
(Notably, you complicated the API contract of get_base_rel_indexes()
without documenting the fact: it's now dependent on the original caller
to have initialized *result to NULL.)

We might have to use a method like this if the function were returning
multiple sets; but since it is not, the existing approach seems more
readable and safer to me.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers