Skip site navigation (1) Skip section navigation (2)

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

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Marti Raudsepp <marti(at)juffo(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Cleanup: use heap_open/heap_close consistently
Date: 2012-02-27 17:35:28
Message-ID: CA+TgmoanRXV6Fr-gk-RyrGFEBx3vqyZJvxPg9=5=gd6CKXHGxA@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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

In response to

pgsql-hackers by date

Next:From: Pavan DeolaseeDate: 2012-02-27 17:43:39
Subject: Re: How to know a table has been modified?
Previous:From: Tom LaneDate: 2012-02-27 17:34:31
Subject: Re: pgstat documentation tables

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group