Re: don't create storage when unnecessary

Hello.

At Sun, 16 Dec 2018 17:47:16 -0300, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote in <20181216204716(dot)jddzijk3fb2dpdk4(at)alvherre(dot)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.

ATExecSetTableSpaceNoStorage assumes that the relation is
actually having storage.

> > Now I have to admit that I don't
> > like your solution. Wouldn't it be cleaner to assign InvalidOid to
> > relfilenode in such cases? The callers of heap_create would need to be
> > made smarter when they now pass down a relfilenode (looking at you,
> > DefineIndex!), but that seems way more consistent to me.
>
> I don't follow. When a relfilenode is passed by callers, they indicate
> that the storage has already been created. Contrariwise, when a
> relation kind that *does* have storage but is not yet created, they
> pass InvalidOid as relfilenode, and heap_create is in charge of creating
> storage. Maybe I'm not quite seeing what problem you mean. Or I could
> add a separate boolean, but that seems pointless.
>
> Another possible improvement is to remove the create_storage boolean
>
> > Some tests would also be welcome.
>
> Added a test in sanity_check.sql that there's no relation with the
> relkinds that aren't supposed to have storage. Without the code fix it
> fails in current regression database, but in the failure result set
> there isn't any relation of kinds 'p' or 'I', so this isn't a terribly
> comprehensive test -- the query runs too early in the regression
> sequence. I'm not sure it's worth bothering further.

Actual files were not created even in the past, create_heap has
been just refactored, which looks fine. pg_class and relcache
entries no longer have false relfilenode, which looks fine. The
test should check only relfilenode won't be given to such
relation, which were given in the past, which looks fine.

The patch applies cleanly and passed the regression test.

regares.

--
Kyotaro Horiguchi
NTT Open Source Software Center