contrib modules and relkind check

Started by Amit Langotealmost 9 years ago24 messages

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

1 attachment(s)

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

Attachments:

0001-Add-relkind-checks-to-certain-contrib-modules.patchtext/x-diff; name=0001-Add-relkind-checks-to-certain-contrib-modules.patchDownload

From 9324727c58f2debb45ceeee7ecd6d620a3fe8b2a Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.
---
 contrib/pageinspect/rawpage.c         |  5 ++++
 contrib/pg_visibility/pg_visibility.c | 28 ++++++++++++++++++++++
 contrib/pgstattuple/pgstatindex.c     | 44 +++++++++++++++++++++++++++++++++++
 contrib/pgstattuple/pgstattuple.c     |  3 +++
 4 files changed, 80 insertions(+)

diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index a2ac078d40..65aae6b6f8 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -121,6 +121,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from foreign table \"%s\"",
 						RelationGetRelationName(rel))));
+	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))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..61d7224ee7 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -114,6 +114,15 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +266,16 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* 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))));
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -464,6 +483,15 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index b40669250a..e9e2dc2207 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -362,6 +362,17 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -383,6 +394,17 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -407,6 +429,17 @@ pg_relpagesbyid(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -426,6 +459,17 @@ pg_relpagesbyid_v1_5(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 06a1992bb1..b2432f43ed 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -293,6 +293,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_FOREIGN_TABLE:
 			err = "foreign table";
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			err = "partitioned table";
+			break;
 		default:
 			err = "unknown";
 			break;
-- 
2.11.0

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Amit Langote (#1)

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Michael Paquier (#2)

Re: contrib modules and relkind check

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Amit Langote (#3)

Re: contrib modules and relkind check

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

Corey Huinker

corey.huinker@gmail.com

almost 9 years ago

In reply to: Amit Langote (#4)

Re: contrib modules and relkind check

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.

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Corey Huinker (#5)

Re: contrib modules and relkind check

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Michael Paquier (#6)

1 attachment(s)

Re: contrib modules and relkind check

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

Attachments:

0001-Add-relkind-checks-to-certain-contrib-modules.patchtext/x-diff; name=0001-Add-relkind-checks-to-certain-contrib-modules.patchDownload

From 03621e49ca9ecf1546a03aeba0ffc6c15fa20c78 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.  Add a test for the same.
---
 contrib/pageinspect/expected/page.out        |  5 ++++
 contrib/pageinspect/rawpage.c                |  5 ++++
 contrib/pageinspect/sql/page.sql             |  5 ++++
 contrib/pg_visibility/pg_visibility.c        | 28 ++++++++++++++++++
 contrib/pgstattuple/expected/pgstattuple.out |  5 ++++
 contrib/pgstattuple/pgstatindex.c            | 44 ++++++++++++++++++++++++++++
 contrib/pgstattuple/pgstattuple.c            |  3 ++
 contrib/pgstattuple/sql/pgstattuple.sql      |  5 ++++
 8 files changed, 100 insertions(+)

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 13964cd878..85b183db4d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -71,3 +71,8 @@ SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 (1 row)
 
 DROP TABLE test1;
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+ERROR:  cannot get raw page from partitioned table "test_partitioned"
+drop table test_partitioned;
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 102f360c6f..1ccc3ff320 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -123,6 +123,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from foreign table \"%s\"",
 						RelationGetRelationName(rel))));
+	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))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 97eef9829a..fa3533f2af 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -28,3 +28,8 @@ SELECT tuple_data_split('test1'::regclass, t_data, t_infomask, t_infomask2, t_bi
 SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 
 DROP TABLE test1;
+
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+drop table test_partitioned;
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..61d7224ee7 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -114,6 +114,15 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +266,16 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* 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))));
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -464,6 +483,15 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 169d1932b2..be5c5b5def 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -138,3 +138,8 @@ select * from pgstathashindex('test_hashidx');
        2 |            4 |              0 |            1 |          0 |          0 |          0 |          100
 (1 row)
 
+-- 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
+drop table test_partitioned;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 17a53e3bb7..867001d8f7 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -388,6 +388,17 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -409,6 +420,17 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -433,6 +455,17 @@ pg_relpagesbyid(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -452,6 +485,17 @@ pg_relpagesbyid_v1_5(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* 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))));
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 06a1992bb1..b2432f43ed 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -293,6 +293,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_FOREIGN_TABLE:
 			err = "foreign table";
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			err = "partitioned table";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 81fd5d693b..a0726a467b 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -51,3 +51,8 @@ select * from pgstatginindex('test_ginidx');
 create index test_hashidx on test using hash (b);
 
 select * from pgstathashindex('test_hashidx');
