Andrew Gierth <and...@tao11.riddles.org.uk> writes: > - this still has everything in amapi.c rather than creating any new > files. Also, the regression tests are in create_index.sql for lack > of any obviously better place.
This more than doubles the size of amapi.c, so it has a definite feel of tail-wags-dog for me, even if that seemed like an appropriate place otherwise which it doesn't really. I think a new .c file under adt/ is the way to go, with extern declarations in builtins.h. Maybe we need a new regression test case file too. I don't much like adding this to create_index because (a) it doesn't particularly seem to match that file's purpose of setting up indexes on the standard regression test tables, and (b) that file is a bottleneck in parallel regression runs because it can't run in parallel with much else. > The list of column properties is: > ordered - (same as "amcanorder" AM capability) > ordered_asc > ordered_desc > ordered_nulls_first > ordered_nulls_last > If "ordered" is true then exactly one of _asc/_desc and exactly one of > _nulls_first/_last will be true; if "ordered" is false then all the > others will be false too. Check, but do we need the "ordered_" prefixes on the last four? Seems like mostly useless typing. And the name space here is never going to be so large that it'd be confusing. > Comments? Why did you go with "capability" rather than "property" in the exposed function names? The latter seems much closer to le mot juste, especially for things like asc/desc. I'd expect the property keywords to be recognized case-insensitively, as for example are the keywords in has_table_privilege() and its ilk. That being the case, you should be using pg_strcasecmp(). This stuff: if (namelen == 10 && memcmp(nameptr, "amcanorder", 10) == 0) is ugly and hard to maintain as well as being unnecessarily non-user- friendly. Just convert the input to a C string and be done with it. It's not like saving a couple of nanoseconds is critical here. I'd personally cut the list of pg_am replacement properties way back, as I believe much of what you've got there is not actually of use to applications, and some of it is outright counterproductive. An example is exposing amcanreturn as an index-AM boolean. That's just wrong these days. If you want that, it should be an index column property. And because what used to be an index-AM property no longer is in that case, I think it's a fine example of why we shouldn't be reporting more than the absolute minimum here. It would tie our hands with respect to making other improvements of that kind later. (This might be a good argument for moving as much as we can over to the index-column-property function, even if it can't be set per-column today. If there's a chance that it could be per-column in the future, let's not call it an AM property.) Also, while I see the point of the amgettuple and amgetbitmap properties, I would call them something like "index_scan" and "bitmap_scan" because that's the user's-eye view of what is supported. I do not think we should slavishly tie the property names to the old, never-intended-as-user-facing column names. I'd drop the "am" prefix, for starters. regards, tom lane -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers