Thread: Some RELKIND macro refactoring
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
Attachment
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/
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
Attachment
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.
Attachment
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
Attachment
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/
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.
Attachment
Small rebase of this patch set.
Attachment
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
Attachment
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.
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/
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/
Attachment
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)
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
Attachment
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.
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.