Re: Macros bundling RELKIND_* conditions

On Mon, May 29, 2017 at 12:55 PM, 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.
>
> For example, if we had a macro IS_RELKIND_WITH_STATS defined as
> #define IS_RELKIND_WITH_STATS(relkind) \
> ((relkind) == RELKIND_RELATION || \
> (relkind) == RELKIND_MATVIEW)
>
> and CreateStatistics() and getExtendedStatistics() had following conditions
> if (!IS_RELKIND_WITH_STATS(rel->rd_rel->relkind)) and if
> (!IS_RELKIND_WITH_STATS(tabinfo->relkind)) resp. The patch would be
> just adding
> (relkind) == RELKIND_FOREIGN_TABLE || \
> (relkind) == RELKIND_PARTITIONED_TABLE)
>
> to that macro without requiring to find out where all we need to add
> those two relkinds for statistics purposes.
> -- excerpt ends
>
> Peter Eisentraut thought that idea is worth a try. I gave it a try on
> my way back from PGCon. Attached is a series of patches, one per
> macro. This isn't a complete series but will give an idea of what's
> involved. It might be possible to change switch cases at some places
> to use if else with these macros. But I haven't done any changes
> towards that.
>

On the thread [1] Joe and Dean expressed that it would be good if we
could also keep the related error messages at a central place. In the
attached patches, I have tried to do that my defining corresponding
ERRMSG macros encapsulating errmsg() macros in ereport() calls. Please
let me know, if this looks good.

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.

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.

Obviously with this approach we would loose a bit of readability. Also
we will be forced to include all the relkind strings and won't be able
to use "table" to mean both regular and partitioned table as we do
today. I am not able to decide whether that's a good change or not.
So, haven't coded it up.

Let me know your opinion.

The patches do not convert all the places that can be converted to use
macros. Once we agree upon the approach, I will continue converting
those.

[1] https://www.postgresql.org/message-id/803c7229-bac6-586a-165b-990d2e0aa68b@joeconway.com

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