Re: Macros bundling RELKIND_* conditions

On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 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.

I started grepping RELKIND_MATVIEW and convert corresponding
conditions into macros. I have gone through all the instances yet, so
I am not sure if all the macros are going to be used in only one
place. I will come to know that only once I have gone through all such
instances.

Some of the macros have same relkinds involved e.g.
RELKIND_HAS_VISIBILITY_MAP and RELKIND_HAS_XIDS or
RELKIND_CAN_HAVE_TOAST_TABLE and RELKIND_CAN_HAVE_INDEX. In such cases
it may be better to use a single macro instead of two by using a name
indicating some common property behind those tests. Can we say that
any relation that has visibility map will also have xids? If yes,
what's that common property? Similarly can any relation that can have
toast table also have an index? If yes, what's the common property? I
didn't get time to investigate it further.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company