Local partitioned indexes and pageinspect

Started by Peter Geogheganover 7 years ago12 messages

pg@bowt.ie

over 7 years ago

It looks like local partitioned indexes are a problem for pageinspect:

pg@regression[28736]=# select bt_metap('at_partitioned_b_idx');
ERROR: relation "at_partitioned_b_idx" is not a btree index

I gather that pageinspect simply didn't get the memo about the new
RELKIND_PARTITIONED_INDEX "placeholder" relkind. I think that it
should at least give a friendlier error message than what we see here.
The same applies to amcheck, and possibly a few other modules.

I also noticed that the new 'I' relkind is not documented within the
pg_class docs. I think that that needs to be updated. The docs should
explain that 'I' relations are not backed by storage, since that will
probably affect users of one or two external tools.

--
Peter Geoghegan

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Peter Geoghegan (#1)

1 attachment(s)

Re: Local partitioned indexes and pageinspect

On Mon, Apr 23, 2018 at 05:29:59PM -0700, Peter Geoghegan wrote:

It looks like local partitioned indexes are a problem for pageinspect:

pg@regression[28736]=# select bt_metap('at_partitioned_b_idx');
ERROR: relation "at_partitioned_b_idx" is not a btree index

Okay, I can see the point you are making here, in this case that's
actually a btree index but it has no on-disk data, so let's improve the
error handling. At the same time hash and gin functions don't make much
effort either...

I gather that pageinspect simply didn't get the memo about the new
RELKIND_PARTITIONED_INDEX "placeholder" relkind. I think that it
should at least give a friendlier error message than what we see here.
The same applies to amcheck, and possibly a few other modules.

So that's another c08d82f3... This needs to be really checked on a
yearly-basis as the trend of the last releases is to add at least one
new relkind per major version. There are three modules at stake here:
- pg_visibility
- pgstattuple
- pageinspect
I get to wonder about anything needed for sepgsql... Let's see that on
a separate thread after I look at the problem.

I also noticed that the new 'I' relkind is not documented within the
pg_class docs. I think that that needs to be updated.

Good catch.

The docs should
explain that 'I' relations are not backed by storage, since that will
probably affect users of one or two external tools.

Hm, the docs about taking backups with the low-level APIs don't care
much about relkind now:
https://www.postgresql.org/docs/devel/static/continuous-archiving.html#BACKUP-LOWLEVEL-BASE-BACKUP
Or do you have another section in the docs in mind?

Does the attached patch address everything you have seen? I have tried
to cope with anything I noticed as well on the way, which is made of:
- pg_visibility should have tests for partitioned indexes, the code
handles errors correctly.
- in pageinspect, get_raw_page() fails for partitioned indexes:
ERROR: could not open file "base/16384/16419": No such file or directory
- in pgstattuple, error message is incorrect for both index-related and
heap-related functions.

Thanks,
--
Michael

Attachments:

0001-Fix-gaps-in-modules-with-handling-of-partitioned-ind.patchtext/x-diff; charset=us-asciiDownload

From e8133ea66a27cd46e187408429d79891e3358978 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 24 Apr 2018 13:53:45 +0900
Subject: [PATCH] Fix gaps in modules with handling of partitioned indexes

The following modules lacked handling for partitioned indexes:
- pgstattuple
- pg_visibility
- pageinspect
For some index-related functions, a partitioned index can be defined of
the same object type as what the function works on but still get a
failure, so tweak those code path so as they complain when trying to
work on partitioned indexes.

Test cases are added to cover all those additions.
---
 contrib/pageinspect/btreefuncs.c               | 18 ++++++++++++++++++
 contrib/pageinspect/expected/btree.out         | 12 ++++++++++++
 contrib/pageinspect/expected/page.out          |  6 +++++-
 contrib/pageinspect/rawpage.c                  |  5 +++++
 contrib/pageinspect/sql/btree.sql              | 10 ++++++++++
 contrib/pageinspect/sql/page.sql               |  5 ++++-
 .../pg_visibility/expected/pg_visibility.out   | 13 ++++++++++++-
 contrib/pg_visibility/sql/pg_visibility.sql    |  8 +++++++-
 contrib/pgstattuple/expected/pgstattuple.out   | 13 +++++++++++++
 contrib/pgstattuple/pgstatindex.c              | 18 ++++++++++++++++++
 contrib/pgstattuple/pgstattuple.c              |  3 +++
 contrib/pgstattuple/sql/pgstattuple.sql        |  7 +++++++
 doc/src/sgml/catalogs.sgml                     |  3 ++-
 13 files changed, 116 insertions(+), 5 deletions(-)

diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 558a8c41f4..9ceb2d0b81 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -180,6 +180,12 @@ bt_page_stats(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("cannot get summary information of B-tree page for partitioned index \"%s\"",
+						RelationGetRelationName(rel))));
+
 	if (!IS_INDEX(rel) || !IS_BTREE(rel))
 		elog(ERROR, "relation \"%s\" is not a btree index",
 			 RelationGetRelationName(rel));
@@ -334,6 +340,12 @@ bt_page_items(PG_FUNCTION_ARGS)
 		relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 		rel = relation_openrv(relrv, AccessShareLock);
 
+		if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+			ereport(ERROR,
+					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+					 errmsg("cannot get information of B-tree items for partitioned index \"%s\"",
+							RelationGetRelationName(rel))));
+
 		if (!IS_INDEX(rel) || !IS_BTREE(rel))
 			elog(ERROR, "relation \"%s\" is not a btree index",
 				 RelationGetRelationName(rel));
@@ -524,6 +536,12 @@ bt_metap(PG_FUNCTION_ARGS)
 	relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
 	rel = relation_openrv(relrv, AccessShareLock);
 
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("cannot get information of B-tree metapage for partitioned index \"%s\"",
+						RelationGetRelationName(rel))));
+
 	if (!IS_INDEX(rel) || !IS_BTREE(rel))
 		elog(ERROR, "relation \"%s\" is not a btree index",
 			 RelationGetRelationName(rel));
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 2aaa4df53b..7b135d18e7 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -58,3 +58,15 @@ data       | 01 00 00 00 00 00 00 01
 SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
 ERROR:  block number 2 is out of range for relation "test1_a_idx"
 DROP TABLE test1;
