Обсуждение: Ctid chain following enhancement

Attached is a patch which should marginally improve the ctid chain followin
code path when current and the next tuple in the chain are in the same block.

In the current code, we unconditionally drop pin of the current block without
checking whether the next tuple is the same block or not. ISTM that this can
be improved.

The attached patch replaces the heap_fetch() with heap_release_fetch() in
EvalPlanQual() and thus releases the buffer iff the chain gets into a different block.
Similarly, ReadBuffer() is replaced with ReleaseAndReadBuffer() in
heap_get_latest_tid() for the same reason. The buffer is set to InvalidBuffer
to start with and is not released at the end of the loop. heap_release_fetch()/
ReleaseAndReadBuffer() would release it if required.

Thanks,
Pavan

--

EnterpriseDB     http://www.enterprisedb.com

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> Attached is a patch which should marginally improve the ctid chain followin
> code path when current and the next tuple in the chain are in the same
> block.

It looks to me that you have introduced a buffer leak into
heap_get_latest_tid ... which is quite unlikely to be worth optimizing
anyway.  EvalPlanQual is not exactly a performance-critical path either,
and given how hard that code is to understand at all, complicating it
for marginal performance gains seems dubious.

            regards, tom lane

I can't spot that. A previously pinned buffer is released at the start
of the loop if we are moving to a different block. Otherwise, the buffer
is released at all places where the for(;;) loop is terminated by a "break".
Am I missing something ?

Thanks,
Pavan

--

EnterpriseDB     http://www.enterprisedb.com

"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> On 1/27/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It looks to me that you have introduced a buffer leak into
>> heap_get_latest_tid ...

> I can't spot that.

No, you're right, my apologies.  I was thinking that the patch ought to
introduce an UnlockReleaseBuffer after the loop, but that's not
necessary given the calls before all the breaks.  (OTOH it might be
cleaner to refactor things that way, if we were going to apply this.
I still don't think heap_get_latest_tid is worth any optimization
effort, though.)

            regards, tom lane

Here is a revised patch which includes refactoring of
heap_get_latest_tid(), as per Tom's suggestion.

Thanks,
Pavan

--

EnterpriseDB     http://www.enterprisedb.com

Your patch has been added to the PostgreSQL unapplied patches list at:

    http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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


Pavan Deolasee wrote:
> On 1/28/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > OTOH it might be
> > cleaner to refactor things that way, if we were going to apply this.
> >
> >
> Here is a revised patch which includes refactoring of
> heap_get_latest_tid(), as per Tom's suggestion.
>
> Thanks,
> Pavan
>
> --
>
> EnterpriseDB     http://www.enterprisedb.com

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to majordomo@postgresql.org so that your
>        message can get through to the mailing list cleanly

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

  + If your life is a hard drive, Christ can be your backup. +

Pavan Deolasee wrote:
>
> On 1/28/07, *Tom Lane* <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>>
> wrote:
>
>     OTOH it might be
>     cleaner to refactor things that way, if we were going to apply this.
>
>
> Here is a revised patch which includes refactoring of
> heap_get_latest_tid(), as per Tom's suggestion.
>

I'm looking on your patch. I have one comment:

If you have old tid and new tid you can easy compare if new tid points
to different page? And if page is still same there is no reason to
Unlock it and lock again. I think add inner loop something like:


Readbufer
Lock
do{

...

} while(ctid.block_id == tid.block_id)
ReleaseAndUnlock

can save some extra locking/unlocking cycle. What do you think?


        Zdenek

Tom has already expressed his unwillingness to add complexity
without any proven benefits. Your suggestion though good would
make the code more unreadable without much benefit since the
function is not called very often.


Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB     http://www.enterprisedb.com

Pavan Deolasee napsal(a):
>
> On 5/29/07, *Zdenek Kotala* <Zdenek.Kotala@sun.com
> <mailto:Zdenek.Kotala@sun.com>> wrote:
>
>
>     I'm looking on your patch. I have one comment:
>
>     If you have old tid and new tid you can easy compare if new tid points
>     to different page? And if page is still same there is no reason to
>     Unlock it and lock again. I think add inner loop something like:
>
>
> Tom has already expressed his unwillingness to add complexity
> without any proven benefits. Your suggestion though good would
> make the code more unreadable without much benefit since the
> function is not called very often.

OK. I think Bruce can remove your patch from pipeline?


        Zdenek

This has been saved for the 8.4 release:

    http://momjian.postgresql.org/cgi-bin/pgpatches_hold

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

Pavan Deolasee wrote:
> On 1/28/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > OTOH it might be
> > cleaner to refactor things that way, if we were going to apply this.
> >
> >
> Here is a revised patch which includes refactoring of
> heap_get_latest_tid(), as per Tom's suggestion.
>
> Thanks,
> Pavan
>
> --
>
> EnterpriseDB     http://www.enterprisedb.com

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to majordomo@postgresql.org so that your
>        message can get through to the mailing list cleanly

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

  + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> This has been saved for the 8.4 release:

I think it should be dropped entirely.  The argument against was that
it complicated the code in a non-performance-critical path, and that
argument isn't going to be different next time.

            regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > This has been saved for the 8.4 release:
>
> I think it should be dropped entirely.  The argument against was that
> it complicated the code in a non-performance-critical path, and that
> argument isn't going to be different next time.

I only kept it for 8.4 because I was worried it might be needed for HOT
performance.  Are we sure that is not the case because that is why it
was originally proposed.

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

  + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> I think it should be dropped entirely.  The argument against was that
>> it complicated the code in a non-performance-critical path, and that
>> argument isn't going to be different next time.

> I only kept it for 8.4 because I was worried it might be needed for HOT
> performance.

No such argument has been made in my hearing, and I can't imagine why
either of the functions touched by the patch would be more
performance-critical for HOT than they are today.

            regards, tom lane

Patch rejected, not held for 8.4, because it uglifies the code.

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

Pavan Deolasee wrote:
> On 1/28/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > OTOH it might be
> > cleaner to refactor things that way, if we were going to apply this.
> >
> >
> Here is a revised patch which includes refactoring of
> heap_get_latest_tid(), as per Tom's suggestion.
>
> Thanks,
> Pavan
>
> --
>
> EnterpriseDB     http://www.enterprisedb.com

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to majordomo@postgresql.org so that your
>        message can get through to the mailing list cleanly

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

  + If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> I think it should be dropped entirely.  The argument against was that
> >> it complicated the code in a non-performance-critical path, and that
> >> argument isn't going to be different next time.
>
> > I only kept it for 8.4 because I was worried it might be needed for HOT
> > performance.
>
> No such argument has been made in my hearing, and I can't imagine why
> either of the functions touched by the patch would be more
> performance-critical for HOT than they are today.

OK, removed from 8.4 queue.

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

  + If your life is a hard drive, Christ can be your backup. +

I am OK with this, though I personally never felt that it complicated
the code :-)

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB     http://www.enterprisedb.com