+
+-- 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');
+drop table test_partitioned;
-- 
2.11.0

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Amit Langote (#7)

Re: contrib modules and relkind check

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

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Michael Paquier (#8)

1 attachment(s)

Re: contrib modules and relkind check

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

Attachments:

0001-Add-relkind-checks-to-certain-contrib-modules.patchtext/x-diff; name=0001-Add-relkind-checks-to-certain-contrib-modules.patchDownload

From b5868a099f46acd8f522f6e93768176526cc001b Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.

Add tests for the newly added checks.
---
 contrib/pageinspect/expected/page.out            |  5 ++
 contrib/pageinspect/rawpage.c                    |  5 ++
 contrib/pageinspect/sql/page.sql                 |  5 ++
 contrib/pg_visibility/.gitignore                 |  4 ++
 contrib/pg_visibility/Makefile                   |  2 +
 contrib/pg_visibility/expected/pg_visibility.out | 68 ++++++++++++++++++++++++
 contrib/pg_visibility/pg_visibility.c            | 44 ++++++++++-----
 contrib/pg_visibility/sql/pg_visibility.sql      | 49 +++++++++++++++++
 contrib/pgstattuple/expected/pgstattuple.out     | 29 ++++++++++
 contrib/pgstattuple/pgstatindex.c                | 30 +++++++++++
 contrib/pgstattuple/pgstattuple.c                |  3 ++
 contrib/pgstattuple/sql/pgstattuple.sql          | 24 +++++++++
 12 files changed, 254 insertions(+), 14 deletions(-)
 create mode 100644 contrib/pg_visibility/.gitignore
 create mode 100644 contrib/pg_visibility/expected/pg_visibility.out
 create mode 100644 contrib/pg_visibility/sql/pg_visibility.sql

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 13964cd878..85b183db4d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -71,3 +71,8 @@ SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 (1 row)
 
 DROP TABLE test1;
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+ERROR:  cannot get raw page from partitioned table "test_partitioned"
+drop table test_partitioned;
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 102f360c6f..1ccc3ff320 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -123,6 +123,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from foreign table \"%s\"",
 						RelationGetRelationName(rel))));
+	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))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 97eef9829a..fa3533f2af 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -28,3 +28,8 @@ SELECT tuple_data_split('test1'::regclass, t_data, t_infomask, t_infomask2, t_bi
 SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 
 DROP TABLE test1;
+
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+drop table test_partitioned;
diff --git a/contrib/pg_visibility/.gitignore b/contrib/pg_visibility/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/contrib/pg_visibility/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
index 379591a098..bc42944426 100644
--- a/contrib/pg_visibility/Makefile
+++ b/contrib/pg_visibility/Makefile
@@ -7,6 +7,8 @@ EXTENSION = pg_visibility
 DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
 PGFILEDESC = "pg_visibility - page visibility information"
 
+REGRESS = pg_visibility
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
new file mode 100644
index 0000000000..7aadf24a8b
--- /dev/null
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -0,0 +1,68 @@
+CREATE EXTENSION pg_visibility;
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+drop table test_partitioned, test_partition;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..2ecfa3c7d0 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -95,6 +95,22 @@ pg_visibility_map(PG_FUNCTION_ARGS)
 }
 
 /*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
+/*
  * Visibility map information for a single block of a relation, plus the
  * page-level information for the same block.
  */
@@ -114,6 +130,9 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +276,10 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -369,13 +392,8 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessExclusiveLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	RelationOpenSmgr(rel);
 	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
@@ -464,6 +482,9 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
@@ -543,13 +564,8 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
 
 	rel = relation_open(relid, AccessShareLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	nblocks = RelationGetNumberOfBlocks(rel);
 
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
new file mode 100644
index 0000000000..2e6c008c39
--- /dev/null
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -0,0 +1,49 @@
+CREATE EXTENSION pg_visibility;
+
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+select pg_visibility_map('test_partitioned');
+select pg_visibility_map_summary('test_partitioned');
+select pg_check_frozen('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned');
+
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+select pg_visibility_map('test_index');
+select pg_visibility_map_summary('test_index');
+select pg_check_frozen('test_index');
+select pg_truncate_visibility_map('test_index');
+
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+select pg_visibility_map('test_view');
+select pg_visibility_map_summary('test_view');
+select pg_check_frozen('test_view');
+select pg_truncate_visibility_map('test_view');
+
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+select pg_visibility_map('test_sequence');
+select pg_visibility_map_summary('test_sequence');
+select pg_check_frozen('test_sequence');
+select pg_truncate_visibility_map('test_sequence');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+select pg_visibility_map('test_foreign_table');
+select pg_visibility_map_summary('test_foreign_table');
+select pg_check_frozen('test_foreign_table');
+select pg_truncate_visibility_map('test_foreign_table');
+
+drop table test_partitioned, test_partition;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 169d1932b2..dfd15b0c63 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -138,3 +138,32 @@ select * from pgstathashindex('test_hashidx');
        2 |            4 |              0 |            1 |          0 |          0 |          0 |          100
 (1 row)
 
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+ERROR:  "test_partitioned" (partitioned table) is not supported
+select pgstattuple_approx('test_partitioned');
+ERROR:  "test_partitioned" is not a table or materialized view
+select pg_relpages('test_partitioned');
+ERROR:  "test_partitioned" is not a table, index, materialized view, sequence, or TOAST table
+create view test_view as select 1;
+select pgstattuple('test_view');
+ERROR:  "test_view" (view) is not supported
+select pgstattuple_approx('test_view');
+ERROR:  "test_view" is not a table or materialized view
+select pg_relpages('test_view');
+ERROR:  "test_view" is not a table, index, materialized view, sequence, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+ERROR:  "test_foreign_table" (foreign table) is not supported
+select pgstattuple_approx('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table or materialized view
+select pg_relpages('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, index, materialized view, sequence, or TOAST table
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 17a53e3bb7..04b20915f8 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -361,6 +361,24 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
 	return result;
 }
 
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	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_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
 /* --------------------------------------------------------
  * pg_relpages()
  *
@@ -388,6 +406,9 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -409,6 +430,9 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -433,6 +457,9 @@ pg_relpagesbyid(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -452,6 +479,9 @@ pg_relpagesbyid_v1_5(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 06a1992bb1..b2432f43ed 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -293,6 +293,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_FOREIGN_TABLE:
 			err = "foreign table";
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			err = "partitioned table";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 81fd5d693b..e42ac5db95 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -51,3 +51,27 @@ select * from pgstatginindex('test_ginidx');
 create index test_hashidx on test using hash (b);
 
 select * from pgstathashindex('test_hashidx');
+
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+select pgstattuple_approx('test_partitioned');
+select pg_relpages('test_partitioned');
+
+create view test_view as select 1;
+select pgstattuple('test_view');
+select pgstattuple_approx('test_view');
+select pg_relpages('test_view');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+select pgstattuple_approx('test_foreign_table');
+select pg_relpages('test_foreign_table');
+
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
-- 
2.11.0

#10

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Amit Langote (#9)

Re: contrib modules and relkind check

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

#11

Corey Huinker

corey.huinker@gmail.com

almost 9 years ago

In reply to: Michael Paquier (#10)

Re: contrib modules and relkind check

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?

#12

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Corey Huinker (#11)

Re: contrib modules and relkind check

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

#13

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Michael Paquier (#10)

1 attachment(s)

Re: contrib modules and relkind check

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

Attachments:

0001-Add-relkind-checks-to-certain-contrib-modules.patchtext/x-diff; name=0001-Add-relkind-checks-to-certain-contrib-modules.patchDownload

From c7762d5bfd9363150cdb94030a47cef252697672 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.

Add tests for the newly added checks.
---
 contrib/pageinspect/expected/page.out            |  5 ++
 contrib/pageinspect/rawpage.c                    |  5 ++
 contrib/pageinspect/sql/page.sql                 |  5 ++
 contrib/pg_visibility/.gitignore                 |  4 ++
 contrib/pg_visibility/Makefile                   |  2 +
 contrib/pg_visibility/expected/pg_visibility.out | 85 ++++++++++++++++++++++++
 contrib/pg_visibility/pg_visibility.c            | 44 ++++++++----
 contrib/pg_visibility/sql/pg_visibility.sql      | 57 ++++++++++++++++
 contrib/pgstattuple/expected/pgstattuple.out     | 29 ++++++++
 contrib/pgstattuple/pgstatindex.c                | 30 +++++++++
 contrib/pgstattuple/pgstattuple.c                |  3 +
 contrib/pgstattuple/sql/pgstattuple.sql          | 24 +++++++
 12 files changed, 279 insertions(+), 14 deletions(-)
 create mode 100644 contrib/pg_visibility/.gitignore
 create mode 100644 contrib/pg_visibility/expected/pg_visibility.out
 create mode 100644 contrib/pg_visibility/sql/pg_visibility.sql

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 13964cd878..85b183db4d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -71,3 +71,8 @@ SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 (1 row)
 
 DROP TABLE test1;
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+ERROR:  cannot get raw page from partitioned table "test_partitioned"
+drop table test_partitioned;
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 102f360c6f..1ccc3ff320 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -123,6 +123,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from foreign table \"%s\"",
 						RelationGetRelationName(rel))));
+	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))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 97eef9829a..fa3533f2af 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -28,3 +28,8 @@ SELECT tuple_data_split('test1'::regclass, t_data, t_infomask, t_infomask2, t_bi
 SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 
 DROP TABLE test1;
+
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+drop table test_partitioned;
diff --git a/contrib/pg_visibility/.gitignore b/contrib/pg_visibility/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/contrib/pg_visibility/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
index 379591a098..bc42944426 100644
--- a/contrib/pg_visibility/Makefile
+++ b/contrib/pg_visibility/Makefile
@@ -7,6 +7,8 @@ EXTENSION = pg_visibility
 DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
 PGFILEDESC = "pg_visibility - page visibility information"
 
+REGRESS = pg_visibility
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
new file mode 100644
index 0000000000..ba74fc5edf
--- /dev/null
+++ 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
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+create table regular_table (a int);
+insert into regular_table values (1), (2);
+vacuum regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column? 
+----------
+ t
+(1 row)
+
+truncate regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column? 
+----------
+ f
+(1 row)
+
+drop table test_partitioned;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
+drop table regular_table;
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..2ecfa3c7d0 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -95,6 +95,22 @@ pg_visibility_map(PG_FUNCTION_ARGS)
 }
 
 /*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
+/*
  * Visibility map information for a single block of a relation, plus the
  * page-level information for the same block.
  */
@@ -114,6 +130,9 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +276,10 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -369,13 +392,8 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessExclusiveLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	RelationOpenSmgr(rel);
 	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
@@ -464,6 +482,9 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
@@ -543,13 +564,8 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
 
 	rel = relation_open(relid, AccessShareLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	nblocks = RelationGetNumberOfBlocks(rel);
 
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
new file mode 100644
index 0000000000..3c7e11fa97
--- /dev/null
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -0,0 +1,57 @@
+CREATE EXTENSION pg_visibility;
+
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+select pg_visibility_map('test_partitioned');
+select pg_visibility_map_summary('test_partitioned');
+select pg_check_frozen('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned');
+
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+select pg_visibility_map('test_index');
+select pg_visibility_map_summary('test_index');
+select pg_check_frozen('test_index');
+select pg_truncate_visibility_map('test_index');
+
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+select pg_visibility_map('test_view');
+select pg_visibility_map_summary('test_view');
+select pg_check_frozen('test_view');
+select pg_truncate_visibility_map('test_view');
+
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+select pg_visibility_map('test_sequence');
+select pg_visibility_map_summary('test_sequence');
+select pg_check_frozen('test_sequence');
+select pg_truncate_visibility_map('test_sequence');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+select pg_visibility_map('test_foreign_table');
+select pg_visibility_map_summary('test_foreign_table');
+select pg_check_frozen('test_foreign_table');
+select pg_truncate_visibility_map('test_foreign_table');
+
+create table regular_table (a int);
+insert into regular_table values (1), (2);
+vacuum regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+truncate regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+
+drop table test_partitioned;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
+drop table regular_table;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 169d1932b2..dfd15b0c63 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -138,3 +138,32 @@ select * from pgstathashindex('test_hashidx');
        2 |            4 |              0 |            1 |          0 |          0 |          0 |          100
 (1 row)
 
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+ERROR:  "test_partitioned" (partitioned table) is not supported
+select pgstattuple_approx('test_partitioned');
+ERROR:  "test_partitioned" is not a table or materialized view
+select pg_relpages('test_partitioned');
+ERROR:  "test_partitioned" is not a table, index, materialized view, sequence, or TOAST table
+create view test_view as select 1;
+select pgstattuple('test_view');
+ERROR:  "test_view" (view) is not supported
+select pgstattuple_approx('test_view');
+ERROR:  "test_view" is not a table or materialized view
+select pg_relpages('test_view');
+ERROR:  "test_view" is not a table, index, materialized view, sequence, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+ERROR:  "test_foreign_table" (foreign table) is not supported
+select pgstattuple_approx('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table or materialized view
+select pg_relpages('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, index, materialized view, sequence, or TOAST table
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 17a53e3bb7..04b20915f8 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -361,6 +361,24 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
 	return result;
 }
 
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	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_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
 /* --------------------------------------------------------
  * pg_relpages()
  *
@@ -388,6 +406,9 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -409,6 +430,9 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -433,6 +457,9 @@ pg_relpagesbyid(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -452,6 +479,9 @@ pg_relpagesbyid_v1_5(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 06a1992bb1..b2432f43ed 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -293,6 +293,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_FOREIGN_TABLE:
 			err = "foreign table";
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			err = "partitioned table";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 81fd5d693b..e42ac5db95 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -51,3 +51,27 @@ select * from pgstatginindex('test_ginidx');
 create index test_hashidx on test using hash (b);
 
 select * from pgstathashindex('test_hashidx');
+
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+select pgstattuple_approx('test_partitioned');
+select pg_relpages('test_partitioned');
+
+create view test_view as select 1;
+select pgstattuple('test_view');
+select pgstattuple_approx('test_view');
+select pg_relpages('test_view');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+select pgstattuple_approx('test_foreign_table');
+select pg_relpages('test_foreign_table');
+
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
-- 
2.11.0

#14

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Amit Langote (#13)

Re: contrib modules and relkind check

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

#15

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Michael Paquier (#14)

1 attachment(s)

Re: contrib modules and relkind check

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

Attachments:

0001-Add-relkind-checks-to-certain-contrib-modules.patchtext/x-diff; name=0001-Add-relkind-checks-to-certain-contrib-modules.patchDownload

From 9679618d65b00d65effdd5de019410998e10b3d9 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.

Add tests for the newly added checks.
---
 contrib/pageinspect/expected/page.out            |   5 ++
 contrib/pageinspect/rawpage.c                    |   5 ++
 contrib/pageinspect/sql/page.sql                 |   5 ++
 contrib/pg_visibility/.gitignore                 |   4 +
 contrib/pg_visibility/Makefile                   |   2 +
 contrib/pg_visibility/expected/pg_visibility.out | 103 +++++++++++++++++++++++
 contrib/pg_visibility/pg_visibility.c            |  44 +++++++---
 contrib/pg_visibility/sql/pg_visibility.sql      |  66 +++++++++++++++
 contrib/pgstattuple/expected/pgstattuple.out     |  29 +++++++
 contrib/pgstattuple/pgstatindex.c                |  30 +++++++
 contrib/pgstattuple/pgstattuple.c                |   3 +
 contrib/pgstattuple/sql/pgstattuple.sql          |  24 ++++++
 12 files changed, 306 insertions(+), 14 deletions(-)
 create mode 100644 contrib/pg_visibility/.gitignore
 create mode 100644 contrib/pg_visibility/expected/pg_visibility.out
 create mode 100644 contrib/pg_visibility/sql/pg_visibility.sql

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 13964cd878..85b183db4d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -71,3 +71,8 @@ SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 (1 row)
 
 DROP TABLE test1;
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+ERROR:  cannot get raw page from partitioned table "test_partitioned"
+drop table test_partitioned;
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 102f360c6f..1ccc3ff320 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -123,6 +123,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from foreign table \"%s\"",
 						RelationGetRelationName(rel))));
+	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))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 97eef9829a..fa3533f2af 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -28,3 +28,8 @@ SELECT tuple_data_split('test1'::regclass, t_data, t_infomask, t_infomask2, t_bi
 SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 
 DROP TABLE test1;
+
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+drop table test_partitioned;
diff --git a/contrib/pg_visibility/.gitignore b/contrib/pg_visibility/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/contrib/pg_visibility/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
index 379591a098..bc42944426 100644
--- a/contrib/pg_visibility/Makefile
+++ b/contrib/pg_visibility/Makefile
@@ -7,6 +7,8 @@ EXTENSION = pg_visibility
 DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
 PGFILEDESC = "pg_visibility - page visibility information"
 
+REGRESS = pg_visibility
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
new file mode 100644
index 0000000000..699d2131b9
--- /dev/null
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -0,0 +1,103 @@
+CREATE EXTENSION pg_visibility;
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+/* check some of the allowed relkinds */
+create table regular_table (a int);
+insert into regular_table values (1), (2);
+vacuum regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column? 
+----------
+ t
+(1 row)
+
+truncate regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column? 
+----------
+ f
+(1 row)
+
+create materialized view matview as select * from regular_table;
+vacuum matview;
+select count(*) > 0 from pg_visibility('matview');
+ ?column? 
+----------
+ f
+(1 row)
+
+insert into regular_table values (1), (2);
+refresh materialized view matview;
+select count(*) > 0 from pg_visibility('matview');
+ ?column? 
+----------
+ t
+(1 row)
+
+drop table test_partitioned;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
+drop materialized view matview;
+drop table regular_table;
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..2ecfa3c7d0 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -95,6 +95,22 @@ pg_visibility_map(PG_FUNCTION_ARGS)
 }
 
 /*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
+/*
  * Visibility map information for a single block of a relation, plus the
  * page-level information for the same block.
  */
@@ -114,6 +130,9 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +276,10 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -369,13 +392,8 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessExclusiveLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	RelationOpenSmgr(rel);
 	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
@@ -464,6 +482,9 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
@@ -543,13 +564,8 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
 
 	rel = relation_open(relid, AccessShareLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	nblocks = RelationGetNumberOfBlocks(rel);
 
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
new file mode 100644
index 0000000000..735854e71c
--- /dev/null
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -0,0 +1,66 @@
+CREATE EXTENSION pg_visibility;
+
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+select pg_visibility_map('test_partitioned');
+select pg_visibility_map_summary('test_partitioned');
+select pg_check_frozen('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned');
+
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+select pg_visibility_map('test_index');
+select pg_visibility_map_summary('test_index');
+select pg_check_frozen('test_index');
+select pg_truncate_visibility_map('test_index');
+
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+select pg_visibility_map('test_view');
+select pg_visibility_map_summary('test_view');
+select pg_check_frozen('test_view');
+select pg_truncate_visibility_map('test_view');
+
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+select pg_visibility_map('test_sequence');
+select pg_visibility_map_summary('test_sequence');
+select pg_check_frozen('test_sequence');
+select pg_truncate_visibility_map('test_sequence');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+select pg_visibility_map('test_foreign_table');
+select pg_visibility_map_summary('test_foreign_table');
+select pg_check_frozen('test_foreign_table');
+select pg_truncate_visibility_map('test_foreign_table');
+
+/* check some of the allowed relkinds */
+create table regular_table (a int);
+insert into regular_table values (1), (2);
+vacuum regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+truncate regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+
+create materialized view matview as select * from regular_table;
+vacuum matview;
+select count(*) > 0 from pg_visibility('matview');
+insert into regular_table values (1), (2);
+refresh materialized view matview;
+select count(*) > 0 from pg_visibility('matview');
+
+drop table test_partitioned;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
+drop materialized view matview;
+drop table regular_table;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 169d1932b2..dfd15b0c63 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -138,3 +138,32 @@ select * from pgstathashindex('test_hashidx');
        2 |            4 |              0 |            1 |          0 |          0 |          0 |          100
 (1 row)
 
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+ERROR:  "test_partitioned" (partitioned table) is not supported
+select pgstattuple_approx('test_partitioned');
+ERROR:  "test_partitioned" is not a table or materialized view
+select pg_relpages('test_partitioned');
+ERROR:  "test_partitioned" is not a table, index, materialized view, sequence, or TOAST table
+create view test_view as select 1;
+select pgstattuple('test_view');
+ERROR:  "test_view" (view) is not supported
+select pgstattuple_approx('test_view');
+ERROR:  "test_view" is not a table or materialized view
+select pg_relpages('test_view');
+ERROR:  "test_view" is not a table, index, materialized view, sequence, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+ERROR:  "test_foreign_table" (foreign table) is not supported
+select pgstattuple_approx('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table or materialized view
+select pg_relpages('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, index, materialized view, sequence, or TOAST table
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 17a53e3bb7..04b20915f8 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -361,6 +361,24 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
 	return result;
 }
 
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	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_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
 /* --------------------------------------------------------
  * pg_relpages()
  *
@@ -388,6 +406,9 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -409,6 +430,9 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -433,6 +457,9 @@ pg_relpagesbyid(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -452,6 +479,9 @@ pg_relpagesbyid_v1_5(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 06a1992bb1..b2432f43ed 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -293,6 +293,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_FOREIGN_TABLE:
 			err = "foreign table";
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			err = "partitioned table";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 81fd5d693b..e42ac5db95 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -51,3 +51,27 @@ select * from pgstatginindex('test_ginidx');
 create index test_hashidx on test using hash (b);
 
 select * from pgstathashindex('test_hashidx');
+
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+select pgstattuple_approx('test_partitioned');
+select pg_relpages('test_partitioned');
+
+create view test_view as select 1;
+select pgstattuple('test_view');
+select pgstattuple_approx('test_view');
+select pg_relpages('test_view');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+select pgstattuple_approx('test_foreign_table');
+select pg_relpages('test_foreign_table');
+
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
-- 
2.11.0

#16

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Amit Langote (#15)

Re: contrib modules and relkind check

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

#17

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Michael Paquier (#16)

1 attachment(s)

Re: contrib modules and relkind check

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

Attachments:

0001-Add-relkind-checks-to-certain-contrib-modules.patchtext/x-diff; name=0001-Add-relkind-checks-to-certain-contrib-modules.patchDownload

From 76c0c1903442ef41bd4a6e15f5b1672d4464c90c Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Tue, 24 Jan 2017 13:22:17 +0900
Subject: [PATCH] Add relkind checks to certain contrib modules

Contrib functions such a pg_visibility, pg_relpages are only applicable
to certain relkinds; error out if otherwise.

Also, RELKIND_PARTITIONED_TABLE was not added to the pageinspect's and
pgstattuple's list of unsupported relkinds.

Add tests for the newly added checks.
---
 contrib/pageinspect/expected/page.out            |   5 ++
 contrib/pageinspect/rawpage.c                    |   5 ++
 contrib/pageinspect/sql/page.sql                 |   5 ++
 contrib/pg_visibility/.gitignore                 |   4 +
 contrib/pg_visibility/Makefile                   |   2 +
 contrib/pg_visibility/expected/pg_visibility.out | 103 +++++++++++++++++++++++
 contrib/pg_visibility/pg_visibility.c            |  44 +++++++---
 contrib/pg_visibility/sql/pg_visibility.sql      |  66 +++++++++++++++
 contrib/pgstattuple/expected/pgstattuple.out     |  29 +++++++
 contrib/pgstattuple/pgstatindex.c                |  30 +++++++
 contrib/pgstattuple/pgstattuple.c                |   3 +
 contrib/pgstattuple/sql/pgstattuple.sql          |  24 ++++++
 12 files changed, 306 insertions(+), 14 deletions(-)
 create mode 100644 contrib/pg_visibility/.gitignore
 create mode 100644 contrib/pg_visibility/expected/pg_visibility.out
 create mode 100644 contrib/pg_visibility/sql/pg_visibility.sql

diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 13964cd878..85b183db4d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -71,3 +71,8 @@ SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 (1 row)
 
 DROP TABLE test1;
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+ERROR:  cannot get raw page from partitioned table "test_partitioned"
+drop table test_partitioned;
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 102f360c6f..1ccc3ff320 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -123,6 +123,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from foreign table \"%s\"",
 						RelationGetRelationName(rel))));
+	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))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 97eef9829a..fa3533f2af 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -28,3 +28,8 @@ SELECT tuple_data_split('test1'::regclass, t_data, t_infomask, t_infomask2, t_bi
 SELECT * FROM fsm_page_contents(get_raw_page('test1', 'fsm', 0));
 
 DROP TABLE test1;
+
+-- check that using any of these functions with a partitioned table would fail
+create table test_partitioned (a int) partition by range (a);
+select get_raw_page('test_partitioned', 0);
+drop table test_partitioned;
diff --git a/contrib/pg_visibility/.gitignore b/contrib/pg_visibility/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/contrib/pg_visibility/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
index 379591a098..bc42944426 100644
--- a/contrib/pg_visibility/Makefile
+++ b/contrib/pg_visibility/Makefile
@@ -7,6 +7,8 @@ EXTENSION = pg_visibility
 DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
 PGFILEDESC = "pg_visibility - page visibility information"
 
+REGRESS = pg_visibility
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
new file mode 100644
index 0000000000..4496fe36f8
--- /dev/null
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -0,0 +1,103 @@
+CREATE EXTENSION pg_visibility;
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_partitioned');
+ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_index');
+ERROR:  "test_index" is not a table, materialized view, or TOAST table
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_view');
+ERROR:  "test_view" is not a table, materialized view, or TOAST table
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_sequence');
+ERROR:  "test_sequence" is not a table, materialized view, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_visibility_map_summary('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_check_frozen('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+select pg_truncate_visibility_map('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, materialized view, or TOAST table
+/* check some of the allowed relkinds */
+create table regular_table (a int);
+insert into regular_table values (1), (2);
+vacuum regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column? 
+----------
+ t
+(1 row)
+
+truncate regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column? 
+----------
+ f
+(1 row)
+
+create materialized view matview_visibility_test as select * from regular_table;
+vacuum matview_visibility_test;
+select count(*) > 0 from pg_visibility('matview_visibility_test');
+ ?column? 
+----------
+ f
+(1 row)
+
+insert into regular_table values (1), (2);
+refresh materialized view matview_visibility_test;
+select count(*) > 0 from pg_visibility('matview_visibility_test');
+ ?column? 
+----------
+ t
+(1 row)
+
+drop table test_partitioned;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
+drop materialized view matview_visibility_test;
+drop table regular_table;
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5580637870..2ecfa3c7d0 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -95,6 +95,22 @@ pg_visibility_map(PG_FUNCTION_ARGS)
 }
 
 /*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	if (rel->rd_rel->relkind != RELKIND_RELATION &&
+		rel->rd_rel->relkind != RELKIND_MATVIEW &&
+		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, materialized view, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
+/*
  * Visibility map information for a single block of a relation, plus the
  * page-level information for the same block.
  */
@@ -114,6 +130,9 @@ pg_visibility(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	if (blkno < 0 || blkno > MaxBlockNumber)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -257,6 +276,10 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	bool		nulls[2];
 
 	rel = relation_open(relid, AccessShareLock);
+
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 
 	for (blkno = 0; blkno < nblocks; ++blkno)
@@ -369,13 +392,8 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessExclusiveLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	RelationOpenSmgr(rel);
 	rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
@@ -464,6 +482,9 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) +nblocks);
 	info->next = 0;
@@ -543,13 +564,8 @@ collect_corrupt_items(Oid relid, bool all_visible, bool all_frozen)
 
 	rel = relation_open(relid, AccessShareLock);
 
-	if (rel->rd_rel->relkind != RELKIND_RELATION &&
-		rel->rd_rel->relkind != RELKIND_MATVIEW &&
-		rel->rd_rel->relkind != RELKIND_TOASTVALUE)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-		   errmsg("\"%s\" is not a table, materialized view, or TOAST table",
-				  RelationGetRelationName(rel))));
+	/* Only some relkind have the visibility info */
+	check_relation_relkind(rel);
 
 	nblocks = RelationGetNumberOfBlocks(rel);
 
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
new file mode 100644
index 0000000000..9f481d23f7
--- /dev/null
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -0,0 +1,66 @@
+CREATE EXTENSION pg_visibility;
+
+--
+-- check that using the module's functions with unsupported relations will fail
+--
+create table test_partitioned (a int) partition by list (a);
+select pg_visibility('test_partitioned', 0);
+select pg_visibility_map('test_partitioned');
+select pg_visibility_map_summary('test_partitioned');
+select pg_check_frozen('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned');
+
+create table test_partition partition of test_partitioned for values in (1);
+create index test_index on test_partition (a);
+select pg_visibility('test_index', 0);
+select pg_visibility_map('test_index');
+select pg_visibility_map_summary('test_index');
+select pg_check_frozen('test_index');
+select pg_truncate_visibility_map('test_index');
+
+create view test_view as select 1;
+select pg_visibility('test_view', 0);
+select pg_visibility_map('test_view');
+select pg_visibility_map_summary('test_view');
+select pg_check_frozen('test_view');
+select pg_truncate_visibility_map('test_view');
+
+create sequence test_sequence;
+select pg_visibility('test_sequence', 0);
+select pg_visibility_map('test_sequence');
+select pg_visibility_map_summary('test_sequence');
+select pg_check_frozen('test_sequence');
+select pg_truncate_visibility_map('test_sequence');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pg_visibility('test_foreign_table', 0);
+select pg_visibility_map('test_foreign_table');
+select pg_visibility_map_summary('test_foreign_table');
+select pg_check_frozen('test_foreign_table');
+select pg_truncate_visibility_map('test_foreign_table');
+
+/* check some of the allowed relkinds */
+create table regular_table (a int);
+insert into regular_table values (1), (2);
+vacuum regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+truncate regular_table;
+select count(*) > 0 from pg_visibility('regular_table');
+
+create materialized view matview_visibility_test as select * from regular_table;
+vacuum matview_visibility_test;
+select count(*) > 0 from pg_visibility('matview_visibility_test');
+insert into regular_table values (1), (2);
+refresh materialized view matview_visibility_test;
+select count(*) > 0 from pg_visibility('matview_visibility_test');
+
+drop table test_partitioned;
+drop view test_view;
+drop sequence test_sequence;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
+drop materialized view matview_visibility_test;
+drop table regular_table;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 169d1932b2..dfd15b0c63 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -138,3 +138,32 @@ select * from pgstathashindex('test_hashidx');
        2 |            4 |              0 |            1 |          0 |          0 |          0 |          100
 (1 row)
 
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+ERROR:  "test_partitioned" (partitioned table) is not supported
+select pgstattuple_approx('test_partitioned');
+ERROR:  "test_partitioned" is not a table or materialized view
+select pg_relpages('test_partitioned');
+ERROR:  "test_partitioned" is not a table, index, materialized view, sequence, or TOAST table
+create view test_view as select 1;
+select pgstattuple('test_view');
+ERROR:  "test_view" (view) is not supported
+select pgstattuple_approx('test_view');
+ERROR:  "test_view" is not a table or materialized view
+select pg_relpages('test_view');
+ERROR:  "test_view" is not a table, index, materialized view, sequence, or TOAST table
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+ERROR:  "test_foreign_table" (foreign table) is not supported
+select pgstattuple_approx('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table or materialized view
+select pg_relpages('test_foreign_table');
+ERROR:  "test_foreign_table" is not a table, index, materialized view, sequence, or TOAST table
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 17a53e3bb7..04b20915f8 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -361,6 +361,24 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
 	return result;
 }
 
+/*
+ * check_relation_relkind - convenience routine to check that relation
+ * is of the relkind supported by the callers
+ */
+static void
+check_relation_relkind(Relation rel)
+{
+	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_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table, index, materialized view, sequence, or TOAST table",
+						RelationGetRelationName(rel))));
+}
+
 /* --------------------------------------------------------
  * pg_relpages()
  *
@@ -388,6 +406,9 @@ pg_relpages(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -409,6 +430,9 @@ pg_relpages_v1_5(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -433,6 +457,9 @@ pg_relpagesbyid(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
@@ -452,6 +479,9 @@ pg_relpagesbyid_v1_5(PG_FUNCTION_ARGS)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* only some relkinds have storage */
+	check_relation_relkind(rel);
+
 	/* note: this will work OK on non-local temp tables */
 
 	relpages = RelationGetNumberOfBlocks(rel);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 06a1992bb1..b2432f43ed 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -293,6 +293,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_FOREIGN_TABLE:
 			err = "foreign table";
 			break;
