Re: [HACKERS] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

Tom Lane wrote:

> I think Peter's got the error and the detail backwards.  It should be
> more like
> 
> ERROR: "someview" cannot have constraints
> DETAIL: "someview" is a view.
> 
> If we do it like that, we need one ERROR message per error reason,
> and one DETAIL per relkind, which should be manageable.

Hmm ... this works for me.  Hopefully we'd have the "foo is a view"
messages all centrally in pg_class.h (or maybe objectaddress, or some
other central place).  Then the "cannot have constraints" part would
appear directly in whatever .c file is doing the check; no need to
centralize in that case.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

Joe Conway wrote:
> On 08/02/2017 10:52 PM, Ashutosh Bapat wrote:
> > On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
> >  wrote:
> >> Alvaro Herrera wrote:
> >>> I think pg_class is a reasonable place to put more generic relkind lists
> >>> alongside a matching error message for each, rather than specialized
> >>> "does this relkind have storage" macros.  What about something like a
> >>> struct list in pg_class.h,
> >>
> >> I just noticed that this doesn't help at all with the initial problem
> >> statement, which is that some of the relkind checks failed to notice
> >> that partitioned tables needed to be added to the set.  Maybe it still
> >> helps because you have something to grep for, as Tom proposed elsewhere.
> > 
> > Having something like relkind_i_t_T, though correct, doesn't make the
> > test readable. That's another improvement because of using such
> > macros. The macro name tells the purpose of the test, which is what a
> > reader would be interested in, rather than the actual values used.
> 
> +1

So add another layer:

#define RELKIND_HAS_STORAGE relkind_i_t_T

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Joe Conway]

On 08/02/2017 10:52 PM, Ashutosh Bapat wrote:
> On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
>  wrote:
>> Alvaro Herrera wrote:
>>> I think pg_class is a reasonable place to put more generic relkind lists
>>> alongside a matching error message for each, rather than specialized
>>> "does this relkind have storage" macros.  What about something like a
>>> struct list in pg_class.h,
>>
>> I just noticed that this doesn't help at all with the initial problem
>> statement, which is that some of the relkind checks failed to notice
>> that partitioned tables needed to be added to the set.  Maybe it still
>> helps because you have something to grep for, as Tom proposed elsewhere.
> 
> Having something like relkind_i_t_T, though correct, doesn't make the
> test readable. That's another improvement because of using such
> macros. The macro name tells the purpose of the test, which is what a
> reader would be interested in, rather than the actual values used.

+1

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Joe Conway]

On 08/02/2017 10:30 PM, Ashutosh Bapat wrote:
> On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas  wrote:
>> 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 disagree.

> Can we say that any relation that has visibility map will also have
> xids? If yes, what's that common property?

Perhaps there are better ways to achieve the same goal (e.g. nearby
comments), but I think this is the salient point. The macro names allow
whoever is looking at the code to focus on the relevant properties of
the relkind without having to arrive at the conclusion themselves that
relkind "A" has property "B". Makes the code easier to read and
understand IMHO.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
 wrote:
> Alvaro Herrera wrote:
>> I think pg_class is a reasonable place to put more generic relkind lists
>> alongside a matching error message for each, rather than specialized
>> "does this relkind have storage" macros.  What about something like a
>> struct list in pg_class.h,
>
> I just noticed that this doesn't help at all with the initial problem
> statement, which is that some of the relkind checks failed to notice
> that partitioned tables needed to be added to the set.  Maybe it still
> helps because you have something to grep for, as Tom proposed elsewhere.

Having something like relkind_i_t_T, though correct, doesn't make the
test readable. That's another improvement because of using such
macros. The macro name tells the purpose of the test, which is what a
reader would be interested in, rather than the actual values used.

>
> However, if there are multiple places that should be kept in sync
> regarding which relkinds to check, then I don't understand Robert's
> objection that only one place requires the check.  Surely we're having
> this discussion precisely because more than one place needs the check,
> and finding those places is not obvious?
>

A new relkind may be required to be added in tests at multiple places.
But each place may have different set of relkinds in that test, which
gets converted into a macro. Given that we have now 9 relkinds, we
could theoretically have 2^9 tests and those many macros. Maintaining
all those would be more cumbersome than grepping RELKIND_ in code.
>From that perspective Robert's concern is valid, but my feeling is
that we are actually using far lesser combinations than that. As I
said, I haven't gone through all the places, so, I don't know the
whole picture yet. But given the number of places where we added
RELKIND_PARTITIONED_TABLE, I guess, there are more than one places
where at least some of those tests are used.

-- 
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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Wed, Aug 2, 2017 at 10:28 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> 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.
>
> I think the value would be in having a centralized checklist of
> things-to-fix-when-adding-a-new-relkind.

right.

> There's more than one way
> to reach that goal, though.  I wonder whether the task should be defined
> more like "grep for 'RELKIND_' and fix every place you find that".

That way one has to scan all code and change many files. Having them
centrally at one place reduces that pain.

> If there are places to touch that fail to mention that string, fix
> them, using comments if nothing else.

I didn't get this.

> (But see fe797b4a6 and
> followon commits for other solutions.)

That and the follow-on commits replace hard-coded relkind values by
corresponding macro. Though that work it itself is important, I do not
see how that helps to find all the places where the new relkind added
needs to be checked.

>
>> I think this might cause some problems for translators.
>
> Yeah, the error messages that list a bunch of different relkinds in text
> form are going to be a hassle no matter what.  Most of the ways you might
> think of to change that will violate our translatability rules.
>

Ok. I agree with that. May be for now, we shouldn't touch the error
messages at all.

-- 
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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas  wrote:
>  On Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat
>  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


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> The actual error, from the perspective of the user, is something like
>> ERROR: "someview" is a view
>> DETAIL: Views cannot have constraints.

> OK.  "%s is a %s" is a reasonable set of errors -- we just need one for
> each relkind.  So the first one is easy.

> But the second one is not easy, because we'd need one message per
> relkind per operation kind.  We cannot possibly write/translate that
> many messages.  If we make the relkind generic in the errdetail message,
> maybe it can work; something like "Relations of that type cannot have
> constraints" would work, for example.  Or "Relations of type "view"
> cannot have constraints", although this reads very strangely.  Maybe
> someone has a better idea?

I think Peter's got the error and the detail backwards.  It should be
more like

ERROR: "someview" cannot have constraints
DETAIL: "someview" is a view.

If we do it like that, we need one ERROR message per error reason,
and one DETAIL per relkind, which should be manageable.

A more verbose approach is

ERROR: "someview" cannot have constraints
DETAIL: "someview" is a view, which is not a supported kind of relation
for this purpose.

regards, tom lane


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

Peter Eisentraut wrote:

> I don't find this style of error message optimal anyway.  If I do, for
> example
> 
> ALTER TABLE someview ADD CONSTRAINT ...
> ERROR: "someview" is not a table, foreign table, whatever
> 
> then this information is not helpful.  It's not like I'm going to turn
> my view into a foreign table in order to be able to proceed with that
> command.

Hmm, this is a good point ... not against my proposal, but rather
against the current coding.  I agree it could be more user-friendly.

> The actual error, from the perspective of the user, is something like
> 
> ERROR: "someview" is a view
> DETAIL: Views cannot have constraints.

OK.  "%s is a %s" is a reasonable set of errors -- we just need one for
each relkind.  So the first one is easy.

But the second one is not easy, because we'd need one message per
relkind per operation kind.  We cannot possibly write/translate that
many messages.  If we make the relkind generic in the errdetail message,
maybe it can work; something like "Relations of that type cannot have
constraints" would work, for example.  Or "Relations of type "view"
cannot have constraints", although this reads very strangely.  Maybe
someone has a better idea?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

Alvaro Herrera wrote:
> I think pg_class is a reasonable place to put more generic relkind lists
> alongside a matching error message for each, rather than specialized
> "does this relkind have storage" macros.  What about something like a
> struct list in pg_class.h,

I just noticed that this doesn't help at all with the initial problem
statement, which is that some of the relkind checks failed to notice
that partitioned tables needed to be added to the set.  Maybe it still
helps because you have something to grep for, as Tom proposed elsewhere.

