"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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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. 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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

---------------------------(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

On 5/29/07, Zdenek Kotala <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.


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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq