I wrote:
> The thing I'm complaining about is having dropped the intermediate
> struct that represents the fully decoded set of reloptions.
After looking at the patch a bit more I have a couple of other comments:
* I disagree with changing the argument of the RelationGetXXX
macros from Relation to bytea*, as in this example for instance:
* after initial build do not.
*/
gistdoinsert(index, itup,
! RelationGetTargetPageFreeSpace(index, GIST_DEFAULT_FILLFACTOR),
&buildstate->giststate);
buildstate->indtuples += 1;
--- 202,208 ----
* after initial build do not.
*/
gistdoinsert(index, itup,
! RelationGetTargetPageFreeSpace(index->rd_options,
GIST_DEFAULT_FILLFACTOR),
&buildstate->giststate);
buildstate->indtuples += 1;
This hasn't got anything to recommend it. It forecloses the possibility
of RelationGetXXX looking aside at, say, relkind or relam; as it might
wish to do to select a default value for example. The flip side of that
coin is that the call sites now know more than they need to about where
the value comes from (ie, from rd_options and not any other part of a
relcache entry). And in any case a function named RelationGetFoo
ought to take a Relation.
* On the other hand, a change I'd approve of is to get rid of the
call-site knowledge about appropriate defaults (GIST_DEFAULT_FILLFACTOR
in this example). If we're going to try to make the thing table-driven
then it's natural to put the default values into the table. You'd need
a slightly more complex table to support defaults that vary across index
AMs, but you really need that anyway since not all options might apply
to all AMs in the first place. I think the place this would end up is
that an rd_options struct would always be created for every relcache
entry, containing default values if nothing was supplied in pg_class.
Or maybe we should just get rid of rd_options and put all the options
fields directly into RelationData? The existing design is predicated
on an assumption that rd_options would be omitted most of the time,
but if we're always going to have it there then it's not real clear
that a separate palloc step buys much.
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
