Re: [PATCH] Use RelationClose rather than table_close in heap_create_with_catalog

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 于2023年5月11日周四 00:32写道：

> Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> writes:
> > And, the /* do not unlock till end of xact */, it looks like it's been
> > there from day 1. It may be indicating that the ref count fo the new
> > relation created in heap_create_with_catalog() will be decremented at
> > the end of xact, but I'm not sure what it means.
>
> Hmm, I think it's been copied-and-pasted from somewhere. It's quite
> common for us to not release locks on modified tables until end of
> transaction. However, that's not what's happening here, because we
> actually *don't have any such lock* at this point, as you can easily
> prove by stepping through this code and watching the contents of
> pg_locks from another session. We do acquire AccessExclusiveLock
> on the new table eventually, but not till control returns to
> DefineRelation.
>
> I'm not real sure that I like the proposed code change: it's unclear
> to me whether it's an unwise piercing of a couple of abstraction
> layers or an okay change because those abstraction layers haven't
> yet been applied to the new relation at all. However, I think the
> existing comment is actively misleading and needs to be changed.
> Maybe something like
>
> /*
> * Close the relcache entry, since we return only an OID not a
> * relcache reference. Note that we do not yet hold any lockmanager
> * lock on the new rel, so there's nothing to release.
> */
> table_close(new_rel_desc, NoLock);
>
> /*
> * ok, the relation has been cataloged, so close catalogs and return
> * the OID of the newly created relation.
> */
> table_close(pg_class_desc, RowExclusiveLock);
>
+1
Personally, I prefer above code.

Given these comments, maybe changing the first call to RelationClose
> would be sensible, but I'm still not quite convinced.
>
> regards, tom lane
>
>
>