[PATCH] Use RelationClose rather than table_close in heap_create_with_catalog

Xiaoran Wang <wxiaoran@vmware.com> 于2023年3月18日周六 15:04写道：

Hi hackers,

In heap_create_with_catalog, the Relation new_rel_desc is created
by RelationBuildLocalRelation, not table_open. So it's better to
call RelationClose to release it.

Why it's better to call RelationClose? Is there a problem if using
table_close()?

What's more, the comment for it seems useless, just delete it.

Thanks!

regard, tender wang

The routine table_close​ takes 2 params: Relation​ and LOCKMODE​, it first calls RelationClose to decrease the relation cache reference count, then deals with the lock on the table based on LOCKMOD​ param.

In heap_create_with_catalog, the Relation new_rel_desc is only a local relation cache, created by RelationBuildLocalRelation. No other processes can see this relation, as the transaction is not committed, so there is no lock on it.

There is no problem to release the relation cache by table_close(new_rel_desc, NoLock) here. However, from my point of view, table_close(new_rel_desc, NoLock); /* do not unlock till end of xact */​
this line is a little confusing since there is no lock on the relation at all. So I think it's better to use RelationColse here.
________________________________
From: tender wang <tndrwang@gmail.com>
Sent: Wednesday, May 10, 2023 10:57 AM
To: Xiaoran Wang <wxiaoran@vmware.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Use RelationClose rather than table_close in heap_create_with_catalog

!! External Email

Xiaoran Wang <wxiaoran@vmware.com<mailto:wxiaoran@vmware.com>> 于2023年3月18日周六 15:04写道：
Hi hackers,

In heap_create_with_catalog, the Relation new_rel_desc is created
by RelationBuildLocalRelation, not table_open. So it's better to
call RelationClose to release it.
Why it's better to call RelationClose? Is there a problem if using table_close()?
What's more, the comment for it seems useless, just delete it.

Thanks!

regard, tender wang

!! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.

On Sat, Mar 18, 2023 at 12:34 PM Xiaoran Wang <wxiaoran@vmware.com> wrote:

Hi hackers,

In heap_create_with_catalog, the Relation new_rel_desc is created
by RelationBuildLocalRelation, not table_open. So it's better to
call RelationClose to release it.

What's more, the comment for it seems useless, just delete it.

Essentially, all the close functions are the same with NoLock, IOW,
table_close(relation, NoLock) = relation_closerelation, NoLock) =
RelationClose(relation). Therefore, table_close(new_rel_desc, NoLock);
looks fine to me.

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.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 于2023年5月10日周三
22:17写道：

On Sat, Mar 18, 2023 at 12:34 PM Xiaoran Wang <wxiaoran@vmware.com> wrote:

Hi hackers,

In heap_create_with_catalog, the Relation new_rel_desc is created
by RelationBuildLocalRelation, not table_open. So it's better to
call RelationClose to release it.

What's more, the comment for it seems useless, just delete it.

Essentially, all the close functions are the same with NoLock, IOW,
table_close(relation, NoLock) = relation_closerelation, NoLock) =
RelationClose(relation). Therefore, table_close(new_rel_desc, NoLock);
looks fine to me.

Agreed.

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.

Me too

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

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

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);

+1
Personally, I prefer above code.

Given these comments, maybe changing the first call to RelationClose

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 <tndrwang@gmail.com>
Sent: Thursday, May 11, 2023 3:26 PM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; 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

!! External Email

Tom Lane <tgl@sss.pgh.pa.us<mailto:tgl@sss.pgh.pa.us>> 于2023年5月11日周四 00:32写道：
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com<mailto: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);
+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.