pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedCode comment change
6+ messages / 4 participants
[nested] [flat]
* Code comment change
@ 2019-06-23 09:21 Vik Fearing <vik.fearing@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Vik Fearing @ 2019-06-23 09:21 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
There is some language in a code comment that has been bothering me for
several years now. After pointing it out in a conversation off-list
recently, I figured it was past time to do something about it.
Patch attached.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachments:
[text/x-patch] code_comment_change.diff (779B, ../../150d3e9f-c7ec-3fb3-4fdb-def47c4144af@2ndquadrant.com/2-code_comment_change.diff)
download | inline diff:
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index de4d4efc46..03570300d8 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -268,7 +268,7 @@ _bt_update_meta_cleanup_info(Relation rel, TransactionId oldestBtpoXact,
* its location from the metadata page, and then read the root page
* itself. If no root page exists yet, we have to create one. The
* standard class of race conditions exists here; I think I covered
- * them all in the Hopi Indian rain dance of lock requests below.
+ * them all in the intricate dance of lock requests below.
*
* The access type parameter (BT_READ or BT_WRITE) controls whether
* a new root page will be created or not. If access = BT_READ,
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Code comment change
@ 2019-06-23 10:35 Thomas Munro <thomas.munro@gmail.com>
parent: Vik Fearing <vik.fearing@2ndquadrant.com>
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Munro @ 2019-06-23 10:35 UTC (permalink / raw)
To: Vik Fearing <vik.fearing@2ndquadrant.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sun, Jun 23, 2019 at 9:21 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
> There is some language in a code comment that has been bothering me for
> several years now. After pointing it out in a conversation off-list
> recently, I figured it was past time to do something about it.
>
> Patch attached.
Pushed. Thanks!
--
Thomas Munro
https://enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Code comment change
@ 2019-06-23 11:43 Vik Fearing <vik.fearing@2ndquadrant.com>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Vik Fearing @ 2019-06-23 11:43 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On 23/06/2019 12:35, Thomas Munro wrote:
> On Sun, Jun 23, 2019 at 9:21 PM Vik Fearing <vik.fearing@2ndquadrant.com> wrote:
>> There is some language in a code comment that has been bothering me for
>> several years now. After pointing it out in a conversation off-list
>> recently, I figured it was past time to do something about it.
>>
>> Patch attached.
>
> Pushed. Thanks!
Thank you!
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Code comment change
@ 2019-07-02 02:03 Peter Geoghegan <pg@bowt.ie>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 6+ messages in thread
From: Peter Geoghegan @ 2019-07-02 02:03 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Vik Fearing <vik.fearing@2ndquadrant.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Sun, Jun 23, 2019 at 3:36 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> Pushed. Thanks!
I wonder what the comment is supposed to mean.
I think that it's addressing the situation prior to commit 70508ba7aed
in 2003, which was the point when the "fast" root concept was
introduced. Prior to that commit, there was only what we would now
call a true root, and _bt_getroot() had to loop to make sure that it
reliably found it without deadlocking, while dealing with concurrent
splits. This was necessary because the old design also involved
maintaining a pointer to each page's parent in each page, which sounds
like a seriously bad approach to me.
I think that the whole sentence about "the standard class of race
conditions" should go. There is no more dance. Nothing in
_bt_getroot() is surprising to me. The other comments explain things
comprehensively.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Code comment change
@ 2019-07-02 02:28 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2019-07-02 02:28 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Vik Fearing <vik.fearing@2ndquadrant.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Peter Geoghegan <pg@bowt.ie> writes:
> On Sun, Jun 23, 2019 at 3:36 AM Thomas Munro <thomas.munro@gmail.com> wrote:
>> Pushed. Thanks!
> I wonder what the comment is supposed to mean.
> I think that it's addressing the situation prior to commit 70508ba7aed
> in 2003, which was the point when the "fast" root concept was
> introduced.
Yeah. I did some research into the provenance of that comment when
Thomas pushed the change. It's *old*. The whole para exists verbatim
in Postgres v4r2, src/backend/access/nbtree/nbtpage.c dated 1993-12-10
(in my copy of that tarball). The only change since then has been to
change the whitespace for 4-space tabs.
Even more interesting, the same para also exists verbatim in
v4r2's src/backend/access/nobtree/nobtpage.c, which is dated 1991-10-29
in the same tarball. (If you're wondering, "nobtree" seems to stand
for "no-overwrite btree"; so I suppose it went the way of all flesh
when Stonebraker lost interest in write-once mass storage.) So presumably
this comment dates back to some common ancestor of the mainline btree code
and the no-overwrite code, which must have been even older than the 1991
date.
This is only marginally relevant to what we should do about it today,
but I think it's reasonable to conclude that the current locking
considerations are nearly unrelated to what they were when the comment
was written.
> I think that the whole sentence about "the standard class of race
> conditions" should go. There is no more dance. Nothing in
> _bt_getroot() is surprising to me. The other comments explain things
> comprehensively.
+1
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Code comment change
@ 2019-07-02 03:09 Peter Geoghegan <pg@bowt.ie>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Peter Geoghegan @ 2019-07-02 03:09 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Vik Fearing <vik.fearing@2ndquadrant.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Mon, Jul 1, 2019 at 7:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Even more interesting, the same para also exists verbatim in
> v4r2's src/backend/access/nobtree/nobtpage.c, which is dated 1991-10-29
> in the same tarball. (If you're wondering, "nobtree" seems to stand
> for "no-overwrite btree"; so I suppose it went the way of all flesh
> when Stonebraker lost interest in write-once mass storage.) So presumably
> this comment dates back to some common ancestor of the mainline btree code
> and the no-overwrite code, which must have been even older than the 1991
> date.
"no-overwrite btree" is described here, if you're interested:
https://pdfs.semanticscholar.org/a0de/438d5efd96e8af51bc7595aa1c30d0497a57.pdf
This is a link to the B-Tree focused paper "An Index Implementation
Supporting Fast Recovery for the POSTGRES Storage System". I found
that the paper provided me with some interesting historic context. I
am pretty sure that the authors were involved in early work on the
Postgres B-Tree code. It references Lanin and Shasha, even though the
nbtree code that is influenced by L&S first appears in the same 2003
commit of yours that I mentioned.
> > I think that the whole sentence about "the standard class of race
> > conditions" should go. There is no more dance. Nothing in
> > _bt_getroot() is surprising to me. The other comments explain things
> > comprehensively.
>
> +1
I'll take care of it soon.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2019-07-02 03:09 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-06-23 09:21 Code comment change Vik Fearing <vik.fearing@2ndquadrant.com>
2019-06-23 10:35 ` Thomas Munro <thomas.munro@gmail.com>
2019-06-23 11:43 ` Vik Fearing <vik.fearing@2ndquadrant.com>
2019-07-02 02:03 ` Peter Geoghegan <pg@bowt.ie>
2019-07-02 02:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-07-02 03:09 ` Peter Geoghegan <pg@bowt.ie>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox