Обсуждение: [PATCH] Remove twice assignment with var pageop (nbtree.c).

Hi,
The var pageop has twice assigment, maybe is a mistake?
The assigned in the line 593, has no effect?

Ranier Vilela

Same case on nbtpage.c at line 1637, with var opaque.
make check, passed all 195 tests here with all commits.

Ranier Vilela

On Tue, Nov 26, 2019 at 01:45:10PM +0000, Ranier Vilela wrote:
> Same case on nbtpage.c at line 1637, with var opaque.
> make check, passed all 195 tests here with all commits.
> 
> Ranier Vilela

You were right about both of these, so removed in master.  I am
surprised no one else saw this before.

---------------------------------------------------------------------------

> diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
> index 268f869a36..144fefccad 100644
> --- a/src/backend/access/nbtree/nbtpage.c
> +++ b/src/backend/access/nbtree/nbtpage.c
> @@ -1634,8 +1634,6 @@ _bt_mark_page_halfdead(Relation rel, Buffer leafbuf, BTStack stack)
>       * delete the following item.
>       */
>      page = BufferGetPage(topparent);
> -    opaque = (BTPageOpaque) PageGetSpecialPointer(page);
> -
>      itemid = PageGetItemId(page, topoff);
>      itup = (IndexTuple) PageGetItem(page, itemid);
>      BTreeInnerTupleSetDownLink(itup, rightsib);
> 


-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Nov 26, 2019 at 01:45:10PM +0000, Ranier Vilela wrote:
>> Same case on nbtpage.c at line 1637, with var opaque.
>> make check, passed all 195 tests here with all commits.

> You were right about both of these, so removed in master.  I am
> surprised no one else saw this before.

I don't think this is actually a good idea.  What it is is a foot-gun,
because if anyone adds code there that wants to access the special area
of that particular page, it'll do the wrong thing, unless they remember
to put back the assignment of "opaque".  The sequence of BufferGetPage()
and PageGetSpecialPointer() is a very standard switch-our-attention-
to-another-page locution in nbtree and other index AMs.

Any optimizing compiler will delete the dead store, we do not have
to do it by hand.

Let me put it this way: if we had the BufferGetPage() and
PageGetSpecialPointer() calls wrapped up as an "access new page" macro,
would we undo that in order to make this code change?  We would not.

            regards, tom lane

On Thu, Dec 19, 2019 at 10:55:42AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Tue, Nov 26, 2019 at 01:45:10PM +0000, Ranier Vilela wrote:
> >> Same case on nbtpage.c at line 1637, with var opaque.
> >> make check, passed all 195 tests here with all commits.
> 
> > You were right about both of these, so removed in master.  I am
> > surprised no one else saw this before.
> 
> I don't think this is actually a good idea.  What it is is a foot-gun,
> because if anyone adds code there that wants to access the special area
> of that particular page, it'll do the wrong thing, unless they remember
> to put back the assignment of "opaque".  The sequence of BufferGetPage()
> and PageGetSpecialPointer() is a very standard switch-our-attention-
> to-another-page locution in nbtree and other index AMs.

Oh, I was not aware that was boilerplate code.  I agree it should be
consistent, so patch reverted.  Sorry.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

De: Bruce Momjian <bruce@momjian.us>
Enviado: quinta-feira, 19 de dezembro de 2019 16:19

>Oh, I was not aware that was boilerplate code.  I agree it should be
>consistent, so patch reverted.  Sorry.
I apologize to you, Bruce.
It is difficult to define where a "boilerplate" exists.
I agree that decent compiler, will remove, maybe, msvc not, but that's another story...

Best regards,
Ranier Vilela

On Thu, Dec 19, 2019 at 7:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't think this is actually a good idea.  What it is is a foot-gun,
> because if anyone adds code there that wants to access the special area
> of that particular page, it'll do the wrong thing, unless they remember
> to put back the assignment of "opaque".  The sequence of BufferGetPage()
> and PageGetSpecialPointer() is a very standard switch-our-attention-
> to-another-page locution in nbtree and other index AMs.

+1

-- 
Peter Geoghegan