+-- Partitioned indexes, all should fail
+CREATE TABLE test_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX test_partitioned_index ON test_partitioned (a);
+SELECT bt_metap('test_partitioned_index');
+ERROR:  cannot get information of B-tree metapage for partitioned index "test_partitioned_index"
+SELECT bt_page_stats('test_partitioned_index', 0);
+ERROR:  cannot get summary information of B-tree page for partitioned index "test_partitioned_index"
+SELECT bt_page_items('test_partitioned_index', 0);
+ERROR:  cannot get information of B-tree items for partitioned index "test_partitioned_index"
+SELECT * FROM bt_page_items(get_raw_page('test_partitioned_index', 0));
+ERROR:  cannot get raw page from partitioned index "test_partitioned_index"
+DROP TABLE test_partitioned;
diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 5edb650085..3fcd9fbe6d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -83,10 +83,14 @@ 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
+-- check that using any of these functions with a partitioned table or index
+-- would fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned (a);
 select get_raw_page('test_partitioned', 0); -- error about partitioned table
 ERROR:  cannot get raw page from partitioned table "test_partitioned"
+select get_raw_page('test_partitioned_index', 0); -- error about partitioned index
+ERROR:  cannot get raw page from partitioned index "test_partitioned_index"
 -- a regular table which is a member of a partition set should work though
 create table test_part1 partition of test_partitioned for values from ( 1 ) to (100);
 select get_raw_page('test_part1', 0); -- get farther and error about empty table
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 72f1d21e1b..d7bf782ccd 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -128,6 +128,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from partitioned table \"%s\"",
 						RelationGetRelationName(rel))));
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("cannot get raw page from partitioned index \"%s\"",
+						RelationGetRelationName(rel))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/btree.sql b/contrib/pageinspect/sql/btree.sql
index 8eac64c7b3..42e90d897d 100644
--- a/contrib/pageinspect/sql/btree.sql
+++ b/contrib/pageinspect/sql/btree.sql
@@ -19,3 +19,13 @@ SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
 SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
 
 DROP TABLE test1;
+
+-- Partitioned indexes, all should fail
+CREATE TABLE test_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX test_partitioned_index ON test_partitioned (a);
+SELECT bt_metap('test_partitioned_index');
+SELECT bt_page_stats('test_partitioned_index', 0);
+SELECT bt_page_items('test_partitioned_index', 0);
+SELECT * FROM bt_page_items(get_raw_page('test_partitioned_index', 0));
+
+DROP TABLE test_partitioned;
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 8f35830e06..8ac9991837 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -33,9 +33,12 @@ 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
+-- check that using any of these functions with a partitioned table or index
+-- would fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned (a);
 select get_raw_page('test_partitioned', 0); -- error about partitioned table
+select get_raw_page('test_partitioned_index', 0); -- error about partitioned index
 
 -- a regular table which is a member of a partition set should work though
 create table test_part1 partition of test_partitioned for values from ( 1 ) to (100);
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index f0dcb897c4..3500e83ebc 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -2,19 +2,30 @@ CREATE EXTENSION pg_visibility;
 --
 -- check that using the module's functions with unsupported relations will fail
 --
--- partitioned tables (the parent ones) don't have visibility maps
+-- partitioned tables and indexes (the parent ones) don't have visibility maps
 create table test_partitioned (a int) partition by list (a);
+create index test_partitioned_index on test_partitioned (a);
 -- these should all fail
 select pg_visibility('test_partitioned', 0);
 ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility('test_partitioned_index', 0);
