Re: [HACKERS] Hint Bits and Write I/O

From: "Pavan Deolasee" <pavan(dot)deolasee(at)gmail(dot)com>
To: "Simon Riggs" <simon(at)2ndquadrant(dot)com>
Cc: "List pgsql-patches" <pgsql-patches(at)postgresql(dot)org>
Subject: Re: [HACKERS] Hint Bits and Write I/O
Date: 2008-07-21 06:06:09
Message-ID: 2e78013d0807202306t21ff41a9q53673dc9a48bf859@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches

On Tue, Jul 1, 2008 at 4:13 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
>
> The first "half" is actually quite large, but that makes it even more
> sensible to commit this part now.
>
> The enclosed patch introduces the machinery by which we might later
> optimise hint bit setting. It differentiates between hint bit setting
> and block dirtying, when the distinction can safely be made. It acts
> safely during VACUUM and correctly during checkpoint. In all other
> respects it emulates current behaviour.
>

As you yourself said, this patch mostly gets the machinery to count
hint bits in place and leaves the actual optimization for future. But
I think we should try at least one or two possible optimizations and
get some numbers before we jump and make substantial changes to the
code. Also that would help us in testing the patch for correctness and
performance.

For example, the following hunk seems buggy to me:

Index: src/backend/storage/buffer/bufmgr.c
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.232
diff -c -r1.232 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 12 Jun 2008 09:12:31 -0000 1.232
--- src/backend/storage/buffer/bufmgr.c 30 Jun 2008 22:17:20 -0000
***************
*** 1460,1473 ****

if (bufHdr->refcount == 0 && bufHdr->usage_count == 0)
result |= BUF_REUSABLE;
! else if (skip_recently_used)
{
/* Caller told us not to write recently-used buffers */
UnlockBufHdr(bufHdr);
return result;
}

! if (!(bufHdr->flags & BM_VALID) || !(bufHdr->flags & BM_DIRTY))
{
/* It's clean, so nothing to do */
UnlockBufHdr(bufHdr);
--- 1462,1477 ----

if (bufHdr->refcount == 0 && bufHdr->usage_count == 0)
result |= BUF_REUSABLE;
! else if (LRU_scan)
{
/* Caller told us not to write recently-used buffers */
UnlockBufHdr(bufHdr);
return result;
}

! if (!(bufHdr->flags & BM_VALID) ||
! !(bufHdr->flags & BM_DIRTY ||
! (LRU_scan && bufHdr->hint_count > 0)))
{
/* It's clean, so nothing to do */
UnlockBufHdr(bufHdr);

In the "if" condition above, we would throw away a buffer if the
hint_count is greater than zero, even if the buffer is dirty. This
doesn't seem correct to me, unless I am missing something obvious.

> The actual tuning patch can be discussed later, probably at length.
> Later patches will be fairly small in comparison and so various people
> can fairly easily come up with their own favoured modifications for
> testing.
>
>

I would suggest, let's have at least one tuning patch along with some
tests and numbers, before we go ahead and commit anything.

Thanks,
Pavan

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2008-07-21 06:11:27 Re: pg_dump additional options for performance
Previous Message Tom Lane 2008-07-21 05:38:16 Re: TODO item: Have psql show current values for a sequence

Browse pgsql-patches by date

  From Date Subject
Next Message Simon Riggs 2008-07-21 06:11:27 Re: pg_dump additional options for performance
Previous Message Tom Lane 2008-07-21 05:38:16 Re: TODO item: Have psql show current values for a sequence