MIME-Version: 1.0
References: <20250911054220.3784-1-root@ip-172-31-36-228.ec2.internal>
 <CANWCAZYZQw-nzTXbx3Bk332VtY9_D7ksDsuMZ0A-iDZ53yG7Ng@mail.gmail.com>
 <CAK64mnfeWLBRbMfnOsag0vGTDnT84KJzpuei40nG0OHyw4SESw@mail.gmail.com>
 <CANWCAZa1b2rcvoK657SmcKwh2P2cgASQ1D-0JPj5d3LbfaAVgA@mail.gmail.com>
 <CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
In-Reply-To: 
 <CAK64mneN20+sW5WhV+r7hMVo4Rd0z11B6=3L039rWMt1wK3nPg@mail.gmail.com>
From: John Naylor <johncnaylorls@gmail.com>
Date: Tue, 21 Oct 2025 10:30:30 +0700
Message-ID: 
 <CANWCAZZuS3sNgLRo8Z4AM=uY4zTmz=dH5D4Z9xV6K0CEuJ8Hdw@mail.gmail.com>
Subject: Re: Proposal for enabling auto-vectorization for checksum
 calculations
To: Andrew Kim <tenistarkim@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org,
	Oleg Tselebrovskiy <o.tselebrovskiy@postgrespro.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CANWCAZZuS3sNgLRo8Z4AM%3DuY4zTmz%3DdH5D4Z9xV6K0CEuJ8Hdw%40mail.gmail.com>
Precedence: bulk

On Fri, Oct 17, 2025 at 2:15=E2=80=AFPM Andrew Kim <tenistarkim@gmail.com> =
wrote:
>
> Hi John,
>
> Thank you for your detailed and constructive feedback on the checksum
> AVX2 optimization patch.
> I've carefully addressed all of your concerns and am pleased to share
> the updated V6 implementation.

Great! I know we're on v7 now, but I'm going to make a request for
next time you respond to a review: Respond in-line to each point. As I
mentioned before,

> On Wed, Oct 1, 2025 at 10:26=E2=80=AFPM John Naylor <johncnaylorls@gmail.=
com> wrote:
> > (BTW, we discourage top-posting and prefer to cut to size and
> > use inline responses)

Please don't top-post again, as it clutters our archives in addition
to making it easy to forget things. I'm now going to copy the things
that were either not addressed or misunderstood:

> > I think a good first refactoring patch would be to move
> > src/backend/storage/checksum.c (which your patch doesn't even touch)
> > to src/port (and src/include/storage/checksum.h to src/include/port)
> > and have all callers use that. With that, I imagine only that
> > checksum.c file would include checksum_impl.h.
> >
> > If that poses a problem, let us know -- we may have to further juggle
> > things. If that works without issue, we can proceed with the
> > specialization.

That means the first patch moves things around without adding any
platform-specific code, and the next patch adds the specialization. I
think that would be a lot easier to review and test, especially to
avoid breaking external programs (see below for more on this). A
committer can always squash things together if it make sense to do so.

> > +    #if defined(__has_attribute) && __has_attribute (target)
> > +    __attribute__((target("avx2")))
> > +    #endif
> > +    static int avx2_test(void)
> > +    {
> > +      const char buf@<:@sizeof(__m256i)@:>@;
> > +      __m256i accum =3D _mm256_loadu_si256((const __m256i *) buf);
> > +   accum =3D _mm256_add_epi32(accum, accum);
> > +      int result =3D _mm256_extract_epi32(accum, 0);
> > +      return (int) result;
> > +    }],
> >
> > If we're just testing if the target works, we can just use an empty
> > function, right?

Oleg mentioned the same thing later. It's a waste of time for us to
repeat ourselves. I said you didn't have to worry about it yet,
because I was hoping to see the refactoring first.

Now, aside from that I looked further into this:

> > The top of the checksum_impl.h has this:
> >
> >  * This file exists for the benefit of external programs that may wish =
to
> >  * check Postgres page checksums.  They can #include this to get the co=
de
> >  * referenced by storage/checksum.h.  (Note: you may need to redefine
> >  * Assert() as empty to compile this successfully externally.)
> >
> > It's going to be a bit tricky to preserve this ability while allowing
> > the core server and client programs to dispatch to a specialized
> > implementation, but we should at least try. That means keeping
> > pg_checksum_block() and pg_checksum_page() where they live now.

Looking at commit f04216341dd1, we have at least one example of an
external program, pg_filedump. If we can keep this working with
minimal fuss, it should be fine everywhere.

https://github.com/df7cb/pg_filedump/blob/master/pg_filedump.c#L29

```
/* checksum_impl.h uses Assert, which doesn't work outside the server */
#undef Assert
#define Assert(X)

#include "storage/checksum.h"
#include "storage/checksum_impl.h"
```

Elsewhere they already have to do things like

```
#if PG_VERSION_NUM < 110000
    "   Previous Checkpoint Record: Log File (%u) Offset (0x%08x)\n"
#endif
```

...so it's probably okay if they have to adjust for a new #include
path, but I want to verify that actually works, and I don't want to
make it any more invasive than that. As we proceed, I can volunteer to
do the work to test that pg_filedump still builds fine with small
changes. Feel free to try building it yourself, but I'm happy to do
it.

Oleg posted another review recently, so I won't complicate things
further, but from a brief glance I will suggest for next time not to
change any comments that haven't been invalidated by the patch.

--
John Naylor
Amazon Web Services