Thread: [HACKERS] contrib modules and relkind check

Some contrib functions fail to fail sooner when relations of unsupported
relkinds are passed, resulting in error message like one below:

create table foo (a int);
create view foov as select * from foo;
select pg_visibility('foov', 0);
ERROR:  could not open file "base/13123/16488": No such file or directory

Attached patch fixes that for all such functions I could find in contrib.

It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
places (in pageinspect and pgstattuple).

Thanks,
Amit

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

On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Some contrib functions fail to fail sooner when relations of unsupported
> relkinds are passed, resulting in error message like one below:
>
> create table foo (a int);
> create view foov as select * from foo;
> select pg_visibility('foov', 0);
> ERROR:  could not open file "base/13123/16488": No such file or directory
>
> Attached patch fixes that for all such functions I could find in contrib.
>
> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
> places (in pageinspect and pgstattuple).

I have spent some time looking at your patch, and did not find any
issues with it, nor did I notice code paths that were not treated or
any other contrib modules sufferring from the same deficiencies that
you may have missed. Nice work.
-- 
Michael

On 2017/01/24 15:11, Michael Paquier wrote:
> On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Some contrib functions fail to fail sooner when relations of unsupported
>> relkinds are passed, resulting in error message like one below:
>>
>> create table foo (a int);
>> create view foov as select * from foo;
>> select pg_visibility('foov', 0);
>> ERROR:  could not open file "base/13123/16488": No such file or directory
>>
>> Attached patch fixes that for all such functions I could find in contrib.
>>
>> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
>> places (in pageinspect and pgstattuple).
> 
> I have spent some time looking at your patch, and did not find any
> issues with it, nor did I notice code paths that were not treated or
> any other contrib modules sufferring from the same deficiencies that
> you may have missed. Nice work.

Thanks for the review, Michael!

Regards,
Amit

On 2017/01/24 15:35, Amit Langote wrote:
> On 2017/01/24 15:11, Michael Paquier wrote:
>> On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> Some contrib functions fail to fail sooner when relations of unsupported
>>> relkinds are passed, resulting in error message like one below:
>>>
>>> create table foo (a int);
>>> create view foov as select * from foo;
>>> select pg_visibility('foov', 0);
>>> ERROR:  could not open file "base/13123/16488": No such file or directory
>>>
>>> Attached patch fixes that for all such functions I could find in contrib.
>>>
>>> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of
>>> places (in pageinspect and pgstattuple).
>>
>> I have spent some time looking at your patch, and did not find any
>> issues with it, nor did I notice code paths that were not treated or
>> any other contrib modules sufferring from the same deficiencies that
>> you may have missed. Nice work.
> 
> Thanks for the review, Michael!

Added to the next CF, just so someone can decide to pick it up later.

https://commitfest.postgresql.org/13/988/

Thanks,
Amit

On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey.huinker@gmail.com> wrote:
> Is this still needing a reviewer?

Useful input is always welcome.

> Code is quite clear. It does raise two questions:
>
> 1. should have these tests named in core functions, like maybe:
>
> relation_has_visibility(Relation)
> relation_has_storage(Relation)
> and/or corresponding void functions check_relation_has_visibility() and
> check_relation_has_storage() which would raise the appropriate error message
> when the boolean test fails.
> Then again, this might be overkill since we don't add new relkinds very
> often.

The visibility checks are localized in pg_visibility.c and the storage
checks in pgstatindex.c, so yes we could have macros in those files.
Or even better: just have a sanity check routine as the error messages
are the same everywhere.

> 2. Is it better stylistically to have an AND-ed list of != tests, or a
> negated list of OR-ed equality tests, and should the one style be changed to
> conform to the other?
>
> No new regression tests. I think we should create a dummy partitioned table
> for each contrib and show that it fails.

Yep, good point. That's easy enough to add.

