From: Tom Lane <tgl@sss.pgh.pa.us>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
cc: Xiaoran Wang <wxiaoran@vmware.com>,
        "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Use RelationClose rather than table_close in heap_create_with_catalog
In-reply-to: <CALj2ACU_AvhYG=RK_CF7c3vnBOVyoozd8wzwOQ+KBuxrQa8yhQ@mail.gmail.com>
References: <DS0PR05MB968933DCAB6EC48776102A7BBA829@DS0PR05MB9689.namprd05.prod.outlook.com> <CALj2ACU_AvhYG=RK_CF7c3vnBOVyoozd8wzwOQ+KBuxrQa8yhQ@mail.gmail.com>
Comments: In-reply-to Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
	message dated "Wed, 10 May 2023 19:47:24 +0530"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <901572.1683736336.1@sss.pgh.pa.us>
Date: Wed, 10 May 2023 12:32:16 -0400
Message-ID: <901573.1683736336@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/901573.1683736336%40sss.pgh.pa.us>
Precedence: bulk

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.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);

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

			regards, tom lane