Re: [PATCH] Cleanup: use heap_open/heap_close consistently

On Mon, Feb 27, 2012 at 11:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'm almost inclined to think that
>> we should be trying to get rid of heap_open() altogether; there are
>> already plenty of places that assume that opening the relation is as
>> good as opening the heap, so I don't think there'd be any real loss of
>> abstraction.
>
> Or, perhaps, restrict it to open actual heaps (ie, relkind 'r')?

That carries a significant risk of breaking third-party code; or even
core code. I'm almost positive that there is core code that relies on
heap_open's failure to reject all relkinds other than 'r'. We can go
through all the callers and audit them, but there's a non-trivial risk
of breaking something.

> I think that if you count, you'll find the vast majority of heap_open
> calls are really opening system catalogs.  So I'd just as soon have
> a relkind check there for sanity's sake, not to mention that renaming
> them all creates a lot of unnecessary code churn.

The code churn does suck. I have to admit, though, that I'd really
like to get out from under the pairing requirement: we've pretty much
already committed ourselves to a future where heap_close() can never
be anything more than relation_close(). Like it or not, that die is
cast. In doing the DDL refactoring that I undertook this release
cycle, I found that it was often necessary to switch from using
heap_openrv() to RangeVarGetRelid + relation_open(). That of course
means tracking down the corresponding heap_close() calls and making
them relation_close(). I think it might be better to bite the bullet
and just do that across the board. In the long run I think life will
be simpler with just one way to do it (Perl slogans nonwithstanding).

> IMO it would be sensible for heap_open to insist on a heap, index_open
> to insist on an index, and for anything else, use relation_open and
> BYO relkind check.  There are a few common patterns (eg "does relation
> have storage") that we should abstract somehow, but it might be better
> to provide separate relkind-check routines than to invent xxx_open.

Definitely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company