Обсуждение: [HACKERS] Macros bundling RELKIND_* conditions

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

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

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

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

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

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

Robert Haas <robertmhaas@gmail.com> 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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> 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

On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>  On Mon, Jul 3, 2017 at 3:52 AM, 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.
>
> 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

On Wed, Aug 2, 2017 at 10:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> 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

On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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

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

On 08/02/2017 10:52 PM, Ashutosh Bapat wrote:
> On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> 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

Joe Conway wrote:
> On 08/02/2017 10:52 PM, Ashutosh Bapat wrote:
> > On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
> > <alvherre@2ndquadrant.com> 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2017-Aug-02, 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.

I support this idea.  Here's a proof-of-concept patch that corresponds
to one of the cases that Ashutosh was on about (specifically, the one
that uses the RELKIND_CAN_HAVE_STORAGE macro I just added).  If there
are no objections to this approach, I'm going to complete it along these
lines.

I put the new function at the bottom of heapam.c but I think it probably
needs a better place.

BTW are there other opinions on the RELKIND_HAS_STORAGE vs.
RELKIND_CAN_HAVE_STORAGE debate?  I'm inclined to change it to the
former.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2017-Aug-02, 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.

> I support this idea.  Here's a proof-of-concept patch that corresponds
> to one of the cases that Ashutosh was on about (specifically, the one
> that uses the RELKIND_CAN_HAVE_STORAGE macro I just added).  If there
> are no objections to this approach, I'm going to complete it along these
> lines.

+1

> I put the new function at the bottom of heapam.c but I think it probably
> needs a better place.

catalog/catalog.c contains some functions with roughly this kind of
knowledge, so maybe there?

Also, please declare the argument as "const char *relname"; and
shouldn't we use quotes around the relname in the messages?

> BTW are there other opinions on the RELKIND_HAS_STORAGE vs.
> RELKIND_CAN_HAVE_STORAGE debate?  I'm inclined to change it to the
> former.

I like RELKIND_HAS_STORAGE.  The other seems to imply that some rels
of a relkind have storage and others don't, which seems like a mess.
(Although maybe foreign tables would act like that?)

            regards, tom lane

On Wed, Dec 19, 2018 at 01:29:48PM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I put the new function at the bottom of heapam.c but I think it probably
>> needs a better place.
>
> catalog/catalog.c contains some functions with roughly this kind of
> knowledge, so maybe there?

+1 for the proposed wrapper.  errdetail_unsuitable_relkind() could be
used in a couple of in-core modules on top of the rest.

> Also, please declare the argument as "const char *relname"; and
> shouldn't we use quotes around the relname in the messages?

Yes, these should be quoted.

>> BTW are there other opinions on the RELKIND_HAS_STORAGE vs.
>> RELKIND_CAN_HAVE_STORAGE debate?  I'm inclined to change it to the
>> former.
>
> I like RELKIND_HAS_STORAGE.  The other seems to imply that some rels
> of a relkind have storage and others don't, which seems like a mess.
> (Although maybe foreign tables would act like that?)

RELKIND_CAN_HAVE_STORAGE implies a possibility of not having storage,
which does not sound right for this stuff.

Please note that I have a patch making use of RELKIND_CAN_HAVE_STORAGE
on my stack:
https://www.postgresql.org/message-id/20181220003140.GE27104@paquier.xyz

I'll just adapt if need be, or perhaps you will need to patch an extra
line depending on the timing ;)
--
Michael

Mmm. My mail on this topic seems to have sent to nowhere..

At Fri, 21 Dec 2018 07:50:04 +0530, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote in
<CAExHW5s+MihMrGmtrUXzR7b6DV2uminqRcgXDw_FVjmHi3VL9Q@mail.gmail.com>
> On Wed, Dec 19, 2018 at 11:37 PM Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
> 
> > On 2017-Aug-02, 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.
> >
> > I support this idea.  Here's a proof-of-concept patch that corresponds
> > to one of the cases that Ashutosh was on about (specifically, the one
> > that uses the RELKIND_CAN_HAVE_STORAGE macro I just added).  If there
> > are no objections to this approach, I'm going to complete it along these
> > lines.
> >
> > I put the new function at the bottom of heapam.c but I think it probably
> > needs a better place.
> >
> > BTW are there other opinions on the RELKIND_HAS_STORAGE vs.
> > RELKIND_CAN_HAVE_STORAGE debate?  I'm inclined to change it to the
> > former.
> >
> 
> +1 I liked the idea.

+1. as I posted to another thread [1]. 

[1] https://www.postgresql.org/message-id/20181218.145600.172055615.horiguchi.kyotaro@lab.ntt.co.jp

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2018-Dec-19, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > I support this idea.  Here's a proof-of-concept patch that corresponds
> > to one of the cases that Ashutosh was on about (specifically, the one
> > that uses the RELKIND_CAN_HAVE_STORAGE macro I just added).  If there
> > are no objections to this approach, I'm going to complete it along these
> > lines.
> 
> +1

Here's a v2 -- completed patching the other places that needed it, fixed
test expected output, and made the other changes you suggested.

It's a bit scary that none of the backend message wording changes affect
any tests.  Only contrib test files had to be patched.  (I didn't run
sepgsql tests.)

One (very small IMO) problem here is that Ashutosh started from the
desire of being able to notice when a new relkind needs to be added to
checks spread in the code.  This patch doesn't really do that; the new
"errdetail_unsuitable_relkind" can be grepped for but that doesn't
really solve it because some places don't throw errors, but just, say,
"continue".  I don't see a solution for this.

But I do see one further problem, which is that each message being
patched was listing the relkinds that are allowed and now they ain't.
It seems user-friendly to preserve that list in some way.  An example:

diff --git a/src/backend/commands/indexcmdsindex bd85099c28..7a255176df 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 437,444 **** DefineIndex(Oid relationId,
          default:
              ereport(ERROR,
                      (errcode(ERRCODE_WRONG_OBJECT_TYPE),
!                      errmsg("\"%s\" is not a table or materialized view",
!                             RelationGetRelationName(rel))));
              break;
      }
  
--- 437,447 ----
          default:
              ereport(ERROR,
                      (errcode(ERRCODE_WRONG_OBJECT_TYPE),
!                      errmsg("cannot create index on \"%s\"",
!                             RelationGetRelationName(rel)),
!                      errdetail_unsuitable_relkind(rel->rd_rel->relkind,
!                                                   RelationGetRelationName(rel)),
!                      errhint("Indexes can only be created on regular tables, materialized views and partitioned
tables.")));
              break;
      }
  

This requires inventing a new message for at least some of the patched
sites.  I'm not happy about using errhint() for that, since that's not a
hint, but I don't see anything else we could use.

(I'm not sure about using the term "regular table" for RELKIND_RELATION,
but I think using unadorned "table" doesn't cut it anymore -- we have
TOAST tables, foreign tables, and partitioned tables, neither of which
are tables for many of these checks.  If you look at DefineIndex you'll
see that we even have a separate message for foreign tables there, which
kinda proves my point.)

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