On Tue, Jan 15, 2019 at 1:43 PM Andres Freund <and...@anarazel.de> wrote:
> Hi, > > On 2019-01-11 12:01:36 -0500, Robert Haas wrote: > > On Thu, Jan 10, 2019 at 7:05 PM Andres Freund <and...@anarazel.de> > wrote: > > > ISTM that the first block would best belong into new files like > > > access/rel[ation].h and access/common/rel[ation].h. > > > > +1. > > > > > I think the second > > > set should be renamed to be table_open() (with backward compat > > > redirects, there's way way too many references) and should go into > > > access/table.h access/table/table.c alongside tableam.[ch], > > > > Sounds reasonable. > > > > > but I could > > > also see just putting them into relation.[ch]. > > > > I would view that as a less-preferred alternative, but not crazy. > > Here's a set of patches. Not commit quality, but enough to discuss. > > > The first patch, the only really interesting one, splits out > relation_(open|openrv|openrv_extended|close) into access/relation.h and > access/common/relation.c > and > heap_(open|openrv|openrv_extended|close) into access/table.h and > access/table/table.c > > It's worthwhile to note that there's another header named > nodes/relation.h. But there's also utils/rel.h, so I couldn't think of a > another good name. > access/relation.[c|h] name is fine. Or how about access/rel.[c|h], because nodes/relation.h is related to planner. utils/rel.h is some how related to relation caches. > I'm basically thinking that table.h, even in the post pluggable storage > world, should not contain lower level functionality like dispatching > into table-am (that'll reside in tableam.h). But e.g. a > simple_table_(insert|update|delete) could live there, as well as > potentially some other heap_ functionality strewn around the backend. > > I made table.h not include relation.h, which means that a few files > might need both. I'm not sure that's the right choice, but it seems > easier to extend that later if shows to be painful, than to do the > reverse. > The need of both relation.h and table.h into a single file is because of use of both relation_open table_open functions. when I checked one of the file where both headers are included, I found that it simply used the relation_open function even the type of the relation is known. I didn't check whether it is possible or not? In case if the kind of the relation is known at every caller of relation_open, it can be replaced with either table_open or index_open or composite_type_open functions. So that there may not need any of the relation functions needs to be exposed. I've left the following in table.h: > /* > * heap_ used to be the prefix for these routines, and a lot of code will > just > * continue to work without adaptions after the introduction of pluggable > * storage, therefore just map these names. > */ > #define heap_open(r, l) table_open(r, l) > #define heap_openrv(r, l) table_openrv(r, l) > #define heap_openrv_extended(r, l, m) table_openrv_extended(r, l, m) > #define heap_close(r, l) table_close(r, l) > > and I think we should leave that in there for the forseeable future. > Above typedefs are good. They are useful to avoid any third party extensions. Regards, Haribabu Kommi Fujitsu Australia