By the way, partition tables create a file on disk but they should
not... Amit, I think you are working on a patch for that as well?
That's tweaking heap_create() to unlist partitioned tables and then be
sure that other code paths don't try to look at the parent partitioned
table on disk.
-- 
Michael

On 2017/02/10 14:32, Michael Paquier wrote:
> On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey.huinker@gmail.com> wrote:

Thanks Corey and Michael for the reviews!

>> 1. should have these tests named in core functions, like maybe:
>>
>> relation_has_visibility(Relation)
>> relation_has_storage(Relation)
>> and/or corresponding void functions check_relation_has_visibility() and
>> check_relation_has_storage() which would raise the appropriate error message
>> when the boolean test fails.
>> Then again, this might be overkill since we don't add new relkinds very
>> often.
> 
> The visibility checks are localized in pg_visibility.c and the storage
> checks in pgstatindex.c, so yes we could have macros in those files.
> Or even better: just have a sanity check routine as the error messages
> are the same everywhere.

If I understand correctly what's being proposed here, tablecmds.c has
something called ATWrongRelkindError() which sounds (kind of) similar.
It's called by ATSimplePermissions() whenever it finds out that relkind of
the table specified in a given ALTER TABLE command is not in the "allowed
relkind targets" for the command.  Different ALTER TABLE commands call
ATSimplePermissions() with an argument that specifies the "allowed relkind
targets" for each command.   I'm not saying that it should be used
elsewhere, but if we do invent a new centralized routine for relkind
checks, it would look similar.

>> 2. Is it better stylistically to have an AND-ed list of != tests, or a
>> negated list of OR-ed equality tests, and should the one style be changed to
>> conform to the other?

I changed the patch so that all newly added checks use an AND-ed list of
!= tests.

>> No new regression tests. I think we should create a dummy partitioned table
>> for each contrib and show that it fails.
> 
> Yep, good point. That's easy enough to add.

I added tests with a partitioned table to pageinspect's page.sql and
pgstattuple.sql.  I don't see any tests for pg_visibility module, maybe I
should add?

Although, I felt a bit uncomfortable the way the error message looks, for
example:

+ -- check that using any of these functions with a partitioned table
would fail
+ create table test_partitioned (a int) partition by range (a);
+ select pg_relpages('test_partitioned');
+ ERROR:  "test_partitioned" is not a table, index, materialized view,
sequence, or TOAST table

test_partitioned IS a table but just the kind without storage.  This is
not the only place however where we do not separate the check for
partitioned tables from other unsupported relkinds in respective contexts.
 Not sure if that would be a better idea.

> By the way, partition tables create a file on disk but they should
> not... Amit, I think you are working on a patch for that as well?
> That's tweaking heap_create() to unlist partitioned tables and then be
> sure that other code paths don't try to look at the parent partitioned
> table on disk.

Yep, I just sent a message titled "Partitioned tables and relfilenode".

Thanks,
Amit

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

On Fri, Feb 10, 2017 at 4:29 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/02/10 14:32, Michael Paquier wrote:
>> On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey.huinker@gmail.com> wrote:
>
> Thanks Corey and Michael for the reviews!
>
>>> 1. should have these tests named in core functions, like maybe:
>>>
>>> relation_has_visibility(Relation)
>>> relation_has_storage(Relation)
>>> and/or corresponding void functions check_relation_has_visibility() and
>>> check_relation_has_storage() which would raise the appropriate error message
>>> when the boolean test fails.
>>> Then again, this might be overkill since we don't add new relkinds very
>>> often.
>>
>> The visibility checks are localized in pg_visibility.c and the storage
>> checks in pgstatindex.c, so yes we could have macros in those files.
>> Or even better: just have a sanity check routine as the error messages
>> are the same everywhere.
>
> If I understand correctly what's being proposed here, tablecmds.c has
> something called ATWrongRelkindError() which sounds (kind of) similar.
> It's called by ATSimplePermissions() whenever it finds out that relkind of
> the table specified in a given ALTER TABLE command is not in the "allowed
> relkind targets" for the command.  Different ALTER TABLE commands call
> ATSimplePermissions() with an argument that specifies the "allowed relkind
> targets" for each command.   I'm not saying that it should be used
> elsewhere, but if we do invent a new centralized routine for relkind
> checks, it would look similar.

You did not get completely what I wrote upthread. This check is
repeated 3 times in pg_visibility.c:
+   /* Other relkinds don't have visibility info */
+   if (rel->rd_rel->relkind != RELKIND_RELATION &&
+       rel->rd_rel->relkind != RELKIND_MATVIEW &&
+       rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+       ereport(ERROR,
+               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                errmsg("\"%s\" is not a table, materialized view, or
TOAST table",
+                       RelationGetRelationName(rel))));

And this one is present 4 times in pgstatindex.c:
+   /* only the following relkinds have storage */
+   if (rel->rd_rel->relkind != RELKIND_RELATION &&
+       rel->rd_rel->relkind != RELKIND_INDEX &&
+       rel->rd_rel->relkind != RELKIND_MATVIEW &&
+       rel->rd_rel->relkind != RELKIND_SEQUENCE &&
+       rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+       ereport(ERROR,
+               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                errmsg("\"%s\" is not a table, index, materialized
view, sequence, or TOAST table",
+                       RelationGetRelationName(rel))));

So the suggestion is simply to encapsulate such blocks into their own
function like check_relation_relkind, each one locally in their
respective contrib's file. And each function includes the error
message, which should use ERRCODE_WRONG_OBJECT_TYPE as errcode by the
way.

>>> 2. Is it better stylistically to have an AND-ed list of != tests, or a
>>> negated list of OR-ed equality tests, and should the one style be changed to
>>> conform to the other?
>
> I changed the patch so that all newly added checks use an AND-ed list of
> != tests.
>
>>> No new regression tests. I think we should create a dummy partitioned table
>>> for each contrib and show that it fails.
>>
>> Yep, good point. That's easy enough to add.
>
> I added tests with a partitioned table to pageinspect's page.sql and
> pgstattuple.sql.  I don't see any tests for pg_visibility module, maybe I
> should add?

Checking those error code paths would be nice. It would be nice as
well that the relation types that are expected to be supported and
unsupported are checked. For pageinspect the check range is correct.
There are some relkinds missing in pgstatindex though.

> Although, I felt a bit uncomfortable the way the error message looks, for
> example:
>
> + -- check that using any of these functions with a partitioned table
> would fail
> + create table test_partitioned (a int) partition by range (a);
> + select pg_relpages('test_partitioned');
> + ERROR:  "test_partitioned" is not a table, index, materialized view,
> sequence, or TOAST table

Would it be a problem to mention this relkind as just "partitioned
table" in the error message.

> test_partitioned IS a table but just the kind without storage.  This is
> not the only place however where we do not separate the check for
> partitioned tables from other unsupported relkinds in respective contexts.
>  Not sure if that would be a better idea.

Well, perhaps I am playing with the words in my last paragraph, but
the documentation clearly states that any relation defined with
PARTITION BY is a "partitioned table".
-- 
Michael

On 2017/02/13 14:59, Michael Paquier wrote:
> On Fri, Feb 10, 2017 at 4:29 PM, Amit Langote wrote:
>> On 2017/02/10 14:32, Michael Paquier wrote:
>>> The visibility checks are localized in pg_visibility.c and the storage
>>> checks in pgstatindex.c, so yes we could have macros in those files.
>>> Or even better: just have a sanity check routine as the error messages
>>> are the same everywhere.
>>
>> If I understand correctly what's being proposed here, tablecmds.c has
>> something called ATWrongRelkindError() which sounds (kind of) similar.
>> It's called by ATSimplePermissions() whenever it finds out that relkind of
>> the table specified in a given ALTER TABLE command is not in the "allowed
>> relkind targets" for the command.  Different ALTER TABLE commands call
>> ATSimplePermissions() with an argument that specifies the "allowed relkind
>> targets" for each command.   I'm not saying that it should be used
>> elsewhere, but if we do invent a new centralized routine for relkind
>> checks, it would look similar.
> 
> You did not get completely what I wrote upthread. This check is
> repeated 3 times in pg_visibility.c:
> +   /* Other relkinds don't have visibility info */
> +   if (rel->rd_rel->relkind != RELKIND_RELATION &&
> +       rel->rd_rel->relkind != RELKIND_MATVIEW &&
> +       rel->rd_rel->relkind != RELKIND_TOASTVALUE)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                errmsg("\"%s\" is not a table, materialized view, or
> TOAST table",
> +                       RelationGetRelationName(rel))));
> 
> And this one is present 4 times in pgstatindex.c:
> +   /* only the following relkinds have storage */
> +   if (rel->rd_rel->relkind != RELKIND_RELATION &&
> +       rel->rd_rel->relkind != RELKIND_INDEX &&
> +       rel->rd_rel->relkind != RELKIND_MATVIEW &&
> +       rel->rd_rel->relkind != RELKIND_SEQUENCE &&
> +       rel->rd_rel->relkind != RELKIND_TOASTVALUE)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                errmsg("\"%s\" is not a table, index, materialized
> view, sequence, or TOAST table",
> +                       RelationGetRelationName(rel))));
> 
> So the suggestion is simply to encapsulate such blocks into their own
> function like check_relation_relkind, each one locally in their
> respective contrib's file. And each function includes the error
> message, which should use ERRCODE_WRONG_OBJECT_TYPE as errcode by the
> way.

I see, thanks for explaining.  Implemented that in the attached, also
fixing the errcode.  Please see if that's what you had in mind.

I was tempted for a moment to name the functions
check_relation_has_storage() with the message "relation %s has no storage"
and check_relation_has_visibility_info() with a similar message, but soon
got over it. ;)

>>>> No new regression tests. I think we should create a dummy partitioned table
>>>> for each contrib and show that it fails.
>>>
>>> Yep, good point. That's easy enough to add.
>>
>> I added tests with a partitioned table to pageinspect's page.sql and
>> pgstattuple.sql.  I don't see any tests for pg_visibility module, maybe I
>> should add?
> 
> Checking those error code paths would be nice. It would be nice as
> well that the relation types that are expected to be supported and
> unsupported are checked. For pageinspect the check range is correct.
> There are some relkinds missing in pgstatindex though.

Added more tests in pgstattuple and the new ones for pg_visibility,
although I may have overdone the latter.

>> Although, I felt a bit uncomfortable the way the error message looks, for
>> example:
>>
>> + -- check that using any of these functions with a partitioned table
>> would fail
>> + create table test_partitioned (a int) partition by range (a);
>> + select pg_relpages('test_partitioned');
>> + ERROR:  "test_partitioned" is not a table, index, materialized view,
>> sequence, or TOAST table
> 
> Would it be a problem to mention this relkind as just "partitioned
> table" in the error message.

In certain contexts where a subset of relkinds are allowed and others are
not or vice versa, partitioned tables are still referred to as "tables".
That's because we still use CREATE/DROP TABLE to create/drop them and
perhaps more to the point, their being partitioned is irrelevant.

Examples of where partitioned tables are referred to as tables:

1. The following check in get_relation_by_qualified_name():

case OBJECT_TABLE:
    if (relation->rd_rel->relkind != RELKIND_RELATION &&
        relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
        ereport(ERROR,
                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                 errmsg("\"%s\" is not a table",
                        RelationGetRelationName(relation))));

2. The following in DefineQueryRewrite() (from the rewriter's point of view):

/*
 * Verify relation is of a type that rules can sensibly be applied to.
 * Internal callers can target materialized views, but transformRuleStmt()
 * blocks them for users.  Don't mention them in the error message.
 */
if (event_relation->rd_rel->relkind != RELKIND_RELATION &&
    event_relation->rd_rel->relkind != RELKIND_MATVIEW &&
    event_relation->rd_rel->relkind != RELKIND_VIEW &&
    event_relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
             errmsg("\"%s\" is not a table or view",
                    RelationGetRelationName(event_relation))));


In other contexts, where a table's being partitioned is relevant, the
message is shown as "relation is/is not partitioned table".  Examples:

1. The following check in DefineIndex():

else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
             errmsg("cannot create index on partitioned table \"%s\"",
                    RelationGetRelationName(rel))));

2. One in get_raw_page_internal():

if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
             errmsg("cannot get raw page from partitioned table \"%s\"",
                    RelationGetRelationName(rel))));

3. On the other hand, the following check in transformAttachPartition()
prevents an attempt to attach a partition to a a regular table:

if (parentRel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
    ereport(ERROR,
            (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
             errmsg("\"%s\" is not partitioned",
                    RelationGetRelationName(parentRel))));

>> test_partitioned IS a table but just the kind without storage.  This is
>> not the only place however where we do not separate the check for
>> partitioned tables from other unsupported relkinds in respective contexts.
>>  Not sure if that would be a better idea.
> 
> Well, perhaps I am playing with the words in my last paragraph, but
> the documentation clearly states that any relation defined with
> PARTITION BY is a "partitioned table".

Yes, however as I tried to describe above, the term used in error messages
differs from context to context.  I can see that a more consistent
user-facing terminology is important irrespective of the internal
implementation details.

Updated patch attached.

Thanks,
Amit

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

On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/02/13 14:59, Michael Paquier wrote:
> I see, thanks for explaining.  Implemented that in the attached, also
> fixing the errcode.  Please see if that's what you had in mind.

Yes. That's it, the overall patch footprint is reduced.

> I was tempted for a moment to name the functions
> check_relation_has_storage() with the message "relation %s has no storage"
> and check_relation_has_visibility_info() with a similar message, but soon
> got over it. ;)

I like the format of your patch: simple and it goes to the point.

>>>>> No new regression tests. I think we should create a dummy partitioned table
>>>>> for each contrib and show that it fails.
>>>>
>>>> Yep, good point. That's easy enough to add.
>>>
>>> I added tests with a partitioned table to pageinspect's page.sql and
>>> pgstattuple.sql.  I don't see any tests for pg_visibility module, maybe I
>>> should add?
>>
>> Checking those error code paths would be nice. It would be nice as
>> well that the relation types that are expected to be supported and
>> unsupported are checked. For pageinspect the check range is correct.
>> There are some relkinds missing in pgstatindex though.
>
> Added more tests in pgstattuple and the new ones for pg_visibility,
> although I may have overdone the latter.

A bonus idea is also to add tests for relkinds that work, with for
example the creation of a table, inserting some data in it, vacuum it,
and look at "SELECT count(*) > 0 FROM pg_visibility('foo'::regclass)".

> In certain contexts where a subset of relkinds are allowed and others are
> not or vice versa, partitioned tables are still referred to as "tables".
> That's because we still use CREATE/DROP TABLE to create/drop them and
> perhaps more to the point, their being partitioned is irrelevant.
>
> Examples of where partitioned tables are referred to as tables:
>
> [...]
>
> In other contexts, where a table's being partitioned is relevant, the
> message is shown as "relation is/is not partitioned table".  Examples:
>
> [...]

Hm... It may be a good idea to be consistent on the whole system and
refer to "partitioned table" as a table without storage and used as an
entry point for partitions. The docs use this term in CREATE TABLE,
and we would finish with messages like "not a table or a partitioned
table". Extra thoughts are welcome here, the current inconsistencies
would be confusing for users.
-- 
Michael

On Tue, Mar 7, 2017 at 3:10 AM, Corey Huinker <corey.huinker@gmail.com> wrote:
> Michael, do you want to add yourself as a reviewer?

Yes, thanks for the reminder.
-- 
Michael

Sorry about the absence on this thread.

On 2017/02/14 15:30, Michael Paquier wrote:
> On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote wrote:
>>
>> Added more tests in pgstattuple and the new ones for pg_visibility,
>> although I may have overdone the latter.
> 
> A bonus idea is also to add tests for relkinds that work, with for
> example the creation of a table, inserting some data in it, vacuum it,
> and look at "SELECT count(*) > 0 FROM pg_visibility('foo'::regclass)".

I assume you meant only for pg_visibility.  Done in the attached (a pretty
basic test though).

>> In certain contexts where a subset of relkinds are allowed and others are
>> not or vice versa, partitioned tables are still referred to as "tables".
>> That's because we still use CREATE/DROP TABLE to create/drop them and
>> perhaps more to the point, their being partitioned is irrelevant.
>>
>> Examples of where partitioned tables are referred to as tables:
>>
>> [...]
>>
>> In other contexts, where a table's being partitioned is relevant, the
>> message is shown as "relation is/is not partitioned table".  Examples:
>>
>> [...]
> 
> Hm... It may be a good idea to be consistent on the whole system and
> refer to "partitioned table" as a table without storage and used as an
> entry point for partitions. The docs use this term in CREATE TABLE,
> and we would finish with messages like "not a table or a partitioned
> table". Extra thoughts are welcome here, the current inconsistencies
> would be confusing for users.

If we decide to go with some different approach, we'd not be doing it
here.  Maybe in the "partitioned tables and relfilenode" thread or a new one.

Thanks,
Amit

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

On Tue, Mar 7, 2017 at 4:15 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Sorry about the absence on this thread.

No problems! Thanks for showing up with an updated patch.

> On 2017/02/14 15:30, Michael Paquier wrote:
>> On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote wrote:
>>>
>>> Added more tests in pgstattuple and the new ones for pg_visibility,
>>> although I may have overdone the latter.
>>
>> A bonus idea is also to add tests for relkinds that work, with for
>> example the creation of a table, inserting some data in it, vacuum it,
>> and look at "SELECT count(*) > 0 FROM pg_visibility('foo'::regclass)".
>
> I assume you meant only for pg_visibility.  Done in the attached (a pretty
> basic test though).

Yep.

> If we decide to go with some different approach, we'd not be doing it
> here.  Maybe in the "partitioned tables and relfilenode" thread or a new one.

Okay.

+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -0,0 +1,85 @@
+CREATE EXTENSION pg_visibility;
+--
+-- check that using the module's functions with unsupported relations will fail
+--
[...]
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column?
+----------
+ t
+(1 row)
Only regular tables are tested as valid objects. Testing toast tables
is not worth the complication. Could you add as well a matview?

Except for this small issue the patch looks good to me.
-- 
Michael

On 2017/03/08 16:47, Michael Paquier wrote:
> On Tue, Mar 7, 2017 at 4:15 PM, Amit Langote wrote:
> +++ b/contrib/pg_visibility/expected/pg_visibility.out
> @@ -0,0 +1,85 @@
> +CREATE EXTENSION pg_visibility;
> +--
> +-- check that using the module's functions with unsupported relations will fail
> +--
> [...]
> +select count(*) > 0 from pg_visibility('regular_table');
> + ?column?
> +----------
> + t
> +(1 row)
> Only regular tables are tested as valid objects. Testing toast tables
> is not worth the complication. Could you add as well a matview?

Done in the attached updated patch.

> 
> Except for this small issue the patch looks good to me.

Thanks.

Regards,
Amit

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

On Wed, Mar 8, 2017 at 5:10 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/03/08 16:47, Michael Paquier wrote:
>> Only regular tables are tested as valid objects. Testing toast tables
>> is not worth the complication. Could you add as well a matview?
>
> Done in the attached updated patch.

+select count(*) > 0 from pg_visibility('matview');
+ ?column?
+----------
+ f
+(1 row)
That's quite a generic name :) You may want to use less common names
in your tests.

OK, I am marking that as ready for committer.
-- 
Michael

On 2017/03/09 11:51, Michael Paquier wrote:
> On Wed, Mar 8, 2017 at 5:10 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2017/03/08 16:47, Michael Paquier wrote:
>>> Only regular tables are tested as valid objects. Testing toast tables
>>> is not worth the complication. Could you add as well a matview?
>>
>> Done in the attached updated patch.
> 
> +select count(*) > 0 from pg_visibility('matview');
> + ?column?
> +----------
> + f
> +(1 row)
> That's quite a generic name :) You may want to use less common names
> in your tests.

I agree.  Updated patch attached.

> 
> OK, I am marking that as ready for committer.

Thanks.

Regards,
Amit

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

Amit, Michael,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/03/09 11:51, Michael Paquier wrote:
> > On Wed, Mar 8, 2017 at 5:10 PM, Amit Langote
> > <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> On 2017/03/08 16:47, Michael Paquier wrote:
> >>> Only regular tables are tested as valid objects. Testing toast tables
> >>> is not worth the complication. Could you add as well a matview?
> >>
> >> Done in the attached updated patch.
> >
> > +select count(*) > 0 from pg_visibility('matview');
> > + ?column?
> > +----------
> > + f
> > +(1 row)
> > That's quite a generic name :) You may want to use less common names
> > in your tests.
>
> I agree.  Updated patch attached.
>
> > OK, I am marking that as ready for committer.

I'm starting to look over this, seems pretty straight-forward.

Barring issues, I should get it committed in probably an hour or two.

Thanks!

Stephen

Amit, Michael,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/03/09 11:51, Michael Paquier wrote:
> > OK, I am marking that as ready for committer.
>
> Thanks.

Thanks for this, I've pushed this now.  I do have a few notes about
changes that I made from your patch;

- Generally speaking, the user-facing functions come first in these .c files, with a prototype at the top for the
staticfunctions defined later on in the file.  I went ahead and did that for the functions you added too. 

- I added more comments to the regression tests, in particular, we usually comment when tests are expected to fail.

- I added some additional regression tests to cover more cases, particularly ones for things that weren't being tested
atall. 

- Not the fault of your patch, but there were cases where elog() was being used when it really should have been
ereport(),so I changed those cases to all be, hopefully, consistent throughout. 

Thanks again!

Stephen

Hi Stephen,

On 2017/03/10 6:48, Stephen Frost wrote:
> Amit, Michael,
> 
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> On 2017/03/09 11:51, Michael Paquier wrote:
>>> OK, I am marking that as ready for committer.
>>
>> Thanks.
> 
> Thanks for this, I've pushed this now.  I do have a few notes about
> changes that I made from your patch;
> 
> - Generally speaking, the user-facing functions come first in these .c
>   files, with a prototype at the top for the static functions defined
>   later on in the file.  I went ahead and did that for the functions you
>   added too.
> 
> - I added more comments to the regression tests, in particular, we
>   usually comment when tests are expected to fail.
> 
> - I added some additional regression tests to cover more cases,
>   particularly ones for things that weren't being tested at all.
> 
> - Not the fault of your patch, but there were cases where elog() was
>   being used when it really should have been ereport(), so I changed
>   those cases to all be, hopefully, consistent throughout.

Thanks a lot for all the improvements and committing.

Thanks,
Amit

On Fri, Mar 10, 2017 at 8:59 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Hi Stephen,
>
> On 2017/03/10 6:48, Stephen Frost wrote:
>> Amit, Michael,
>>
>> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>>> On 2017/03/09 11:51, Michael Paquier wrote:
>>>> OK, I am marking that as ready for committer.
>>>
>>> Thanks.
>>
>> Thanks for this, I've pushed this now.  I do have a few notes about
>> changes that I made from your patch;
>>
>> - Generally speaking, the user-facing functions come first in these .c
>>   files, with a prototype at the top for the static functions defined
>>   later on in the file.  I went ahead and did that for the functions you
>>   added too.
>>
>> - I added more comments to the regression tests, in particular, we
>>   usually comment when tests are expected to fail.
>>
>> - I added some additional regression tests to cover more cases,
>>   particularly ones for things that weren't being tested at all.
>>
>> - Not the fault of your patch, but there were cases where elog() was
>>   being used when it really should have been ereport(), so I changed
>>   those cases to all be, hopefully, consistent throughout.
>
> Thanks a lot for all the improvements and committing.

Thanks. Shouldn't this fix be back-patched? pg_visibility should fail
properly for indexes and other relkinds even in 9.6. pgstattuple can
also trigger failures. It would be confusing for users to face "could
not open file" kind of errors instead of a proper error message. Note
that I am fine to produce those patches if there is a resource issue
for any of you two.

+-- an actual index of a partitiond table should work though
Typo here => s/partitiond/partitioned/
-- 
Michael

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> Thanks. Shouldn't this fix be back-patched? pg_visibility should fail
> properly for indexes and other relkinds even in 9.6. pgstattuple can
> also trigger failures. It would be confusing for users to face "could
> not open file" kind of errors instead of a proper error message. Note
> that I am fine to produce those patches if there is a resource issue
> for any of you two.

I'm not really convinced that back-patching this is worthwhile, which is
why I didn't go through the effort to do so.  A reasonable error will be
thrown in either case, after all, without any bad behavior happening,
from what I can see.

That said, if you feel strongly enough about it to propose appropriate
patches for the back-branches, I'll try and look at them before the next
set of point releases, but I'm not going to deal with them this month as
I'd like to get through as much of the CF for PG10 as we can.

> +-- an actual index of a partitiond table should work though
> Typo here => s/partitiond/partitioned/

Will fix.

Thanks!

Stephen

On Fri, Mar 10, 2017 at 9:34 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> Thanks. Shouldn't this fix be back-patched? pg_visibility should fail
>> properly for indexes and other relkinds even in 9.6. pgstattuple can
>> also trigger failures. It would be confusing for users to face "could
>> not open file" kind of errors instead of a proper error message. Note
>> that I am fine to produce those patches if there is a resource issue
>> for any of you two.
>
> I'm not really convinced that back-patching this is worthwhile, which is
> why I didn't go through the effort to do so.  A reasonable error will be
> thrown in either case, after all, without any bad behavior happening,
> from what I can see.

Okay, I don't mind as nobody has complained about pgstattuple in particular.

> That said, if you feel strongly enough about it to propose appropriate
> patches for the back-branches, I'll try and look at them before the next
> set of point releases, but I'm not going to deal with them this month as
> I'd like to get through as much of the CF for PG10 as we can.

Nah, let's see if somebody else complains.
-- 
Michael

On Thu, Mar 9, 2017 at 7:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Thanks. Shouldn't this fix be back-patched? pg_visibility should fail
> properly for indexes and other relkinds even in 9.6. pgstattuple can
> also trigger failures. It would be confusing for users to face "could
> not open file" kind of errors instead of a proper error message. Note
> that I am fine to produce those patches if there is a resource issue
> for any of you two.

I don't see a real need to back-patch this.  I mean, it probably
wouldn't break anything, but it feels more like we're raising our
standards than fixing bugs per se.  I don't think it benefits us when
users see the release notes for a new minor release series cluttered
with essentially non-critical fixes.  It makes people worry about
whether upgrading is safe.  You might brand those people as silly, but
they (indirectly) pay my mortgage.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company