On 6/3/07, Neil Conway <[EMAIL PROTECTED]> wrote:

On Mon, 2007-21-05 at 12:23 +0530, NikhilS wrote:
> I had spent some time on this earlier so decided to complete and send
> the patch to you for review. This patch supports copying of
> expressions, predicates, opclass, amorder, reloptions etc. The test
> case also contains some more additions with this patch. Please let me
> know if there are any issues.

Attached is a revised version of this patch. Note that this pattern is
always unsafe:

       ht_am = SearchSysCache(AMOID, ...);
       if (!HeapTupleIsValid(ht_am))
               elog(ERROR, "...");
       amrec = (Form_pg_am) GETSTRUCT(ht_am);
       index->accessMethod = NameStr(amrec->amname);

       /* ... */

       return index;

Before calling ReleaseSysCache(), all the data you need from the
syscache entry needs to be deep-copied to allow subsequent access, but
NameStr() doesn't do a deep-copy. Adding "-DFORCE_CATCACHE_RELEASE" is a
useful way to catch these kinds of problems (I wonder if this is worth
adding to the default CFLAGS when assertions are enabled?)

I should have delved deep into the NameStr functionality myself for this. I
too agree that adding the flag makes sense when assertions are enabled.

I also made a bunch of editorial changes, including moving the
varattnos_map_schema() call out of the loop in transformInhRelation().

BTW, comments like "This function is based on code from ruleutils.c"
would be helpful for reviewers (whether in the patch itself or just in
the email containing the patch).

Yes I had this in mind but somehow forgot to mention this when I posted the
patch. I had done some changes in ruleutils.c specificly to take cognizance
of this fact.

EnterpriseDB               http://www.enterprisedb.com

Reply via email to