Re: Macros bundling RELKIND_* conditions

On Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> I noticed, that
>> after we introduced RELKIND_PARTITIONED_TABLE, we required to change a
>> number of conditions to include this relkind. We missed some places in
>> initial commits and fixed those later. I am wondering whether we
>> should creates macros clubbing relevant relkinds together based on the
>> purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind
>> is added, one can examine these macros to check whether the new
>> relkind fits in the given macro. If all those macros are placed
>> together, there is a high chance that we will not miss any place in
>> the initial commit itself.

I've thought about this kind of thing, too. But the thing is that
most of these macros you're proposing to introduce only get used in
one place.

0001-RELKIND_HAS_VISIBILITY_MAP.patch - one place
0002-RELKIND_HAS_STORAGE.patch - one place
0003-RELKIND_HAS_XIDS-macro.patch - one place
0004-RELKIND_HAS_COMPOSITE_TYPE-macro.patch - one place
0005-RELKIND_CAN_HAVE_TOAST_TABLE-macro.patch - one place
0006-RELKIND_CAN_HAVE_COLUMN_COMMENT-macro.patch - one place
0007-RELKIND_CAN_HAVE_INDEX-macro.patch - two places
0008-RELKIND_CAN_HAVE_COLUMN_SECLABEL-macro.patch - one place
0009-RELKIND_CAN_HAVE_STATS-macro.patch - two places

I'm totally cool with doing this where we can use the macro in more
than one place, but otherwise I don't think it helps.

> With this approach the macro which tests relkinds and the macro which
> reports error are places together in pg_class.h. If somebody adds a
> new relkind, s/he will notice the comment there and update the macros
> below also keeping the error message in sync with the test. Please
> note that partitioned tables are not explicitly mentioned in the error
> messages when the corresponding test has RELKIND_PARTITIONED_TABLE. I
> think we don't need to differentiate between a regular table and
> partitioned table in those error messages; a "table" implies both a
> regular table and a partitioned table.

I'm honestly not sure this buys us anything, unless you can use those
macros in a lot more places.

> With this approach, if a developer may still fail to update the error
> message when the test is updated. We can further tighten this by
> following approach.
> 1. For every test declare an array of relkinds that the test accepts e.g.
> int relkinds_with_vm[] = {RELKIND_RELATION, RELKIND_MATVIEW,
> RELKIND_TOASTVALUE};
> 2. Write a function is_relkind_in_array(int *relkinds_array, int
> num_relkinds, int relkind) to check whether the given relkind is in
> the array.
> 3. Each test macro now calls this function passing appropriate array
> e.g. #define RELKIND_WITH_VISIBILITY_MAP(relkind) \
> is_relkind_in_array(relkinds_with_vm,
> sizeof(relkinds_with_vm)/sizeof(relkinds_with_vm[0], (relkind))
> 4. Declare an array of relkinds and their readable strings e.g
> {{RELKIND_RELATION, "table"}, {RELKIND_MATVIEW, "materialized view"}}
> 5. Write a function to collect the readable strings for all relkinds a
> given array of relkinds say char *relkind_names(int *relkinds, int
> num_relkinds)
> 6. Declare error message macros to call this function by passing
> appropriate array.

I think this might cause some problems for translators.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company