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.
> 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.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
