agora inbox for [email protected]  

help / color / mirror / Atom feed

From: Antonin Houska <[email protected]>
To: [email protected]
Cc: Oleg Ivanov <[email protected]>
Subject: Re: [HACKERS] Proposal: generic WAL compression
Date: Thu, 16 Nov 2017 18:31:23 +0100
Message-ID: <23990.1510853483@localhost> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

Oleg Ivanov <[email protected]> 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 patch.

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 reference
   should contain more information, e.g. name of the author(s). I think I even
   saw URLs to scientific papers in the PG source but I'm not 100% sure right
   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 described.


* 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 = 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.


-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

view thread (14+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: [HACKERS] Proposal: generic WAL compression
  In-Reply-To: <23990.1510853483@localhost>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox