Re: [HACKERS] Macros bundling RELKIND_* conditions

Forgot to attach the patch with the earlier mail.

On Mon, Jul 3, 2017 at 1:22 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Mon, May 29, 2017 at 12:55 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.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



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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers