Thread
Commits
-
Some RELKIND macro refactoring
- 37b2764593c0 15.0 landed
-
pg_dump: Add missing relkind case
- a22d6a2cb62c 15.0 landed
-
Fix handling of partitioned index in RelationGetNumberOfBlocksInFork()
- 0d906b2c0b1f 15.0 landed
-
Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-16T13:20:07Z
Attached patches introduce more macros to group some RELKIND_* macros: - RELKIND_HAS_PARTITIONS() - RELKIND_HAS_TABLESPACE() - RELKIND_HAS_TABLE_AM() - RELKIND_HAS_INDEX_AM() I collected those mainly while working on the relkind error messages patch [0]. I think they improve the self-documentation of the code in many places that are currently just random collections or endless streams of RELKIND macros. Some may recall the previous thread [1] that made a similar proposal. The result here was that those macros were too thinly sliced and not generally useful enough. My proposal is completely separate from that. [0]: https://www.postgresql.org/message-id/flat/dc35a398-37d0-75ce-07ea-1dd71d98f8ec@2ndquadrant.com [1]: https://www.postgresql.org/message-id/flat/CAFjFpRcfzs%2Byst6YBCseD_orEcDNuAr9GUTraZ5GC%3DAvCYh55Q%40mail.gmail.com
-
Re: Some RELKIND macro refactoring
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-08-16T14:22:50Z
On 2021-Aug-16, Peter Eisentraut wrote: > + if (rel->rd_rel->relkind == RELKIND_INDEX || > + rel->rd_rel->relkind == RELKIND_SEQUENCE) > + RelationCreateStorage(rel->rd_node, relpersistence); > + else if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) > + table_relation_set_new_filenode(rel, &rel->rd_node, > + relpersistence, > + relfrozenxid, relminmxid); > + else > + Assert(false); I think you could turn this one (and the one in RelationSetNewRelfilenode) around: if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) table_relation_set_new_filenode(...); else if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) RelationCreateStorage(...); > +/* > + * Relation kinds with a table access method (rd_tableam). Although sequences > + * use the heap table AM, they are enough of a special case in most uses that > + * they are not included here. > + */ > +#define RELKIND_HAS_TABLE_AM(relkind) \ > + ((relkind) == RELKIND_RELATION || \ > + (relkind) == RELKIND_TOASTVALUE || \ > + (relkind) == RELKIND_MATVIEW) Partitioned tables are not listed here, but IIRC there's a patch to let partitioned tables have a table AM so that their partitions inherit it. I'm wondering if that patch is going to have to change this spot; and if it does, how will that affect the callers of this macro that this patch creates. I think a few places assume that HAS_TABLE_AM means that the table has storage, but perhaps it would be better to spell that out explicitly: @@ -303,9 +303,7 @@ verify_heapam(PG_FUNCTION_ARGS) /* * Check that a relation's relkind and access method are both supported. */ - if (ctx.rel->rd_rel->relkind != RELKIND_RELATION && - ctx.rel->rd_rel->relkind != RELKIND_MATVIEW && - ctx.rel->rd_rel->relkind != RELKIND_TOASTVALUE) + if (!(RELKIND_HAS_TABLE_AM(ctx.rel->rd_rel->relkind) && RELKIND_HAS_STOAGE(ctx.rel->rd_rel->relkind))) ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot check relation \"%s\"", -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ -
Re: Some RELKIND macro refactoring
Michael Paquier <michael@paquier.xyz> — 2021-08-17T02:59:53Z
On Mon, Aug 16, 2021 at 10:22:50AM -0400, Alvaro Herrera wrote: > Partitioned tables are not listed here, but IIRC there's a patch to let > partitioned tables have a table AM so that their partitions inherit it. This was raised in the thread for ALTER TABLE SET ACCESS METHOD (see patch 0002): https://www.postgresql.org/message-id/20210308010707.GA29832@telsasoft.com I am not sure whether we'd want to do that for table AMs as property inheritance combined with partitioned tables has always been a sensitive topic for any properties, but if we discuss this it should be on a new thread. -- Michael
-
Re: Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-24T10:01:33Z
While analyzing this again, I think I found an existing mistake. The handling of RELKIND_PARTITIONED_INDEX in RelationGetNumberOfBlocksInFork() seems to be misplaced. See attached patch.
-
Re: Some RELKIND macro refactoring
Michael Paquier <michael@paquier.xyz> — 2021-08-25T05:44:27Z
On Tue, Aug 24, 2021 at 12:01:33PM +0200, Peter Eisentraut wrote: > While analyzing this again, I think I found an existing mistake. The > handling of RELKIND_PARTITIONED_INDEX in RelationGetNumberOfBlocksInFork() > seems to be misplaced. See attached patch. Right. This maps with RELKIND_HAS_STORAGE(). Makes me wonder whether is would be better to add a check on RELKIND_HAS_STORAGE() in this area, even if that's basically the same as the Assert() already used in this code path. -- Michael
-
Re: Some RELKIND macro refactoring
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-08-25T13:31:46Z
On 2021-Aug-25, Michael Paquier wrote: > On Tue, Aug 24, 2021 at 12:01:33PM +0200, Peter Eisentraut wrote: > > While analyzing this again, I think I found an existing mistake. The > > handling of RELKIND_PARTITIONED_INDEX in RelationGetNumberOfBlocksInFork() > > seems to be misplaced. See attached patch. Agreed, that's a mistake. > Right. This maps with RELKIND_HAS_STORAGE(). Makes me wonder whether > is would be better to add a check on RELKIND_HAS_STORAGE() in this > area, even if that's basically the same as the Assert() already used > in this code path. Well, the patch replaces the switch on individual relkind values with if tests on RELKIND_HAS_FOO macros. I suppose we'd have Assert(RELKIND_HAS_STORAGE(relkind)); so the function would not even be called for partitioned indexes. (In a quick scan of 'git grep RelationGetNumberOfBlocks' I see nothing that would obviously call this function on a partitioned index.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "XML!" Exclaimed C++. "What are you doing here? You're not a programming language." "Tell that to the people who use me," said XML. https://burningbird.net/the-parable-of-the-languages/
-
Re: Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-30T08:17:30Z
On 16.08.21 16:22, Alvaro Herrera wrote: > On 2021-Aug-16, Peter Eisentraut wrote: > >> + if (rel->rd_rel->relkind == RELKIND_INDEX || >> + rel->rd_rel->relkind == RELKIND_SEQUENCE) >> + RelationCreateStorage(rel->rd_node, relpersistence); >> + else if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) >> + table_relation_set_new_filenode(rel, &rel->rd_node, >> + relpersistence, >> + relfrozenxid, relminmxid); >> + else >> + Assert(false); > > I think you could turn this one (and the one in RelationSetNewRelfilenode) around: > > if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) > table_relation_set_new_filenode(...); > else if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) > RelationCreateStorage(...); done >> +/* >> + * Relation kinds with a table access method (rd_tableam). Although sequences >> + * use the heap table AM, they are enough of a special case in most uses that >> + * they are not included here. >> + */ >> +#define RELKIND_HAS_TABLE_AM(relkind) \ >> + ((relkind) == RELKIND_RELATION || \ >> + (relkind) == RELKIND_TOASTVALUE || \ >> + (relkind) == RELKIND_MATVIEW) > > Partitioned tables are not listed here, but IIRC there's a patch to let > partitioned tables have a table AM so that their partitions inherit it. > I'm wondering if that patch is going to have to change this spot; and if > it does, how will that affect the callers of this macro that this patch > creates. Interesting. It would be useful to consider both patches together then, so that conceptual clarity is achieved. I will take a look. After some consideration, I have removed RELKIND_HAS_INDEX_AM(), since the way I had it and also considering that other patch effort, it didn't make much sense. > I think a few places assume that HAS_TABLE_AM means that the > table has storage, but perhaps it would be better to spell that out > explicitly: > > @@ -303,9 +303,7 @@ verify_heapam(PG_FUNCTION_ARGS) > /* > * Check that a relation's relkind and access method are both supported. > */ > - if (ctx.rel->rd_rel->relkind != RELKIND_RELATION && > - ctx.rel->rd_rel->relkind != RELKIND_MATVIEW && > - ctx.rel->rd_rel->relkind != RELKIND_TOASTVALUE) > + if (!(RELKIND_HAS_TABLE_AM(ctx.rel->rd_rel->relkind) && RELKIND_HAS_STOAGE(ctx.rel->rd_rel->relkind))) > ereport(ERROR, > (errcode(ERRCODE_WRONG_OBJECT_TYPE), > errmsg("cannot check relation \"%s\"", > I think, if something has a table AM, it implies that it has storage. In the contrib modules, the right way to phrase the check is sometimes not entirely clear, since part of the job of these modules is sometimes to bypass the normal APIs and do extra checks etc. -
Re: Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-01T06:28:16Z
Small rebase of this patch set.
-
Re: Some RELKIND macro refactoring
Michael Paquier <michael@paquier.xyz> — 2021-11-19T07:31:41Z
On Mon, Nov 01, 2021 at 07:28:16AM +0100, Peter Eisentraut wrote: > > Small rebase of this patch set. Regarding 0001, I find the existing code a bit more self-documenting if we keep those checks flagInhAttrs() and guessConstraintInheritance(). So I would rather leave these. I like 0002, which makes things a bit easier to go through across various places where relkind-only checks are replaced by those new macros. There is one thing I bumped into, though.. > if (create_storage) > { > - switch (rel->rd_rel->relkind) > - { > - case RELKIND_VIEW: > - case RELKIND_COMPOSITE_TYPE: > - case RELKIND_FOREIGN_TABLE: > - case RELKIND_PARTITIONED_TABLE: > - case RELKIND_PARTITIONED_INDEX: > - Assert(false); > - break; > - [ .. deletions .. ] > - } > + if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) > + table_relation_set_new_filenode(rel, &rel->rd_node, > + relpersistence, > + relfrozenxid, relminmxid); > + else > + RelationCreateStorage(rel->rd_node, relpersistence); > } I think that you should keep this block as it is now. The part where a relkind does not support table AMs and does not require storage would get uncovered, and this new code would just attempt to create storage, so it seems to me that the existing code is better when it comes to prevent mistakes from developers? You could as well use an else if (RELKIND_INDEX || RELKIND_SEQUENCE) for RelationCreateStorage(), and fall back to Assert(false) in the else branch, to get the same result as the original without impacting the level of safety of the original code. -- Michael -
Re: Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-22T10:21:52Z
On 19.11.21 08:31, Michael Paquier wrote: > On Mon, Nov 01, 2021 at 07:28:16AM +0100, Peter Eisentraut wrote: >> >> Small rebase of this patch set. > > Regarding 0001, I find the existing code a bit more self-documenting > if we keep those checks flagInhAttrs() and guessConstraintInheritance(). > So I would rather leave these. In that case, the existing check in guessConstraintInheritance() seems wrong, because it doesn't check for RELKIND_MATVIEW. Should we fix that? It's dead code either way, but if the code isn't exercises, then these kinds of inconsistency come about. > I like 0002, which makes things a bit easier to go through across > various places where relkind-only checks are replaced by those new > macros. There is one thing I bumped into, though.. > >> if (create_storage) >> { >> - switch (rel->rd_rel->relkind) >> - { >> - case RELKIND_VIEW: >> - case RELKIND_COMPOSITE_TYPE: >> - case RELKIND_FOREIGN_TABLE: >> - case RELKIND_PARTITIONED_TABLE: >> - case RELKIND_PARTITIONED_INDEX: >> - Assert(false); >> - break; >> - [ .. deletions .. ] >> - } >> + if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) >> + table_relation_set_new_filenode(rel, &rel->rd_node, >> + relpersistence, >> + relfrozenxid, relminmxid); >> + else >> + RelationCreateStorage(rel->rd_node, relpersistence); >> } > > I think that you should keep this block as it is now. The part where > a relkind does not support table AMs and does not require storage > would get uncovered, and this new code would just attempt to create > storage, so it seems to me that the existing code is better when it > comes to prevent mistakes from developers? You could as well use an > else if (RELKIND_INDEX || RELKIND_SEQUENCE) for > RelationCreateStorage(), and fall back to Assert(false) in the else > branch, to get the same result as the original without impacting the > level of safety of the original code. Maybe else { Assert(RELKIND_HAS_STORAGE(rel->rd_rel->relkind); RelationCreateStorage(rel->rd_node, relpersistence); } create_storage is set earlier based on RELKIND_HAS_STORAGE(), so this would be consistent. -
Re: Some RELKIND macro refactoring
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-23T15:09:00Z
On 2021-Nov-19, Michael Paquier wrote: > I think that you should keep this block as it is now. The part where > a relkind does not support table AMs and does not require storage > would get uncovered, and this new code would just attempt to create > storage, so it seems to me that the existing code is better when it > comes to prevent mistakes from developers? You could as well use an > else if (RELKIND_INDEX || RELKIND_SEQUENCE) for > RelationCreateStorage(), and fall back to Assert(false) in the else > branch, to get the same result as the original without impacting the > level of safety of the original code. Hmm, I think the added condition and else would make it safe and clear: + if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) + table_relation_set_new_filenode(rel, &rel->rd_node, + relpersistence, + relfrozenxid, relminmxid); + else if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) + RelationCreateStorage(rel->rd_node, relpersistence); + else + Assert(false); This is the same coding the patch proposed to put in RelationSetNewRelfilenode, which IMO validates the idea. In init_fork(), there's a typo: + * For tables, the AM callback creates both the main and the init fork. should read: + * For tables, the AM callback creates both the main and the init forks. In heap_create_with_catalog, you have + if (!relid) should be + if (!OidIsValid(relid)) Overall, LGTM. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
-
Re: Some RELKIND macro refactoring
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-23T15:19:13Z
On 2021-Nov-22, Peter Eisentraut wrote: > Maybe > > else > { > Assert(RELKIND_HAS_STORAGE(rel->rd_rel->relkind); > RelationCreateStorage(rel->rd_node, relpersistence); > } > > create_storage is set earlier based on RELKIND_HAS_STORAGE(), so this would > be consistent. Hmm, right ... but I think we can make this a little simpler. How about the attached? -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ -
Re: Some RELKIND macro refactoring
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-11-23T15:36:15Z
On 2021-Nov-23, Alvaro Herrera wrote: > On 2021-Nov-22, Peter Eisentraut wrote: > > > Maybe > > > > else > > { > > Assert(RELKIND_HAS_STORAGE(rel->rd_rel->relkind); > > RelationCreateStorage(rel->rd_node, relpersistence); > > } > > > > create_storage is set earlier based on RELKIND_HAS_STORAGE(), so this would > > be consistent. > > Hmm, right ... but I think we can make this a little simpler. How about > the attached? This doesn't actually work, so nevermind that. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ Essentially, you're proposing Kevlar shoes as a solution for the problem that you want to walk around carrying a loaded gun aimed at your foot. (Tom Lane) -
Re: Some RELKIND macro refactoring
Michael Paquier <michael@paquier.xyz> — 2021-11-24T04:20:29Z
On Mon, Nov 22, 2021 at 11:21:52AM +0100, Peter Eisentraut wrote: > On 19.11.21 08:31, Michael Paquier wrote: >> Regarding 0001, I find the existing code a bit more self-documenting >> if we keep those checks flagInhAttrs() and guessConstraintInheritance(). >> So I would rather leave these. > > In that case, the existing check in guessConstraintInheritance() seems > wrong, because it doesn't check for RELKIND_MATVIEW. Should we fix that? > It's dead code either way, but if the code isn't exercises, then these kinds > of inconsistency come about. Yeah, this one could be added. Perhaps that comes down to one's taste at the end, but I would add it. > Maybe > > else > { > Assert(RELKIND_HAS_STORAGE(rel->rd_rel->relkind); > RelationCreateStorage(rel->rd_node, relpersistence); > } > > create_storage is set earlier based on RELKIND_HAS_STORAGE(), so this would > be consistent. Sounds fine by me. Perhaps you should apply the same style in RelationGetNumberOfBlocksInFork(), then? -- Michael -
Re: Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-02T15:56:02Z
On 24.11.21 05:20, Michael Paquier wrote: > On Mon, Nov 22, 2021 at 11:21:52AM +0100, Peter Eisentraut wrote: >> On 19.11.21 08:31, Michael Paquier wrote: >>> Regarding 0001, I find the existing code a bit more self-documenting >>> if we keep those checks flagInhAttrs() and guessConstraintInheritance(). >>> So I would rather leave these. >> In that case, the existing check in guessConstraintInheritance() seems >> wrong, because it doesn't check for RELKIND_MATVIEW. Should we fix that? >> It's dead code either way, but if the code isn't exercises, then these kinds >> of inconsistency come about. > Yeah, this one could be added. Perhaps that comes down to one's taste > at the end, but I would add it. Ok, I have committed adding the missing relkind, as you suggest. Patch v3-0001 is therefore obsolete.
-
Re: Some RELKIND macro refactoring
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-03T13:12:35Z
On 23.11.21 16:09, Alvaro Herrera wrote: > In init_fork(), there's a typo: > > + * For tables, the AM callback creates both the main and the init fork. > should read: > + * For tables, the AM callback creates both the main and the init forks. I believe the original wording is correct. > Overall, LGTM. Committed with the (other) suggested adjustments.