Thread: 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
Attachment
"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
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
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
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
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
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. +
On 6/2/07, Bruce Momjian <bruce@momjian.us> wrote:
OK, removed from 8.4 queue.
I am OK with this, though I personally never felt that it complicated
the code :-)
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com