contrib modules and relkind check

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

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

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

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

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

--
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, Feb 6, 2017 at 4:01 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
wrote:

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

Is this still needing a reviewer? If so, here it goes:

Patch applies.

make check-pgstattuple-recurse, check-pg_visibility-recurse,
check-pageinspect-recurse
all pass.

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.

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.

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

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

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

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

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

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

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

--
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 1:30 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

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.

Updated CF status to "Waiting on Author". Waiting, mostly for the
regression tests suggested.

Michael, do you want to add yourself as a reviewer?

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

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

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

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

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

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

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

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

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

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

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