MIME-Version: 1.0
References: <CALj2ACVfF2Uj9NoFy-5m98HNtjHpuD17EDE9twVeJng-jTAe7A@mail.gmail.com>
 <CAPsk3_CzLbMe-D07H5Vo6yWFvyXHh5is7AoPUCFcztrUmf1haw@mail.gmail.com> <CALj2ACVzL4uU=hxFpSfkqP4HeFCPucbBTEg6HNf_MPTYm52pHg@mail.gmail.com>
In-Reply-To: <CALj2ACVzL4uU=hxFpSfkqP4HeFCPucbBTEg6HNf_MPTYm52pHg@mail.gmail.com>
From: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Date: Sat, 8 Jun 2024 17:24:17 +0530
Message-ID: <CAMm1aWYa1fGKcuG69xGJPNXLQ_9zHrPqhr-ZGdj4so6Exq66MQ@mail.gmail.com>
Subject: Re: Use WALReadFromBuffers in more places
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Jingtang Zhang <mrdrivingduck@gmail.com>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAMm1aWYa1fGKcuG69xGJPNXLQ_9zHrPqhr-ZGdj4so6Exq66MQ%40mail.gmail.com>
Precedence: bulk

Hi Bharath,

I spent some time examining the patch. Here are my observations from the re=
view.

- I believe there=E2=80=99s no need for an extra variable =E2=80=98nbytes=
=E2=80=99 in this
context. We can repurpose the =E2=80=98count=E2=80=99 variable for the same=
 function.
If necessary, we might think about renaming =E2=80=98count=E2=80=99 to =E2=
=80=98nbytes=E2=80=99.

- The operations performed by logical_read_xlog_page() and
read_local_xlog_page_guts() are identical. It might be beneficial to
create a shared function to minimize code repetition.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft

On Mon, May 13, 2024 at 12:17=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 8, 2024 at 9:51=E2=80=AFAM Jingtang Zhang <mrdrivingduck@gmai=
l.com> wrote:
> >
> > Hi, Bharath. I've been testing this. It's cool. Is there any way we cou=
ld
> > monitor the hit rate about directly reading from WAL buffers by exporti=
ng
> > to some views?
>
> Thanks for looking into this. I used purpose-built patches for
> verifying the WAL buffers hit ratio, please check
> USE-ON-HEAD-Collect-WAL-read-from-file-stats.txt and
> USE-ON-PATCH-Collect-WAL-read-from-buffers-and-file-stats.txt at
> https://www.postgresql.org/message-id/CALj2ACU9cfAcfVsGwUqXMace_7rfSBJ7%2=
BhXVJfVV1jnspTDGHQ%40mail.gmail.com.
> In the long run, it's better to extend what's proposed in the thread
> https://www.postgresql.org/message-id/CALj2ACU_f5_c8F%2BxyNR4HURjG%3DJzii=
z07wCpQc%3DAqAJUFh7%2B8w%40mail.gmail.com.
> I haven't had a chance to dive deep into that thread yet, but a quick
> glance over v8 patch tells me that it hasn't yet implemented the idea
> of collecting WAL read stats for both walsenders and the backends
> reading WAL. If that's done, we can extend it for WAL read from WAL
> buffers.
>
> --
> Bharath Rupireddy
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>
>