Received: from localhost (unknown [200.46.204.183])
	by postgresql.org (Postfix) with ESMTP id ACAB2650131
	for <pgsql-hackers-postgresql.org@postgresql.org>;
	Fri, 11 Jul 2008 07:02:27 -0300 (ADT)
Received: from postgresql.org ([200.46.204.86])
	by localhost (mx1.hub.org [200.46.204.183]) (amavisd-maia, port 10024)
	with ESMTP id 32016-01 for
	<pgsql-hackers-postgresql.org@postgresql.org>;
	Fri, 11 Jul 2008 07:02:20 -0300 (ADT)
X-Greylist: from auto-whitelisted by SQLgrey-1.7.6
Received: from fugue.toroid.org (fugue.toroid.org [85.10.196.113])
	by postgresql.org (Postfix) with ESMTP id 3B4A964FFD8
	for <pgsql-hackers@postgresql.org>;
	Fri, 11 Jul 2008 07:02:22 -0300 (ADT)
Received: from penne.toroid.org (penne-vpn [10.8.0.6])
	by fugue.toroid.org (Postfix) with ESMTP id E2B3355835A;
	Fri, 11 Jul 2008 12:02:16 +0200 (CEST)
Received: by penne.toroid.org (Postfix, from userid 1000)
	id 33942ADC364; Fri, 11 Jul 2008 15:32:16 +0530 (IST)
Date: Fri, 11 Jul 2008 15:32:16 +0530
From: Abhijit Menon-Sen <ams@oryx.com>
To: Zoltan Boszormenyi <zb@cybertec.at>
Cc: pgsql-hackers@postgresql.org
Subject: Re: posix advises ...
Message-ID: <20080711100215.GA14893@toroid.org>
References: <4826C200.8060807@cybertec.at>
	<Pine.GSO.4.64.0805132139250.20823@westnet.com>
	<485A3F20.1080907@cybertec.at>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <485A3F20.1080907@cybertec.at>
X-Virus-Scanned: Maia Mailguard 1.0.1
X-Archive-Number: 200807/571
X-Sequence-Number: 120831

Hi Zoltán.

I was reading through your posix_fadvise patch, and I wanted to ask
about this change in particular:

> --- a/src/backend/executor/nodeIndexscan.c
> +++ b/src/backend/executor/nodeIndexscan.c
> @@ -290,7 +290,7 @@ ExecIndexEvalArrayKeys(ExprContext *econtext,
>         /* We want to keep the arrays in per-tuple memory */
>         oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>  
> -       for (j = 0; j < numArrayKeys; j++)
> +       for (j = numArrayKeys-1; j >= 0; j--)
>         {
>                 ScanKey         scan_key = arrayKeys[j].scan_key;
>                 ExprState  *array_expr = arrayKeys[j].array_expr;

Why is this loop reversed? (I could have missed some discussion about
this, I just wanted to make sure it was intentional.)

I can confirm that the patch applies to HEAD, that the configure test
correctly #defines USE_POSIX_FADVISE, that it compiles cleanly, and it
looks basically sensible to me. The nodeBitmapHeapscan.c and iterator
changes need a second look by someone who understands the code better
than I do.

The documentation patch could use a little tweaking:

> +        <productname>PostgreSQL</productname> can give hints to POSIX compatible
> +        operating systems using posix_fadvise(2) to pre-read pages that will
> +        be needed in the near future. Reading the pages into the OS kernel's
> +        disk buffer will be done asynchronically while <productname>PostgreSQL</productname>
> +        works on pages which are already in memory. This may speed up bitmap scans
> +        considerably. This setting only applies to bitmap scans.
> +        Hinting for sequential scans is also used but no GUC is needed in this case.

I would suggest something like this instead:

    <productname>PostgreSQL</productname> can use posix_fadvise(2) to
    tell the operating system about pages that it knows will be needed
    in the near future. The performance of bitmap scans is considerably
    improved when the kernel pre-reads such pages into its disk buffers.
    This variable controls how many pages are marked for pre-reading at
    a time during a bitmap scan.

But I'm not convinced that this GUC is well-advised; at least, it needs
some advice about how to determine a sensible size for the parameter
(and maybe a better name). But is it really necessary at all?

Thanks.

-- ams