Re: RangeVarGetRelid()

On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011:
>> The trouble is, I'm not quite sure how to do that.  It seems like
>> permissions checks and lock-the-heap-for-this-index should be done in
>> RangeVarGetRelid() just after the block that says "if (retry)" and
>> just before the block that calls LockRelationOid().  That way, if we
>> end up deciding we need to retry the name lookup, we'll retry all that
>> other stuff as well, which is exactly right.  The difficulty is that
>> different callers have different needs for what should go in that
>> space, to the degree that I'm a bit nervous about continuing to add
>> arguments to that function to satisfy what everybody needs.  Maybe we
>> could make most of them Booleans and pass an "int flags" argument.
>> Another option would be to add a "callback" argument to that function
>> that would be called at that point with the relation, relId, and
>> oldRelId as arguments.  Alternatively, we could just resign ourselves
>> to duplicating the loop in this function into each place in the code
>> that has a special-purpose requirement, but the function is complex
>> enough to make that a pretty unappealing prospect.
>
> I'm for the callback.

I worked up the attached patch showing how this might work. For
demonstration purposes, the attached patch modifies REINDEX INDEX and
REINDEX TABLE to use this facility. It turns out that, in the current
code, they have opposite problems, so this is a good example of how
this gives you the best of both worlds. Suppose we do:

rhaas=# create table foo (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"foo_pkey" for table "foo"
CREATE TABLE
rhaas=# begin;
BEGIN
rhaas=# insert into foo values (1);
INSERT 0 1

At this point, if we start up another session as non-privileged user,
REINDEX INDEX foo_pkey will immediately fail with a permissions error
(which is good), but REINDEX TABLE foo will queue up for a table lock
(which is bad). Now suppose we set up like this:

rhaas=# create table foo (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"foo_pkey" for table "foo"
CREATE TABLE
rhaas=# begin;
BEGIN
rhaas=# drop table foo;
DROP TABLE
rhaas=# create table foo (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"foo_pkey" for table "foo"
CREATE TABLE

If we open another session as superuser and say "REINDEX INDEX
foo_pkey", and then commit the open transaction in the first session,
it will fail:

rhaas=# reindex index foo_pkey;
ERROR: could not open relation with OID 16481

However, with the same setup, "REINDEX TABLE foo" will work.

With the attached patch, both situations are handled correctly by both commands.

Having now written the patch, I'm convinced that the callback is in
fact the right choice. It requires only very minor reorganization of
the existing code, which is always a plus.

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