Re: [HACKERS] contrib modules and relkind check
On Thu, Mar 9, 2017 at 7:23 PM, Michael Paquier 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
Re: [HACKERS] contrib modules and relkind check
On Fri, Mar 10, 2017 at 9:34 AM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@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
Re: [HACKERS] contrib modules and relkind check
Michael, * Michael Paquier (michael.paqu...@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 signature.asc Description: Digital signature
Re: [HACKERS] contrib modules and relkind check
On Fri, Mar 10, 2017 at 8:59 AM, Amit Langote wrote: > Hi Stephen, > > On 2017/03/10 6:48, Stephen Frost wrote: >> Amit, Michael, >> >> * Amit Langote (langote_amit...@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
Re: [HACKERS] contrib modules and relkind check
Hi Stephen, On 2017/03/10 6:48, Stephen Frost wrote: > Amit, Michael, > > * Amit Langote (langote_amit...@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
Re: [HACKERS] contrib modules and relkind check
Amit, Michael, * Amit Langote (langote_amit...@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 signature.asc Description: Digital signature
Re: [HACKERS] contrib modules and relkind check
Amit, Michael, * Amit Langote (langote_amit...@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 > > 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 signature.asc Description: Digital signature
Re: [HACKERS] 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 > 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 >From 76c0c1903442ef41bd4a6e15f5b1672d4464c90c Mon Sep 17 00:00:00 2001 From: amit 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 00..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 00..4496fe36f8 --- /dev/null +++ b/contrib/pg_visibility/expected/pg_visibility.out @@ -0,0 +1,1
Re: [HACKERS] contrib modules and relkind check
On Wed, Mar 8, 2017 at 5:10 PM, Amit Langote 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
Re: [HACKERS] 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 >From 9679618d65b00d65effdd5de019410998e10b3d9 Mon Sep 17 00:00:00 2001 From: amit 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 00..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 00..699d2131b9 --- /dev/null +++ b/contrib/pg_vi
Re: [HACKERS] contrib modules and relkind check
On Tue, Mar 7, 2017 at 4:15 PM, Amit Langote 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
Re: [HACKERS] 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 >From c7762d5bfd9363150cdb94030a47cef252697672 Mon Sep 17 00:00:00 2001 From: amit 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); +
Re: [HACKERS] contrib modules and relkind check
On Tue, Mar 7, 2017 at 3:10 AM, Corey Huinker 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
Re: [HACKERS] contrib modules and relkind check
On Tue, Feb 14, 2017 at 1:30 AM, Michael Paquier 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?
Re: [HACKERS] contrib modules and relkind check
On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote 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
Re: [HACKERS] 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 materializ
Re: [HACKERS] contrib modules and relkind check
On Fri, Feb 10, 2017 at 4:29 PM, Amit Langote wrote: > On 2017/02/10 14:32, Michael Paquier wrote: >> On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker >> 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
Re: [HACKERS] 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 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 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\"", Relatio
Re: [HACKERS] contrib modules and relkind check
On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker 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
Re: [HACKERS] contrib modules and relkind check
On Mon, Feb 6, 2017 at 4:01 AM, Amit Langote 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 > >> 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.
Re: [HACKERS] 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 >> 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
Re: [HACKERS] 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 > 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
Re: [HACKERS] contrib modules and relkind check
On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote 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
[HACKERS] contrib modules and relkind check
Some contrib functions fail to fail sooner when relations of unsupported relkinds are passed, resulting in error message like one below: create table foo (a int); create view foov as select * from foo; select pg_visibility('foov', 0); ERROR: could not open file "base/13123/16488": No such file or directory Attached patch fixes that for all such functions I could find in contrib. It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple of places (in pageinspect and pgstattuple). Thanks, Amit >From 9324727c58f2debb45c7ecd6d620a3fe8b2a Mon Sep 17 00:00:00 2001 From: amit 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 = makeRangeVarFromNameL