Thanks for all your responses. It seems better to change the comments on the code rather than call RelationClose here.
table_close(new_rel_desc, NoLock); /* do not unlock till end of xact */ Do I need to create another patch to fix the comments? Best regards, xiaoran ________________________________ From: tender wang <tndrw...@gmail.com> Sent: Thursday, May 11, 2023 3:26 PM To: Tom Lane <t...@sss.pgh.pa.us> Cc: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com>; Xiaoran Wang <wxiao...@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 !! External Email Tom Lane <t...@sss.pgh.pa.us<mailto:t...@sss.pgh.pa.us>> 于2023年5月11日周四 00:32写道： Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com<mailto:bharath.rupireddyforpostg...@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); +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 !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.
