From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Proposed refactoring of planner header files
Date: Thu, 31 Jan 2019 15:28:57 -0500
Message-ID: <9003.1548966537@sss.pgh.pa.us> (raw)
In-Reply-To: <CA+TgmoaZNN5Xb9SfiQEeFdL+zPdqmEMym8TX7Cecfae46ZSf+Q@mail.gmail.com>
References: <11460.1548706639@sss.pgh.pa.us>
<30320.1548904075@sss.pgh.pa.us>
<CA+Tgmoa9kQZJ1eCDaiupQ5ZVW3c65YxnoE1NwwwdZzO=0gp=mw@mail.gmail.com>
<11671.1548963958@sss.pgh.pa.us>
<CA+TgmoaZNN5Xb9SfiQEeFdL+zPdqmEMym8TX7Cecfae46ZSf+Q@mail.gmail.com>
Robert Haas <robertmhaas@gmail.com> writes:
> I do not have a powerful opinion on exactly what to do here, but I
> offer the following for what it's worth:
> - I do not really think much of the selfuncs.c naming. Nobody who is
> not deeply immersed in the PostgreSQL code knows what a "selfunc" is.
> Therefore, breaking selfuncs.c into opr_selfuncs.c and
> index_selfuncs.c doesn't strike me as the ideal naming choice. I
> would suggest looking for a name that is less steeped in venerable
> tradition; perhaps estimator.c or indexsupport.c or selectivity.c or
> whatever could be considered for whatever new files we are creating.
Hm. I'm not really persuaded. There isn't any part of this code that
isn't "steeped in venerable tradition"; the problem I'm ultimately
trying to fix here can fairly be characterized as 25-year-old technical
debt. I see your point that adt/ is not where new people might look for
selectivity estimators, but we should also have some sympathy for people
who *do* know the code base expecting to find these functions more or
less where they are.
> - I have always found the placement of selfuncs.c a bit strange: why
> is it in utils/adt? Perhaps there is an argument for having the
> things that are specific to a particular type or a particular operator
> in that directory, but if so, shouldn't they be grouped with the type
> or operator to which they relate, rather than each other?
We do already have that to some extent:
src/backend/tsearch/ts_selfuncs.c
src/backend/utils/adt/array_selfuncs.c
src/backend/utils/adt/rangetypes_selfuncs.c
src/backend/utils/adt/geo_selfuncs.c
src/backend/utils/adt/network_selfuncs.c
src/backend/utils/adt/selfuncs.c
src/include/utils/index_selfuncs.h
src/include/utils/selfuncs.h
contrib/intarray/_int_selfuncs.c
Breaking up selfuncs.c would carry this a bit further, but giving it
a random new name doesn't seem to me to really improve matters.
Now, if we move the support functions to someplace under optimizer/,
we do have to think about a name for that. I was considering
optimizer/utils/selsupport.c and optimizer/selsupport.h, but I'm
certainly not wedded to that.
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Proposed refactoring of planner header files
In-Reply-To: <9003.1548966537@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox