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

Hi,

Here's a tiny cleanup: currently get_tables_to_cluster uses
heap_open() to open the relation, but then closes it with
relation_close(). Currently relation_close=heap_close, but it seems
like good idea to be consistent -- in case these functions need to
diverge in the future.

Regards,
Marti

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 349d130..a10ec2d 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1551,7 +1551,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
     }
     heap_endscan(scan);

-    relation_close(indRelation, AccessShareLock);
+    heap_close(indRelation, AccessShareLock);

     return rvs;
 }

On Mon, Feb 27, 2012 at 5:45 AM, Marti Raudsepp <marti@juffo.org> wrote:
> Here's a tiny cleanup: currently get_tables_to_cluster uses
> heap_open() to open the relation, but then closes it with
> relation_close(). Currently relation_close=heap_close, but it seems
> like good idea to be consistent -- in case these functions need to
> diverge in the future.

I'm inclined to fix this in the opposite way as what you've proposed:
replace heap_open() with relation_open(), rather than relation_close()
with heap_close().  The only thing heap_open() does that
relation_open() doesn't do is check the relkind, which is superfluous
here anyway; and if the check weren't superfluous it would most likely
be wrong, because heap_open rejects only some, not all, of the things
that aren't heaps.  During some of the DDL cleanup that I've been
doing during the 9.2 cycle, I've already found some cases where
careless use of heap_open rather than or in addition to an explicit
relkind check led to crappy error messages; the idea that there is
some systematic usefulness to a function that rejects indexes and
composite types (but not views, foreign tables, or sequences) doesn't
seem to be well-founded; the actual needs of people opening relations
are much more variable than that.  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.

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

Robert Haas <robertmhaas@gmail.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')?

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.

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.
        regards, tom lane

On Mon, Feb 27, 2012 at 11:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.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