+ERROR:  "test_partitioned_index" 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('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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_visibility_map_summary('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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_check_frozen('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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
+select pg_truncate_visibility_map('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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);
 -- indexes do not, so these all fail
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
index c2a7f1d9e4..648ee86d05 100644
--- a/contrib/pg_visibility/sql/pg_visibility.sql
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -4,14 +4,20 @@ CREATE EXTENSION pg_visibility;
 -- check that using the module's functions with unsupported relations will fail
 --
 
--- partitioned tables (the parent ones) don't have visibility maps
+-- partitioned tables and indexes (the parent ones) don't have visibility maps
 create table test_partitioned (a int) partition by list (a);
+create index test_partitioned_index on test_partitioned (a);
 -- these should all fail
 select pg_visibility('test_partitioned', 0);
+select pg_visibility('test_partitioned_index', 0);
 select pg_visibility_map('test_partitioned');
+select pg_visibility_map('test_partitioned_index');
 select pg_visibility_map_summary('test_partitioned');
+select pg_visibility_map_summary('test_partitioned_index');
 select pg_check_frozen('test_partitioned');
+select pg_check_frozen('test_partitioned_index');
 select pg_truncate_visibility_map('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned_index');
 
 create table test_partition partition of test_partitioned for values in (1);
 create index test_index on test_partition (a);
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index a7087f6d45..c0bbd1440b 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -152,19 +152,32 @@ select pgstatginindex('test_hashidx');
 ERROR:  relation "test_hashidx" is not a GIN index
 -- check that using any of these functions with unsupported relations will fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned(a);
 -- these should all fail
 select pgstattuple('test_partitioned');
 ERROR:  "test_partitioned" (partitioned table) is not supported
+select pgstattuple('test_partitioned_index');
+ERROR:  "test_partitioned_index" (partitioned index) is not supported
 select pgstattuple_approx('test_partitioned');
 ERROR:  "test_partitioned" is not a table or materialized view
+select pgstattuple_approx('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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
+select pg_relpages('test_partitioned_index');
+ERROR:  "test_partitioned_index" is not a table, index, materialized view, sequence, or TOAST table
 select pgstatindex('test_partitioned');
 ERROR:  relation "test_partitioned" is not a btree index
+select pgstatindex('test_partitioned_index');
+ERROR:  "test_partitioned_index" is a partitioned index
 select pgstatginindex('test_partitioned');
 ERROR:  relation "test_partitioned" is not a GIN index
+select pgstatginindex('test_partitioned_index');
+ERROR:  "test_partitioned_index" is a partitioned index
 select pgstathashindex('test_partitioned');
 ERROR:  "test_partitioned" is not an index
+select pgstathashindex('test_partitioned_index');
+ERROR:  "test_partitioned_index" is a partitioned index
 create view test_view as select 1;
 -- these should all fail
 select pgstattuple('test_view');
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 75317b96a2..3b99ed46c7 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -220,6 +220,12 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
 	BTIndexStat indexStat;
 	BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
 
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is a partitioned index",
+						RelationGetRelationName(rel))));
+
 	if (!IS_INDEX(rel) || !IS_BTREE(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
@@ -521,6 +527,12 @@ pgstatginindex_internal(Oid relid, FunctionCallInfo fcinfo)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is a partitioned index",
+						RelationGetRelationName(rel))));
+
 	if (!IS_INDEX(rel) || !IS_GIN(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
@@ -597,6 +609,12 @@ pgstathashindex(PG_FUNCTION_ARGS)
 
 	rel = index_open(relid, AccessShareLock);
 
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is a partitioned index",
+						RelationGetRelationName(rel))));
+
 	/* index_open() checks that it's an index */
 	if (!IS_HASH(rel))
 		ereport(ERROR,
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index b599b6ca21..6d67bd8271 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -296,6 +296,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_PARTITIONED_TABLE:
 			err = "partitioned table";
 			break;
+		case RELKIND_PARTITIONED_INDEX:
+			err = "partitioned index";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index a8e341e351..1459672cab 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -64,13 +64,20 @@ select pgstatginindex('test_hashidx');
 
 -- check that using any of these functions with unsupported relations will fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned(a);
 -- these should all fail
 select pgstattuple('test_partitioned');
+select pgstattuple('test_partitioned_index');
 select pgstattuple_approx('test_partitioned');
+select pgstattuple_approx('test_partitioned_index');
 select pg_relpages('test_partitioned');
+select pg_relpages('test_partitioned_index');
 select pgstatindex('test_partitioned');
+select pgstatindex('test_partitioned_index');
 select pgstatginindex('test_partitioned');
+select pgstatginindex('test_partitioned_index');
 select pgstathashindex('test_partitioned');
+select pgstathashindex('test_partitioned_index');
 
 create view test_view as select 1;
 -- these should all fail
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 14aeed3076..91003522f2 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1840,7 +1840,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <literal>m</literal> = materialized view,
        <literal>c</literal> = composite type,
        <literal>f</literal> = foreign table,
-       <literal>p</literal> = partitioned table
+       <literal>p</literal> = partitioned table,
+       <literal>I</literal> = partitioned index
       </entry>
      </row>
 
-- 
2.17.0

Peter Geoghegan

pg@bowt.ie

over 7 years ago

In reply to: Michael Paquier (#2)

Re: Local partitioned indexes and pageinspect

On Mon, Apr 23, 2018 at 9:58 PM, Michael Paquier <michael@paquier.xyz> wrote:

Hm, the docs about taking backups with the low-level APIs don't care
much about relkind now:
https://www.postgresql.org/docs/devel/static/continuous-archiving.html#BACKUP-LOWLEVEL-BASE-BACKUP
Or do you have another section in the docs in mind?

No, I didn't. We should definitely document new relkinds in the
pg_class docs, though.

Does the attached patch address everything you have seen?

What about amcheck? I did change the example query in the docs to
account for this, so anyone that generalizes from that won't have a
problem, but it would be nice if it had a friendlier message. I didn't
change amcheck to account for this myself because I thought it was
possible that somebody else would want to use a more general solution.

--
Peter Geoghegan

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Peter Geoghegan (#3)

Re: Local partitioned indexes and pageinspect

On Sun, Apr 29, 2018 at 06:20:02PM -0700, Peter Geoghegan wrote:

What about amcheck? I did change the example query in the docs to
account for this, so anyone that generalizes from that won't have a
problem, but it would be nice if it had a friendlier message. I didn't
change amcheck to account for this myself because I thought it was
possible that somebody else would want to use a more general solution.

Perhaps it would help if an errhint is added which is written as "This
relation is a %s", where %s is a relkind converted to a translatable
string describing the kind? All those modules work on different objects
and have different goals and prospoectives, so it looks difficult to me
to come up with a sane API which is rather portable across modules.

The statu-quo is not completely bad either (aka no extra error
messages) as incorrect relation kind are still filtered out, perhaps the
docs don't emphasize enough that an index and a partitioned index are
two different things?
--
Michael

Robert Haas

robertmhaas@gmail.com

over 7 years ago

In reply to: Michael Paquier (#4)

Re: Local partitioned indexes and pageinspect

On Mon, Apr 30, 2018 at 2:50 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Apr 29, 2018 at 06:20:02PM -0700, Peter Geoghegan wrote:

What about amcheck? I did change the example query in the docs to
account for this, so anyone that generalizes from that won't have a
problem, but it would be nice if it had a friendlier message. I didn't
change amcheck to account for this myself because I thought it was
possible that somebody else would want to use a more general solution.

Perhaps it would help if an errhint is added which is written as "This
relation is a %s", where %s is a relkind converted to a translatable
string describing the kind? All those modules work on different objects
and have different goals and prospoectives, so it looks difficult to me
to come up with a sane API which is rather portable across modules.

That's probably going to cause some translation problems. The form of
"a" that you need will vary: "a" vs. "an" in English, "un" vs. "una"
in Spanish, etc. And it wouldn't be surprising if there are problems
in some languages even if you make it "This relation is %s". There's
a reason why the documentation advises against building up
translatable strings from constituent parts. If we go this route,
it's probably best to separately translate "This relation is a
table.", "This relation is an index.", etc.

However, backing up a minute, I don't think "relation \"%s\" is not a
btree index" is such a terrible message. These modules are intended
to be intended by people who Know What They Are Doing. If we do want
to change the message, I submit that the only thing that makes it a
little unclear is that a user might fail to realize that a partitioned
index is not an index. But that could be fixed just by adding a
separate message for that one case (index \"%s\" is partitioned) and
sticking with the existing message for other cases.

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

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Robert Haas (#5)

1 attachment(s)

Re: Local partitioned indexes and pageinspect

On Tue, May 01, 2018 at 12:30:44PM -0400, Robert Haas wrote:

That's probably going to cause some translation problems. The form of
"a" that you need will vary: "a" vs. "an" in English, "un" vs. "una"
in Spanish, etc. And it wouldn't be surprising if there are problems
in some languages even if you make it "This relation is %s". There's
a reason why the documentation advises against building up
translatable strings from constituent parts. If we go this route,
it's probably best to separately translate "This relation is a
table.", "This relation is an index.", etc.

Yeah, I get the feeling that this is not going to be much
consistent-proof either, so while I have not been able to come up with a
better idea, let's not go through this route.

However, backing up a minute, I don't think "relation \"%s\" is not a
btree index" is such a terrible message. These modules are intended
to be intended by people who Know What They Are Doing. If we do want
to change the message, I submit that the only thing that makes it a
little unclear is that a user might fail to realize that a partitioned
index is not an index. But that could be fixed just by adding a
separate message for that one case (index \"%s\" is partitioned) and
sticking with the existing message for other cases.

I have been chewing on that, and I come to agree that there is perhaps
little point to complicate the code as long as a failure is properly
reported to the user. I propose hence the attached, which adds test
cases in the contrib module set for partitioned indexes (amcheck also
lacked tests for partition tables and indexes), and fixes a set of code
paths to be consistent with the presence of this new relkind.

Visibly I have found a bug in git, format-patch reports the following
line in the stats:
.../pg_visibility/expected/pg_visibility.out | 13 ++++++++++++-
But the patch contents are actually correct...
--
Michael

Attachments:

0001-Fix-gaps-in-modules-with-handling-of-partitioned-ind.patchtext/x-diff; charset=us-asciiDownload

From f71b4d1932fd6da3a679f9d755f387f5986c71e8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 2 May 2018 10:56:29 +0900
Subject: [PATCH] Fix gaps in modules with handling of partitioned indexes

The following modules lacked handling and/or coverage for partitioned
indexes:
- pgstattuple
- pg_visibility
- pageinspect
- amcheck
For some index-related functions, a partitioned index can be defined of
the same object type as what the function works on but still get a
failure, still the error messages are kept the same to keep the code
simple.

Test cases are added to cover all those additions.
---
 contrib/amcheck/expected/check_btree.out        | 17 ++++++++++++++++-
 contrib/amcheck/sql/check_btree.sql             | 13 ++++++++++++-
 contrib/pageinspect/expected/btree.out          | 12 ++++++++++++
 contrib/pageinspect/expected/page.out           |  6 +++++-
 contrib/pageinspect/rawpage.c                   |  5 +++++
 contrib/pageinspect/sql/btree.sql               | 10 ++++++++++
 contrib/pageinspect/sql/page.sql                |  5 ++++-
 .../pg_visibility/expected/pg_visibility.out    | 13 ++++++++++++-
 contrib/pg_visibility/sql/pg_visibility.sql     |  8 +++++++-
 contrib/pgstattuple/expected/pgstattuple.out    | 13 +++++++++++++
 contrib/pgstattuple/pgstatindex.c               |  1 -
 contrib/pgstattuple/pgstattuple.c               |  3 +++
 contrib/pgstattuple/sql/pgstattuple.sql         |  7 +++++++
 doc/src/sgml/catalogs.sgml                      |  3 ++-
 14 files changed, 108 insertions(+), 8 deletions(-)

diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index e864579774..b288565be9 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -2,7 +2,9 @@ CREATE TABLE bttest_a(id int8);
 CREATE TABLE bttest_b(id int8);
 CREATE TABLE bttest_multi(id int8, data int8);
 CREATE TABLE delete_test_table (a bigint, b bigint, c bigint, d bigint);
--- Stabalize tests
+CREATE TABLE bttest_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX bttest_partitioned_index ON bttest_partitioned (a);
+-- Stabilize tests
 ALTER TABLE bttest_a SET (autovacuum_enabled = false);
 ALTER TABLE bttest_b SET (autovacuum_enabled = false);
 ALTER TABLE bttest_multi SET (autovacuum_enabled = false);
@@ -48,6 +50,18 @@ SELECT bt_index_check('bttest_a');
 ERROR:  "bttest_a" is not an index
 SELECT bt_index_parent_check('bttest_a');
 ERROR:  "bttest_a" is not an index
+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index
+SELECT bt_index_parent_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index
+-- verify partitioned indexes are rejected (error)
+SELECT bt_index_check('bttest_partitioned_index');
+ERROR:  only B-Tree indexes are supported as targets for verification
+DETAIL:  Relation "bttest_partitioned_index" is not a B-Tree index.
+SELECT bt_index_parent_check('bttest_partitioned_index');
+ERROR:  only B-Tree indexes are supported as targets for verification
+DETAIL:  Relation "bttest_partitioned_index" is not a B-Tree index.
 -- verify non-existing indexes are rejected (error)
 SELECT bt_index_check(17);
 ERROR:  could not open relation with OID 17
@@ -145,5 +159,6 @@ DROP TABLE bttest_a;
 DROP TABLE bttest_b;
 DROP TABLE bttest_multi;
 DROP TABLE delete_test_table;
+DROP TABLE bttest_partitioned;
 DROP OWNED BY bttest_role; -- permissions
 DROP ROLE bttest_role;
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 7b1ab4f148..cad496eda8 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -2,8 +2,10 @@ CREATE TABLE bttest_a(id int8);
 CREATE TABLE bttest_b(id int8);
 CREATE TABLE bttest_multi(id int8, data int8);
 CREATE TABLE delete_test_table (a bigint, b bigint, c bigint, d bigint);
+CREATE TABLE bttest_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX bttest_partitioned_index ON bttest_partitioned (a);
 
--- Stabalize tests
+-- Stabilize tests
 ALTER TABLE bttest_a SET (autovacuum_enabled = false);
 ALTER TABLE bttest_b SET (autovacuum_enabled = false);
 ALTER TABLE bttest_multi SET (autovacuum_enabled = false);
@@ -42,6 +44,14 @@ RESET ROLE;
 SELECT bt_index_check('bttest_a');
 SELECT bt_index_parent_check('bttest_a');
 
+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+SELECT bt_index_parent_check('bttest_partitioned');
+
+-- verify partitioned indexes are rejected (error)
+SELECT bt_index_check('bttest_partitioned_index');
+SELECT bt_index_parent_check('bttest_partitioned_index');
+
 -- verify non-existing indexes are rejected (error)
 SELECT bt_index_check(17);
 SELECT bt_index_parent_check(17);
@@ -93,5 +103,6 @@ DROP TABLE bttest_a;
 DROP TABLE bttest_b;
 DROP TABLE bttest_multi;
 DROP TABLE delete_test_table;
+DROP TABLE bttest_partitioned;
 DROP OWNED BY bttest_role; -- permissions
 DROP ROLE bttest_role;
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 2aaa4df53b..cb2f04874b 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -58,3 +58,15 @@ data       | 01 00 00 00 00 00 00 01
 SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
 ERROR:  block number 2 is out of range for relation "test1_a_idx"
 DROP TABLE test1;
+-- Partitioned indexes, all should fail
+CREATE TABLE test_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX test_partitioned_index ON test_partitioned (a);
+SELECT bt_metap('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a btree index
+SELECT bt_page_stats('test_partitioned_index', 0);
+ERROR:  relation "test_partitioned_index" is not a btree index
+SELECT bt_page_items('test_partitioned_index', 0);
+ERROR:  relation "test_partitioned_index" is not a btree index
+SELECT * FROM bt_page_items(get_raw_page('test_partitioned_index', 0));
+ERROR:  cannot get raw page from partitioned index "test_partitioned_index"
+DROP TABLE test_partitioned;
diff --git a/contrib/pageinspect/expected/page.out b/contrib/pageinspect/expected/page.out
index 5edb650085..3fcd9fbe6d 100644
--- a/contrib/pageinspect/expected/page.out
+++ b/contrib/pageinspect/expected/page.out
@@ -83,10 +83,14 @@ 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
+-- check that using any of these functions with a partitioned table or index
+-- would fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned (a);
 select get_raw_page('test_partitioned', 0); -- error about partitioned table
 ERROR:  cannot get raw page from partitioned table "test_partitioned"
+select get_raw_page('test_partitioned_index', 0); -- error about partitioned index
+ERROR:  cannot get raw page from partitioned index "test_partitioned_index"
 -- a regular table which is a member of a partition set should work though
 create table test_part1 partition of test_partitioned for values from ( 1 ) to (100);
 select get_raw_page('test_part1', 0); -- get farther and error about empty table
diff --git a/contrib/pageinspect/rawpage.c b/contrib/pageinspect/rawpage.c
index 72f1d21e1b..d7bf782ccd 100644
--- a/contrib/pageinspect/rawpage.c
+++ b/contrib/pageinspect/rawpage.c
@@ -128,6 +128,11 @@ get_raw_page_internal(text *relname, ForkNumber forknum, BlockNumber blkno)
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot get raw page from partitioned table \"%s\"",
 						RelationGetRelationName(rel))));
+	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("cannot get raw page from partitioned index \"%s\"",
+						RelationGetRelationName(rel))));
 
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
diff --git a/contrib/pageinspect/sql/btree.sql b/contrib/pageinspect/sql/btree.sql
index 8eac64c7b3..42e90d897d 100644
--- a/contrib/pageinspect/sql/btree.sql
+++ b/contrib/pageinspect/sql/btree.sql
@@ -19,3 +19,13 @@ SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
 SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
 
 DROP TABLE test1;
+
+-- Partitioned indexes, all should fail
+CREATE TABLE test_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX test_partitioned_index ON test_partitioned (a);
+SELECT bt_metap('test_partitioned_index');
+SELECT bt_page_stats('test_partitioned_index', 0);
+SELECT bt_page_items('test_partitioned_index', 0);
+SELECT * FROM bt_page_items(get_raw_page('test_partitioned_index', 0));
+
+DROP TABLE test_partitioned;
diff --git a/contrib/pageinspect/sql/page.sql b/contrib/pageinspect/sql/page.sql
index 8f35830e06..8ac9991837 100644
--- a/contrib/pageinspect/sql/page.sql
+++ b/contrib/pageinspect/sql/page.sql
@@ -33,9 +33,12 @@ 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
+-- check that using any of these functions with a partitioned table or index
+-- would fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned (a);
 select get_raw_page('test_partitioned', 0); -- error about partitioned table
+select get_raw_page('test_partitioned_index', 0); -- error about partitioned index
 
 -- a regular table which is a member of a partition set should work though
 create table test_part1 partition of test_partitioned for values from ( 1 ) to (100);
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index f0dcb897c4..3500e83ebc 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -2,19 +2,30 @@ CREATE EXTENSION pg_visibility;
 --
 -- check that using the module's functions with unsupported relations will fail
 --
--- partitioned tables (the parent ones) don't have visibility maps
+-- partitioned tables and indexes (the parent ones) don't have visibility maps
 create table test_partitioned (a int) partition by list (a);
+create index test_partitioned_index on test_partitioned (a);
 -- these should all fail
 select pg_visibility('test_partitioned', 0);
 ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility('test_partitioned_index', 0);
+ERROR:  "test_partitioned_index" 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('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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_visibility_map_summary('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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_check_frozen('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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
+select pg_truncate_visibility_map('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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);
 -- indexes do not, so these all fail
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
index c2a7f1d9e4..648ee86d05 100644
--- a/contrib/pg_visibility/sql/pg_visibility.sql
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -4,14 +4,20 @@ CREATE EXTENSION pg_visibility;
 -- check that using the module's functions with unsupported relations will fail
 --
 
--- partitioned tables (the parent ones) don't have visibility maps
+-- partitioned tables and indexes (the parent ones) don't have visibility maps
 create table test_partitioned (a int) partition by list (a);
+create index test_partitioned_index on test_partitioned (a);
 -- these should all fail
 select pg_visibility('test_partitioned', 0);
+select pg_visibility('test_partitioned_index', 0);
 select pg_visibility_map('test_partitioned');
+select pg_visibility_map('test_partitioned_index');
 select pg_visibility_map_summary('test_partitioned');
+select pg_visibility_map_summary('test_partitioned_index');
 select pg_check_frozen('test_partitioned');
+select pg_check_frozen('test_partitioned_index');
 select pg_truncate_visibility_map('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned_index');
 
 create table test_partition partition of test_partitioned for values in (1);
 create index test_index on test_partition (a);
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index a7087f6d45..4b126d3be8 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -152,19 +152,32 @@ select pgstatginindex('test_hashidx');
 ERROR:  relation "test_hashidx" is not a GIN index
 -- check that using any of these functions with unsupported relations will fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned(a);
 -- these should all fail
 select pgstattuple('test_partitioned');
 ERROR:  "test_partitioned" (partitioned table) is not supported
+select pgstattuple('test_partitioned_index');
+ERROR:  "test_partitioned_index" (partitioned index) is not supported
 select pgstattuple_approx('test_partitioned');
 ERROR:  "test_partitioned" is not a table or materialized view
+select pgstattuple_approx('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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
+select pg_relpages('test_partitioned_index');
+ERROR:  "test_partitioned_index" is not a table, index, materialized view, sequence, or TOAST table
 select pgstatindex('test_partitioned');
 ERROR:  relation "test_partitioned" is not a btree index
+select pgstatindex('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a btree index
 select pgstatginindex('test_partitioned');
 ERROR:  relation "test_partitioned" is not a GIN index
+select pgstatginindex('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a GIN index
 select pgstathashindex('test_partitioned');
 ERROR:  "test_partitioned" is not an index
+select pgstathashindex('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a HASH index
 create view test_view as select 1;
 -- these should all fail
 select pgstattuple('test_view');
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 75317b96a2..3d50262f83 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -604,7 +604,6 @@ pgstathashindex(PG_FUNCTION_ARGS)
 				 errmsg("relation \"%s\" is not a HASH index",
 						RelationGetRelationName(rel))));
 
-
 	/*
 	 * Reject attempts to read non-local temporary relations; we would be
 	 * likely to get wrong data since we have no visibility into the owning
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index b599b6ca21..6d67bd8271 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -296,6 +296,9 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 		case RELKIND_PARTITIONED_TABLE:
 			err = "partitioned table";
 			break;
+		case RELKIND_PARTITIONED_INDEX:
+			err = "partitioned index";
+			break;
 		default:
 			err = "unknown";
 			break;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index a8e341e351..1459672cab 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -64,13 +64,20 @@ select pgstatginindex('test_hashidx');
 
 -- check that using any of these functions with unsupported relations will fail
 create table test_partitioned (a int) partition by range (a);
+create index test_partitioned_index on test_partitioned(a);
 -- these should all fail
 select pgstattuple('test_partitioned');
+select pgstattuple('test_partitioned_index');
 select pgstattuple_approx('test_partitioned');
+select pgstattuple_approx('test_partitioned_index');
 select pg_relpages('test_partitioned');
+select pg_relpages('test_partitioned_index');
 select pgstatindex('test_partitioned');
+select pgstatindex('test_partitioned_index');
 select pgstatginindex('test_partitioned');
+select pgstatginindex('test_partitioned_index');
 select pgstathashindex('test_partitioned');
+select pgstathashindex('test_partitioned_index');
 
 create view test_view as select 1;
 -- these should all fail
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 26984b6cba..16181db926 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1840,7 +1840,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        <literal>m</literal> = materialized view,
        <literal>c</literal> = composite type,
        <literal>f</literal> = foreign table,
-       <literal>p</literal> = partitioned table
+       <literal>p</literal> = partitioned table,
+       <literal>I</literal> = partitioned index
       </entry>
      </row>
 
-- 
2.17.0

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 7 years ago

In reply to: Michael Paquier (#6)

Re: [SPAM] Re: Local partitioned indexes and pageinspect

Hi.

On 2018/05/02 11:05, Michael Paquier wrote:

On Tue, May 01, 2018 at 12:30:44PM -0400, Robert Haas wrote:

However, backing up a minute, I don't think "relation \"%s\" is not a
btree index" is such a terrible message. These modules are intended
to be intended by people who Know What They Are Doing. If we do want
to change the message, I submit that the only thing that makes it a
little unclear is that a user might fail to realize that a partitioned
index is not an index. But that could be fixed just by adding a
separate message for that one case (index \"%s\" is partitioned) and
sticking with the existing message for other cases.

I have been chewing on that, and I come to agree that there is perhaps
little point to complicate the code as long as a failure is properly
reported to the user. I propose hence the attached, which adds test
cases in the contrib module set for partitioned indexes (amcheck also
lacked tests for partition tables and indexes), and fixes a set of code
paths to be consistent with the presence of this new relkind.

--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out

+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index

Perhaps, I'm just repeating what's already been said, but I think it might
be better to have the word "partitioned" in the message.

ERROR: "bttest_partitioned" is partitioned index

..which Robert seems to think might not be too bad.

That will need adding some code to these modules like we did in
c08d82f38ebf763 [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c08d82f38eb.

Thanks,
Amit

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c08d82f38eb

Amit Langote

Langote_Amit_f8@lab.ntt.co.jp

over 7 years ago

In reply to: Amit Langote (#7)

Re: Local partitioned indexes and pageinspect

On 2018/05/02 13:38, Amit Langote wrote:

--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index
Perhaps, I'm just repeating what's already been said, but I think it might
be better to have the word "partitioned" in the message.

ERROR: "bttest_partitioned" is partitioned index

..which Robert seems to think might not be too bad.

That will need adding some code to these modules like we did in
c08d82f38ebf763 [1].

Sorry, it appears that our mail system added "[SPAM]" to the subject-line
for one reason or another, which I forgot to manually remove when editing
the email.

- Amit

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Amit Langote (#7)

Re: [SPAM] Re: Local partitioned indexes and pageinspect

On Wed, May 02, 2018 at 01:38:22PM +0900, Amit Langote wrote:

Perhaps, I'm just repeating what's already been said, but I think it might
be better to have the word "partitioned" in the message.

That's what Peter is pointing to upthread and what the v1 of upthread
was doing. I would tend to think to just keep the code simple and don't
add those extra checks, but I am fine to be beaten as well.
--
Michael

#10

Alvaro Herrera

alvherre@2ndquadrant.com

over 7 years ago

In reply to: Michael Paquier (#9)

1 attachment(s)

Re: [SPAM] Re: Local partitioned indexes and pageinspect

Michael Paquier wrote:

On Wed, May 02, 2018 at 01:38:22PM +0900, Amit Langote wrote:

Perhaps, I'm just repeating what's already been said, but I think it might
be better to have the word "partitioned" in the message.

That's what Peter is pointing to upthread and what the v1 of upthread
was doing. I would tend to think to just keep the code simple and don't
add those extra checks, but I am fine to be beaten as well.

I pushed some fixes produced here. Attached is the remainder of the
patch you submitted. I notice now that we haven't actually fixed
Peter's source of complaint, though. AFAICS your patch just adds test
cases, and upthread discussion apparently converges on not doing
anything about the code. I'm not yet sure what to think of that ...

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

Attachments:

v3-0001-Fix-gaps-in-modules-with-handling-of-partitioned-.patchtext/plain; charset=us-asciiDownload

From 4399d0e7a69faa28403cc211c9b7d6495da23cf3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 2 May 2018 10:56:29 +0900
Subject: [PATCH v3] Fix gaps in modules with handling of partitioned indexes

The following modules lacked handling and/or coverage for partitioned
indexes:
- pgstattuple
- pg_visibility
- pageinspect
- amcheck
For some index-related functions, a partitioned index can be defined of
the same object type as what the function works on but still get a
failure, still the error messages are kept the same to keep the code
simple.

Test cases are added to cover all those additions.
---
 contrib/amcheck/expected/check_btree.out         | 17 ++++++++++++++++-
 contrib/amcheck/sql/check_btree.sql              | 13 ++++++++++++-
 contrib/pageinspect/expected/btree.out           | 12 ++++++++++++
 contrib/pageinspect/sql/btree.sql                | 10 ++++++++++
 contrib/pg_visibility/expected/pg_visibility.out | 13 ++++++++++++-
 contrib/pg_visibility/sql/pg_visibility.sql      |  8 +++++++-
 contrib/pgstattuple/expected/pgstattuple.out     | 10 ++++++++++
 contrib/pgstattuple/sql/pgstattuple.sql          |  5 +++++
 8 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index e864579774..b288565be9 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -2,7 +2,9 @@ CREATE TABLE bttest_a(id int8);
 CREATE TABLE bttest_b(id int8);
 CREATE TABLE bttest_multi(id int8, data int8);
 CREATE TABLE delete_test_table (a bigint, b bigint, c bigint, d bigint);
--- Stabalize tests
+CREATE TABLE bttest_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX bttest_partitioned_index ON bttest_partitioned (a);
+-- Stabilize tests
 ALTER TABLE bttest_a SET (autovacuum_enabled = false);
 ALTER TABLE bttest_b SET (autovacuum_enabled = false);
 ALTER TABLE bttest_multi SET (autovacuum_enabled = false);
@@ -48,6 +50,18 @@ SELECT bt_index_check('bttest_a');
 ERROR:  "bttest_a" is not an index
 SELECT bt_index_parent_check('bttest_a');
 ERROR:  "bttest_a" is not an index
+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index
+SELECT bt_index_parent_check('bttest_partitioned');
+ERROR:  "bttest_partitioned" is not an index
+-- verify partitioned indexes are rejected (error)
+SELECT bt_index_check('bttest_partitioned_index');
+ERROR:  only B-Tree indexes are supported as targets for verification
+DETAIL:  Relation "bttest_partitioned_index" is not a B-Tree index.
+SELECT bt_index_parent_check('bttest_partitioned_index');
+ERROR:  only B-Tree indexes are supported as targets for verification
+DETAIL:  Relation "bttest_partitioned_index" is not a B-Tree index.
 -- verify non-existing indexes are rejected (error)
 SELECT bt_index_check(17);
 ERROR:  could not open relation with OID 17
@@ -145,5 +159,6 @@ DROP TABLE bttest_a;
 DROP TABLE bttest_b;
 DROP TABLE bttest_multi;
 DROP TABLE delete_test_table;
+DROP TABLE bttest_partitioned;
 DROP OWNED BY bttest_role; -- permissions
 DROP ROLE bttest_role;
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 7b1ab4f148..cad496eda8 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -2,8 +2,10 @@ CREATE TABLE bttest_a(id int8);
 CREATE TABLE bttest_b(id int8);
 CREATE TABLE bttest_multi(id int8, data int8);
 CREATE TABLE delete_test_table (a bigint, b bigint, c bigint, d bigint);
+CREATE TABLE bttest_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX bttest_partitioned_index ON bttest_partitioned (a);
 
--- Stabalize tests
+-- Stabilize tests
 ALTER TABLE bttest_a SET (autovacuum_enabled = false);
 ALTER TABLE bttest_b SET (autovacuum_enabled = false);
 ALTER TABLE bttest_multi SET (autovacuum_enabled = false);
@@ -42,6 +44,14 @@ RESET ROLE;
 SELECT bt_index_check('bttest_a');
 SELECT bt_index_parent_check('bttest_a');
 
+-- verify partitioned tables are rejected (error)
+SELECT bt_index_check('bttest_partitioned');
+SELECT bt_index_parent_check('bttest_partitioned');
+
+-- verify partitioned indexes are rejected (error)
+SELECT bt_index_check('bttest_partitioned_index');
+SELECT bt_index_parent_check('bttest_partitioned_index');
+
 -- verify non-existing indexes are rejected (error)
 SELECT bt_index_check(17);
 SELECT bt_index_parent_check(17);
@@ -93,5 +103,6 @@ DROP TABLE bttest_a;
 DROP TABLE bttest_b;
 DROP TABLE bttest_multi;
 DROP TABLE delete_test_table;
+DROP TABLE bttest_partitioned;
 DROP OWNED BY bttest_role; -- permissions
 DROP ROLE bttest_role;
diff --git a/contrib/pageinspect/expected/btree.out b/contrib/pageinspect/expected/btree.out
index 2aaa4df53b..cb2f04874b 100644
--- a/contrib/pageinspect/expected/btree.out
+++ b/contrib/pageinspect/expected/btree.out
@@ -58,3 +58,15 @@ data       | 01 00 00 00 00 00 00 01
 SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
 ERROR:  block number 2 is out of range for relation "test1_a_idx"
 DROP TABLE test1;
+-- Partitioned indexes, all should fail
+CREATE TABLE test_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX test_partitioned_index ON test_partitioned (a);
+SELECT bt_metap('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a btree index
+SELECT bt_page_stats('test_partitioned_index', 0);
+ERROR:  relation "test_partitioned_index" is not a btree index
+SELECT bt_page_items('test_partitioned_index', 0);
+ERROR:  relation "test_partitioned_index" is not a btree index
+SELECT * FROM bt_page_items(get_raw_page('test_partitioned_index', 0));
+ERROR:  cannot get raw page from partitioned index "test_partitioned_index"
+DROP TABLE test_partitioned;
diff --git a/contrib/pageinspect/sql/btree.sql b/contrib/pageinspect/sql/btree.sql
index 8eac64c7b3..42e90d897d 100644
--- a/contrib/pageinspect/sql/btree.sql
+++ b/contrib/pageinspect/sql/btree.sql
@@ -19,3 +19,13 @@ SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 1));
 SELECT * FROM bt_page_items(get_raw_page('test1_a_idx', 2));
 
 DROP TABLE test1;
+
+-- Partitioned indexes, all should fail
+CREATE TABLE test_partitioned (a int) PARTITION BY RANGE (a);
+CREATE INDEX test_partitioned_index ON test_partitioned (a);
+SELECT bt_metap('test_partitioned_index');
+SELECT bt_page_stats('test_partitioned_index', 0);
+SELECT bt_page_items('test_partitioned_index', 0);
+SELECT * FROM bt_page_items(get_raw_page('test_partitioned_index', 0));
+
+DROP TABLE test_partitioned;
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index f0dcb897c4..3500e83ebc 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -2,19 +2,30 @@ CREATE EXTENSION pg_visibility;
 --
 -- check that using the module's functions with unsupported relations will fail
 --
--- partitioned tables (the parent ones) don't have visibility maps
+-- partitioned tables and indexes (the parent ones) don't have visibility maps
 create table test_partitioned (a int) partition by list (a);
+create index test_partitioned_index on test_partitioned (a);
 -- these should all fail
 select pg_visibility('test_partitioned', 0);
 ERROR:  "test_partitioned" is not a table, materialized view, or TOAST table
+select pg_visibility('test_partitioned_index', 0);
+ERROR:  "test_partitioned_index" 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('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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_visibility_map_summary('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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_check_frozen('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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
+select pg_truncate_visibility_map('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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);
 -- indexes do not, so these all fail
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
index c2a7f1d9e4..648ee86d05 100644
--- a/contrib/pg_visibility/sql/pg_visibility.sql
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -4,14 +4,20 @@ CREATE EXTENSION pg_visibility;
 -- check that using the module's functions with unsupported relations will fail
 --
 
--- partitioned tables (the parent ones) don't have visibility maps
+-- partitioned tables and indexes (the parent ones) don't have visibility maps
 create table test_partitioned (a int) partition by list (a);
+create index test_partitioned_index on test_partitioned (a);
 -- these should all fail
 select pg_visibility('test_partitioned', 0);
+select pg_visibility('test_partitioned_index', 0);
 select pg_visibility_map('test_partitioned');
+select pg_visibility_map('test_partitioned_index');
 select pg_visibility_map_summary('test_partitioned');
+select pg_visibility_map_summary('test_partitioned_index');
 select pg_check_frozen('test_partitioned');
+select pg_check_frozen('test_partitioned_index');
 select pg_truncate_visibility_map('test_partitioned');
+select pg_truncate_visibility_map('test_partitioned_index');
 
 create table test_partition partition of test_partitioned for values in (1);
 create index test_index on test_partition (a);
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 9858ea69d4..96311b8386 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -160,14 +160,24 @@ select pgstattuple('test_partitioned_index');
 ERROR:  "test_partitioned_index" (partitioned index) is not supported
 select pgstattuple_approx('test_partitioned');
 ERROR:  "test_partitioned" is not a table or materialized view
+select pgstattuple_approx('test_partitioned_index');
+ERROR:  "test_partitioned_index" 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
+select pg_relpages('test_partitioned_index');
+ERROR:  "test_partitioned_index" is not a table, index, materialized view, sequence, or TOAST table
 select pgstatindex('test_partitioned');
 ERROR:  relation "test_partitioned" is not a btree index
+select pgstatindex('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a btree index
 select pgstatginindex('test_partitioned');
 ERROR:  relation "test_partitioned" is not a GIN index
+select pgstatginindex('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a GIN index
 select pgstathashindex('test_partitioned');
 ERROR:  "test_partitioned" is not an index
+select pgstathashindex('test_partitioned_index');
+ERROR:  relation "test_partitioned_index" is not a hash index
 create view test_view as select 1;
 -- these should all fail
 select pgstattuple('test_view');
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index cfa540302d..1459672cab 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -69,10 +69,15 @@ create index test_partitioned_index on test_partitioned(a);
 select pgstattuple('test_partitioned');
 select pgstattuple('test_partitioned_index');
 select pgstattuple_approx('test_partitioned');
+select pgstattuple_approx('test_partitioned_index');
 select pg_relpages('test_partitioned');
+select pg_relpages('test_partitioned_index');
 select pgstatindex('test_partitioned');
+select pgstatindex('test_partitioned_index');
 select pgstatginindex('test_partitioned');
+select pgstatginindex('test_partitioned_index');
 select pgstathashindex('test_partitioned');
+select pgstathashindex('test_partitioned_index');
 
 create view test_view as select 1;
 -- these should all fail
-- 
2.11.0

#11

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Alvaro Herrera (#10)

Re: [SPAM] Re: Local partitioned indexes and pageinspect

On Wed, May 09, 2018 at 02:28:50PM -0300, Alvaro Herrera wrote:

I pushed some fixes produced here. Attached is the remainder of the
patch you submitted. I notice now that we haven't actually fixed
Peter's source of complaint, though. AFAICS your patch just adds test
cases, and upthread discussion apparently converges on not doing
anything about the code. I'm not yet sure what to think of that ...

Thanks Álvaro. I tend to think that there is little point in changing
the error handling, still it would be good to get test coverage. That's
not really a bug though as in all those cases we don't get errors like
"could not open file" or such. So we could also let things as they are
now.
--
Michael

#12

Peter Geoghegan

pg@bowt.ie

over 7 years ago

In reply to: Michael Paquier (#11)

Re: [SPAM] Re: Local partitioned indexes and pageinspect

On Wed, May 9, 2018 at 2:04 PM, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 09, 2018 at 02:28:50PM -0300, Alvaro Herrera wrote:

I pushed some fixes produced here. Attached is the remainder of the
patch you submitted. I notice now that we haven't actually fixed
Peter's source of complaint, though. AFAICS your patch just adds test
cases, and upthread discussion apparently converges on not doing
anything about the code. I'm not yet sure what to think of that ...

Thanks Álvaro. I tend to think that there is little point in changing
the error handling, still it would be good to get test coverage. That's
not really a bug though as in all those cases we don't get errors like
"could not open file" or such. So we could also let things as they are
now.

Now that the relkind issue is documented, I wouldn't mind just leaving it as-is.

--
Peter Geoghegan