On 2017/02/10 14:32, Michael Paquier wrote:
> On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey.huin...@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
>From 03621e49ca9ecf1546a03aeba0ffc6c15fa20c78 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
