From: Antonin Houska <ah@cybertec.at>
To: pgsql-hackers@postgresql.org
cc: Oleg Ivanov <o.ivanov@postgrespro.ru>
Subject: Re: [HACKERS] Proposal: generic WAL compression
In-reply-to: <59F90ABF.2060407@postgrespro.ru>
References: <59F90ABF.2060407@postgrespro.ru>
Comments: In-reply-to Oleg Ivanov <o.ivanov@postgrespro.ru>
   message dated "Wed, 01 Nov 2017 02:43:59 +0300."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Thu, 16 Nov 2017 18:31:23 +0100
Message-ID: <23990.1510853483@localhost>
Precedence: bulk

Oleg Ivanov <o.ivanov@postgrespro.ru> wrote:

> In order to overcome that issue, I would like to propose the patch, which
> provides possibility to use another approach of the WAL record
> construction.

After having spent several hours reviewing this patch I dare to send the
following comments:


* writeToPtr() and readFromPtr() are applied to the existing code. I think
  this is a reason for a separate diff, to be applied before the actual pat=
ch.

BTW, if you're going to do any refactoring of the existing code, I think the
"Begin" and "Start" words should be used consistently, see "fragmentBegin" =
vs
"validStart" in computeRegionBaseDelta().


* What's the reason for changing FRAGMENT_HEADER_SIZE ? I see no change in
  the data layout that writeFragment() produces.


* alignRegions()

** typo in the prologue: "mismathing".

** "An O(ND) Difference Algorithm and Its Variations" - I think the referen=
ce
   should contain more information, e.g. name of the author(s). I think I e=
ven
   saw URLs to scientific papers in the PG source but I'm not 100% sure rig=
ht
   now.

** As for the algorithm itself: Although I didn't have enough patience (and
   time) to follow it in every detail for this first review, I think I can
   imagine what it is about. Yet I think reading would be easier if the
   concepts of alignment and "diff delta" were depicted in the comments,
   perhaps using some sort of "ASCII graphics".

** DiffDeltaOperations enumeration: the individual values should be describ=
ed.


* computeRegionDiffDelta()

** Does "previous delta" in one of the comments refer to "base delta",
   i.e. the delta computation w/o your patch? If so, the comment should
   mention it explicitly.

** If you use Min() to initialize the for-loop, it'd be more consistent if =
you
   also used Max() when checking the limits:

for (i =3D Min(validStart, targetStart); i < Max(validEnd, targetEnd); ++i)

And similarly you can simplify the body of the loop.


* computeDelta()

As the expresssion

*((bool *) pageData->delta)

is used more than once, I think a separate (bool *) variable should be used.


--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at