However, if there are multiple places that should be kept in sync
regarding which relkinds to check, then I don't understand Robert's
objection that only one place requires the check.  Surely we're having
this discussion precisely because more than one place needs the check,
and finding those places is not obvious?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Peter Eisentraut]

On 8/2/17 13:28, Alvaro Herrera wrote:
> I think pg_class is a reasonable place to put more generic relkind lists
> alongside a matching error message for each, rather than specialized
> "does this relkind have storage" macros.  What about something like a
> struct list in pg_class.h,
> 
> {
> {
>   relkinds_r_i_T,
>   { 'r', 'i', 'T' },
>   gettext_noop("relation %s is not a table, index or toast table")
> },
> ...
> }

I don't find this style of error message optimal anyway.  If I do, for
example

ALTER TABLE someview ADD CONSTRAINT ...
ERROR: "someview" is not a table, foreign table, whatever

then this information is not helpful.  It's not like I'm going to turn
my view into a foreign table in order to be able to proceed with that
command.  The actual error, from the perspective of the user, is
something like

ERROR: "someview" is a view
DETAIL: Views cannot have constraints.

(Maybe they can.  This is an example.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Alvaro Herrera]

I think pg_class is a reasonable place to put more generic relkind lists
alongside a matching error message for each, rather than specialized
"does this relkind have storage" macros.  What about something like a
struct list in pg_class.h,

{
{
relkinds_r_i_T,
{ 'r', 'i', 'T' },
gettext_noop("relation %s is not a table, index or toast table")
},
...
}

and then in the .c checks you do something like

relkinds = relkind_r_i_T;
if (rel_doesnt_match(rel, relkinds))
ereport(ERROR, (errcode(...),
errmsg(relkinds_get_message(relkinds)));

then, in order to update the set of relkinds that some particular
operation works with, you just need to change the "relkinds" variable,
and the message is automatically up to date (you may need to add a new
entry, if there isn't one for the set you want, but the number of
permutations needed shouldn't grow too large).  This doesn't create a
problem for translators because you're not constructing an error
message, and it doesn't pollute pg_class.h with things that don't really
belong there.

One possible objection is that rel_doesnt_match() in the above
formulation is slow, because it has to scan the entire list.  Maybe this
is not a problem because the list isn't large anyway, or maybe there's
some better formulation -- for example, we could assign a distinct bit
to each relkind, and create bitmasks for each set; then figuring out
whether there's a match or not is just a matter of bit-anding.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Tom Lane]

Robert Haas  writes:
> 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.

I think the value would be in having a centralized checklist of
things-to-fix-when-adding-a-new-relkind.  There's more than one way
to reach that goal, though.  I wonder whether the task should be defined
more like "grep for 'RELKIND_' and fix every place you find that".
If there are places to touch that fail to mention that string, fix
them, using comments if nothing else.  (But see fe797b4a6 and
followon commits for other solutions.)

> I think this might cause some problems for translators.

Yeah, the error messages that list a bunch of different relkinds in text
form are going to be a hassle no matter what.  Most of the ways you might
think of to change that will violate our translatability rules.

regards, tom lane


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Robert Haas]

 On Mon, Jul 3, 2017 at 3:52 AM, Ashutosh Bapat
 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


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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

Forgot to attach the patch with the earlier mail.

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

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

On Mon, May 29, 2017 at 12:55 PM, Ashutosh Bapat
 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


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

[HACKERS] Macros bundling RELKIND_* conditions

[Ashutosh Bapat]

Hi,
We saw a handful bugs reported because RELKIND_PARTITIONED_TABLE was
not added to appropriate conditions on relkind. One such report is
[1]. On that thread I suggested that we encapsulate these conditions
as macros. Here's excerpt of my mail.

--
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.


[1] 
https://www.postgresql.org/message-id/CAKJS1f-BmGo410bh5RSPZUvOO0LhmHL2NYmdrC_Jm8pk_FfyCA%40mail.gmail.com
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


pg_relkind_macros_patches_v1.tar.gzip
Description: Binary data

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