Forgot to attach the patch with the earlier mail. On Mon, Jul 3, 2017 at 1:22 PM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > On Mon, May 29, 2017 at 12:55 PM, Ashutosh Bapat > <ashutosh.ba...@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-990d2e0aa...@joeconway.com > > -- > Best Wishes, > Ashutosh Bapat > EnterpriseDB Corporation > The Postgres Database Company
-- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
pg_relkind_macros_patches_v2.tar.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers