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

Re: get_whatever_oid, part 1: object types with unqualifed names

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: get_whatever_oid, part 1: object types with unqualifed names
Date: 2010-07-02 06:24:22
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers

I checked the v2 patch, but I could not find anything to comment
except for its patch format.
The -v2 patch still uses unified format, instead of context format.
I believe it is just a careless miss. Please fix it later, if necessary.

When I submit a patch, I makes it as follows :D
  git diff master my_branch | filterdiff --format=context > my_feature.patch

The transitions from object name to its oid are widely used in
the routines of the backend, so I also think this reworks will
enough worthwhile to keep the code clean.

So, I marked it as "ready for committer".


(2010/06/28 22:55), Robert Haas wrote:
> 2010/6/28 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> * ExecAlterOwnerStmt()
>> The original code uses get_roleid_checked() which does not allow invalid
>> username, but the new code gives missing_ok = true on the get_role_oid().
>> It should be fixed.
> Woops, good catch.  Fixed.
>> * assign_temp_tablespaces()?
>> It seems to me "if (!OidIsValid(curoid))" should be here, instead of comparison
>> with InvalidOid here. However, it may be cleaned up in any other patch instead
>> of get_whatever_oid() efforts.
> Yeah, we have a lot of places that check foo == InvalidOid or foo !=
> InvalidOid rather than using OidIsValid().  That might or might not be
> worth changing, but I don't see a strong need for this patch to change
> this particular instance.
>> * at the CreateRole(CreateRoleStmt *stmt)
>> I saw similar code which was replaced with the new APIs in this patch.
>> It seems to me "if (OidIsValid(get_role_oid(stmt->role, true)))" can be used,
>> and it enables to write the code more clean.
> I agree.  Changed.
>> * at the DefineOpFamily()
>> |     /* Get necessary info about access method */
>> |     tup = SearchSysCache1(AMNAME, CStringGetDatum(stmt->amname));
>> |     if (!HeapTupleIsValid(tup))
>> |         ereport(ERROR,
>> |                 (errcode(ERRCODE_UNDEFINED_OBJECT),
>> |                  errmsg("access method \"%s\" does not exist",
>> |                         stmt->amname)));
>> |
>> |     amoid = HeapTupleGetOid(tup);
>> |
>> |     /* XXX Should we make any privilege check against the AM? */
>> |
>> |     ReleaseSysCache(tup);
>> It can be replaced by get_am_oid(stmt->amname, false).
> I agree, done.  In fact, aren't we leaking a syscache reference here?
> And if so, should we fix it in HEAD and the backbranches also?  I'm
> tempted not to bother because, after all, how often do you invoke
> DefineOpFamily(), but maybe someone else has a different opinion?
>> * at the RenameSchema()
>> |     /* make sure the new name doesn't exist */
>> |     if (HeapTupleIsValid(
>> |                          SearchSysCache1(NAMESPACENAME,
>> |                                          CStringGetDatum(newname))))
>> |         ereport(ERROR,
>> |                 (errcode(ERRCODE_DUPLICATE_SCHEMA),
>> |                  errmsg("schema \"%s\" already exists", newname)));
>> It is similar to the case of CreateRole(). Does get_namespace_oid()
>> enables to write the code more clean?
> This looks like another syscache reference leak.  I have changed it to
> use get_namespace_oid() in the patch, but we need to decide whether to
> fix this in pre-9.1 releases.

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

In response to


pgsql-hackers by date

Next:From: KaiGai KoheiDate: 2010-07-02 07:35:50
Subject: Re: server authentication over Unix-domain sockets
Previous:From: Tom LaneDate: 2010-07-02 04:13:10
Subject: Re: failover vs. read only queries

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