Re: [HACKERS] contrib modules and relkind check
От | Amit Langote |
---|---|
Тема | Re: [HACKERS] contrib modules and relkind check |
Дата | |
Msg-id | 210da917-0a18-1402-1425-d319a0b08555@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: [HACKERS] contrib modules and relkind check (Michael Paquier <michael.paquier@gmail.com>) |
Ответы |
Re: [HACKERS] contrib modules and relkind check
(Michael Paquier <michael.paquier@gmail.com>)
|
Список | pgsql-hackers |
On 2017/02/10 14:32, Michael Paquier wrote: > On Thu, Feb 9, 2017 at 7:21 AM, Corey Huinker <corey.huinker@gmail.com> wrote: Thanks Corey and Michael for the reviews! >> 1. should have these tests named in core functions, like maybe: >> >> relation_has_visibility(Relation) >> relation_has_storage(Relation) >> and/or corresponding void functions check_relation_has_visibility() and >> check_relation_has_storage() which would raise the appropriate error message >> when the boolean test fails. >> Then again, this might be overkill since we don't add new relkinds very >> often. > > The visibility checks are localized in pg_visibility.c and the storage > checks in pgstatindex.c, so yes we could have macros in those files. > Or even better: just have a sanity check routine as the error messages > are the same everywhere. If I understand correctly what's being proposed here, tablecmds.c has something called ATWrongRelkindError() which sounds (kind of) similar. It's called by ATSimplePermissions() whenever it finds out that relkind of the table specified in a given ALTER TABLE command is not in the "allowed relkind targets" for the command. Different ALTER TABLE commands call ATSimplePermissions() with an argument that specifies the "allowed relkind targets" for each command. I'm not saying that it should be used elsewhere, but if we do invent a new centralized routine for relkind checks, it would look similar. >> 2. Is it better stylistically to have an AND-ed list of != tests, or a >> negated list of OR-ed equality tests, and should the one style be changed to >> conform to the other? I changed the patch so that all newly added checks use an AND-ed list of != tests. >> No new regression tests. I think we should create a dummy partitioned table >> for each contrib and show that it fails. > > Yep, good point. That's easy enough to add. I added tests with a partitioned table to pageinspect's page.sql and pgstattuple.sql. I don't see any tests for pg_visibility module, maybe I should add? Although, I felt a bit uncomfortable the way the error message looks, for example: + -- check that using any of these functions with a partitioned table would fail + create table test_partitioned (a int) partition by range (a); + select pg_relpages('test_partitioned'); + ERROR: "test_partitioned" is not a table, index, materialized view, sequence, or TOAST table test_partitioned IS a table but just the kind without storage. This is not the only place however where we do not separate the check for partitioned tables from other unsupported relkinds in respective contexts. Not sure if that would be a better idea. > By the way, partition tables create a file on disk but they should > not... Amit, I think you are working on a patch for that as well? > That's tweaking heap_create() to unlist partitioned tables and then be > sure that other code paths don't try to look at the parent partitioned > table on disk. Yep, I just sent a message titled "Partitioned tables and relfilenode". Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Amit LangoteДата:
Сообщение: Re: [HACKERS] Documentation improvements for partitioning