Re: don't create storage when unnecessary - Mailing list pgsql-hackers
From | Kyotaro HORIGUCHI |
---|---|
Subject | Re: don't create storage when unnecessary |
Date | |
Msg-id | 20181218.145600.172055615.horiguchi.kyotaro@lab.ntt.co.jp Whole thread Raw |
In response to | Re: don't create storage when unnecessary (Alvaro Herrera <alvherre@2ndquadrant.com>) |
Responses |
Re: don't create storage when unnecessary
(Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
|
List | pgsql-hackers |
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. 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
pgsql-hackers by date: