From: | Xiaoran Wang <wxiaoran(at)vmware(dot)com> |
---|---|
To: | tender wang <tndrwang(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] Use RelationClose rather than table_close in heap_create_with_catalog |
Date: | 2023-05-13 04:03:53 |
Message-ID: | DS0PR05MB9689BA4C4B3E4633DC0A6250BA7A9@DS0PR05MB9689.namprd05.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)gmail(dot)com>
Sent: Thursday, May 11, 2023 3:26 PM
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>; Xiaoran Wang <wxiaoran(at)vmware(dot)com>; pgsql-hackers(at)lists(dot)postgresql(dot)org <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] Use RelationClose rather than table_close in heap_create_with_catalog
!! External Email
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us<mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us>> 于2023年5月11日周四 00:32写道:
Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com<mailto: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
!! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.
From | Date | Subject | |
---|---|---|---|
Next Message | John Naylor | 2023-05-13 04:13:50 | Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound |
Previous Message | Peter Geoghegan | 2023-05-13 02:40:26 | Re: Overhauling "Routine Vacuuming" docs, particularly its handling of freezing |