Hi,
On 2018/12/18 14:56, Kyotaro HORIGUCHI wrote:
> Hello.
>
> At Sun, 16 Dec 2018 17:47:16 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20181216204716.jddzijk3fb2dpdk4@alvherre.pgsql>
>> On 2018-Dec-07, Michael Paquier wrote:
>>
>>> A macro makes sense to control that.
>>
>> I added Ashutosh's RELKIND_HAS_STORAGE, but renamed it to
>> RELKIND_CAN_HAVE_STORAGE, because some of the relkinds can be mapped and
>> thus would have relfilenode set to 0. I think this is a bit misleading
>> either way.
>
> FWIW.. I RELKIND_HAS_STORAGE looks better to me.
>
> # Since it's a bit too late, I don't insist on that.
>
> Mapped relations have storage, which is signalled by relfilenode
> = 0 and the real file node is given by relation mapper. And it is
> actually created at the boostrap time. In this sense,
> (RELKIND_HAS_STORAGE && relfilenode == 0) in heap_craete makes
> sense.
>
> Assertion (..->relNode != InvalidOid) at the end of
> RelationInitPhysicalAddr doesn't fail. I think RELKIND_HAS_STORGE
> is preferable also in this sense.
Sorry to be saying it late, but I have to agree with Horiguchi-san here
that RELKIND_HAS_STORAGE sounds better and is clear. Looking at what's
committed:
/*
* Relation kinds that have physical storage. These relations normally have
* relfilenode set to non-zero, but it can also be zero if the relation is
* mapped.
*/
#define RELKIND_CAN_HAVE_STORAGE(relkind) \
((relkind) == RELKIND_RELATION || \
(relkind) == RELKIND_INDEX || \
(relkind) == RELKIND_SEQUENCE || \
(relkind) == RELKIND_TOASTVALUE || \
(relkind) == RELKIND_MATVIEW)
So, they all *do have* storage, as the comment's first sentence also says.
Mixing the bit about mapped relations in the naming of this macro will
make it hard to reason about using it in other parts of the backend code
(although, in admittedly not too far off places), where it shouldn't
matter if the relation's file is determined using relfilenode or via
relation to file mapper.
> ATExecSetTableSpaceNoStorage assumes that the relation is
> actually having storage.
Hmm, it has the following Assert:
/*
* Shouldn't be called on relations having storage; these are processed
* in phase 3.
*/
Assert(!RELKIND_CAN_HAVE_STORAGE(rel->rd_rel->relkind));
So, it actually assumes that it's called for relations that don't have
storage (...but do have a tablespace property that applies to its children.)
Thanks,
Amit