+		case RELKIND_PARTITIONED_TABLE:
+			err = "partitioned table";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 81fd5d693b..e42ac5db95 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -51,3 +51,27 @@ select * from pgstatginindex('test_ginidx');
 create index test_hashidx on test using hash (b);
 
 select * from pgstathashindex('test_hashidx');
+
+-- check that using any of these functions with unsupported relations will fail
+create table test_partitioned (a int) partition by range (a);
+select pgstattuple('test_partitioned');
+select pgstattuple_approx('test_partitioned');
+select pg_relpages('test_partitioned');
+
+create view test_view as select 1;
+select pgstattuple('test_view');
+select pgstattuple_approx('test_view');
+select pg_relpages('test_view');
+
+create foreign data wrapper dummy;
+create server dummy_server foreign data wrapper dummy;
+create foreign table test_foreign_table () server dummy_server;
+select pgstattuple('test_foreign_table');
+select pgstattuple_approx('test_foreign_table');
+select pg_relpages('test_foreign_table');
+
+drop table test_partitioned;
+drop view test_view;
+drop foreign table test_foreign_table;
+drop server dummy_server;
+drop foreign data wrapper dummy;
-- 
2.11.0

#18

Stephen Frost

sfrost@snowman.net

almost 9 years ago

In reply to: Amit Langote (#17)

Re: contrib modules and relkind check

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

#19

Stephen Frost

sfrost@snowman.net

almost 9 years ago

In reply to: Amit Langote (#17)

Re: contrib modules and relkind check

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

#20

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

almost 9 years ago

In reply to: Stephen Frost (#19)

Re: contrib modules and relkind check

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

#21

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Amit Langote (#20)

Re: contrib modules and relkind check

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

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

#22

Stephen Frost

sfrost@snowman.net

almost 9 years ago

In reply to: Michael Paquier (#21)

Re: contrib modules and relkind check

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

#23

Michael Paquier

michael.paquier@gmail.com

almost 9 years ago

In reply to: Stephen Frost (#22)

Re: contrib modules and relkind check

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

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

#24

Robert Haas

robertmhaas@gmail.com

almost 9 years ago

In reply to: Michael Paquier (#21)

Re: contrib modules and relkind check

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

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