Horton

Thread

Commits

  1. Cleanup comments in xlog compression

  1. Minor typo

    Stephen Frost <sfrost@snowman.net> — 2018-11-27T23:43:41Z

    Greetings,

While reviewing a bit of code around full page images, I came across a
typo and fixed it in the attach.

Sending it here in case anyone feels that we should do more than just
correct the word..?  Perhaps for non-native English speakers seeing
"whose" used here is confusing?

If I don't hear any concerns then I'll go ahead and push it some time
tomorrow; it's a pretty minor change, of course, and we can always
adjust it further if someone raises an issue later too.

Thanks!

Stephen

  2. Re: Minor typo

    Daniel Gustafsson <daniel@yesql.se> — 2018-11-27T23:49:49Z

    > On 28 Nov 2018, at 00:43, Stephen Frost <sfrost@snowman.net> wrote:

> Sending it here in case anyone feels that we should do more than just
> correct the word..?  Perhaps for non-native English speakers seeing
> "whose" used here is confusing?

Being a non-native English speaker I think it’s fine and, in my own bias,
commonly understood.

+1 on raising the “does this make sense for a non-native speaker” question
though, making the documentation (regardless of if it’s docs, code comments or
READMEs) accessible is very important.

cheers ./daniel

  3. Re: Minor typo

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-28T00:18:39Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 28 Nov 2018, at 00:43, Stephen Frost <sfrost@snowman.net> wrote:
>> Sending it here in case anyone feels that we should do more than just
>> correct the word..?  Perhaps for non-native English speakers seeing
>> "whose" used here is confusing?

> Being a non-native English speaker I think it’s fine and, in my own bias,
> commonly understood.

I agree that "which" is just wrong here, but "whose" seems a bit malaprop
as well, and it's not the only poor English in that sentence.  Maybe
rewrite the whole sentence to avoid the problem?

  * When wal_compression is enabled and a "hole" is removed from a full
  * page image, the page image is compressed using PGLZ compression.

(BTW, is this trying to say that we don't apply compression if the page
contains no hole?  If so, why not?)

			regards, tom lane

  4. Re: Minor typo

    Michael Paquier <michael@paquier.xyz> — 2018-11-28T00:31:04Z

    On Tue, Nov 27, 2018 at 07:18:39PM -0500, Tom Lane wrote:
>   * When wal_compression is enabled and a "hole" is removed from a full
>   * page image, the page image is compressed using PGLZ compression.
> 
> (BTW, is this trying to say that we don't apply compression if the page
> contains no hole?  If so, why not?)

Compression can be applied on a page with or without a hole.  What this
sentence means is that the equivalent of a double level of compression
can be applied on top of the hole being removed, if the hole can be
removed.
--
Michael

  5. Re: Minor typo

    Stephen Frost <sfrost@snowman.net> — 2018-11-28T00:45:09Z

    Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
> >> On 28 Nov 2018, at 00:43, Stephen Frost <sfrost@snowman.net> wrote:
> >> Sending it here in case anyone feels that we should do more than just
> >> correct the word..?  Perhaps for non-native English speakers seeing
> >> "whose" used here is confusing?
> 
> > Being a non-native English speaker I think it’s fine and, in my own bias,
> > commonly understood.
> 
> I agree that "which" is just wrong here, but "whose" seems a bit malaprop
> as well, and it's not the only poor English in that sentence.  Maybe
> rewrite the whole sentence to avoid the problem?
> 
>   * When wal_compression is enabled and a "hole" is removed from a full
>   * page image, the page image is compressed using PGLZ compression.

Yeah, that seems a bit cleaner.

> (BTW, is this trying to say that we don't apply compression if the page
> contains no hole?  If so, why not?)

Sure seems to be saying that, and later on..

     * If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED, an
     * XLogRecordBlockCompressHeader struct follows.

Yet XLogCompressBackupBlock() sure looks like it'll happily compress a
page even if it doesn't have a hole.  The replay logic certainly seems
to only check if BKPIMAGE_IS_COMPRESSED is set.

I'm thinking there's quite a bit of room for improvement here...  I
wonder if the idea originally was "the page is 'compressed', in some
way, if it either had a hole or was actually compressed"..

Thanks!

Stephen

  6. Re: Minor typo

    Stephen Frost <sfrost@snowman.net> — 2018-11-28T00:51:03Z

    Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Nov 27, 2018 at 07:18:39PM -0500, Tom Lane wrote:
> >   * When wal_compression is enabled and a "hole" is removed from a full
> >   * page image, the page image is compressed using PGLZ compression.
> > 
> > (BTW, is this trying to say that we don't apply compression if the page
> > contains no hole?  If so, why not?)
> 
> Compression can be applied on a page with or without a hole.  What this
> sentence means is that the equivalent of a double level of compression
> can be applied on top of the hole being removed, if the hole can be
> removed.

That isn't at all what I got from that.

A rewrite of this really should avoid talking about removing the hole as
if it's 'compression' because, first of all, it isn't, and second, now
that we have *actual* compression happening, it's terribly confusing to
talk about removing the hole using that terminology.

Thanks!

Stephen

  7. Re: Minor typo

    Michael Paquier <michael@paquier.xyz> — 2018-11-28T01:01:36Z

    On Tue, Nov 27, 2018 at 07:51:03PM -0500, Stephen Frost wrote:
> That isn't at all what I got from that.
> 
> A rewrite of this really should avoid talking about removing the hole as
> if it's 'compression' because, first of all, it isn't, and second, now
> that we have *actual* compression happening, it's terribly confusing to
> talk about removing the hole using that terminology.

You may want to be careful about other comments in xloginsert.c or such
then.  Removing a page hole is mentioned in a couple of places as a form
of compression if I recall correctly.
--
Michael

  8. Re: Minor typo

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-11-28T06:02:26Z

    This is a noise from a Japanese having poor English skill..

At Wed, 28 Nov 2018 10:01:36 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20181128010136.GU1716@paquier.xyz>
> On Tue, Nov 27, 2018 at 07:51:03PM -0500, Stephen Frost wrote:
> > That isn't at all what I got from that.
> > 
> > A rewrite of this really should avoid talking about removing the hole as
> > if it's 'compression' because, first of all, it isn't, and second, now
> > that we have *actual* compression happening, it's terribly confusing to
> > talk about removing the hole using that terminology.
> 
> You may want to be careful about other comments in xloginsert.c or such
> then.  Removing a page hole is mentioned in a couple of places as a form
> of compression if I recall correctly.

org> When wal_compression is enabled, a full page image which "hole" was
org> removed is additionally compressed using PGLZ compression algorithm.

Even though it has an obvious mistake, I could read this as
full_page_image is always compressed after removing a hole in it
if any. (I'm not sure it makes correct sense, though.) I feel
hopelessly that the sentence structure model in my mind is
different from that of natives. I need to study harder, but..

Sorry for the noise in advance.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  9. Re: Minor typo

    Stephen Frost <sfrost@snowman.net> — 2018-12-04T16:37:16Z

    Greetings,

* Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
> At Wed, 28 Nov 2018 10:01:36 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20181128010136.GU1716@paquier.xyz>
> > On Tue, Nov 27, 2018 at 07:51:03PM -0500, Stephen Frost wrote:
> > > That isn't at all what I got from that.
> > > 
> > > A rewrite of this really should avoid talking about removing the hole as
> > > if it's 'compression' because, first of all, it isn't, and second, now
> > > that we have *actual* compression happening, it's terribly confusing to
> > > talk about removing the hole using that terminology.
> > 
> > You may want to be careful about other comments in xloginsert.c or such
> > then.  Removing a page hole is mentioned in a couple of places as a form
> > of compression if I recall correctly.
> 
> org> When wal_compression is enabled, a full page image which "hole" was
> org> removed is additionally compressed using PGLZ compression algorithm.
> 
> Even though it has an obvious mistake, I could read this as
> full_page_image is always compressed after removing a hole in it
> if any. (I'm not sure it makes correct sense, though.) I feel
> hopelessly that the sentence structure model in my mind is
> different from that of natives. I need to study harder, but..

Thanks to everyone for sharing their thoughts here, the goal is simply
to try and have the comments as clear as we can for everyone.

Please find attached a larger rewording of the comment in xlogrecord.h,
and a minor change to xloginsert.c to clarify that we're removing a hole
and not doing compression at that point.

Other thoughts on this..?

Thanks!

Stephen

  10. Re: Minor typo

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-12-06T06:21:29Z

    At Tue, 4 Dec 2018 11:37:16 -0500, Stephen Frost <sfrost@snowman.net> wrote in <20181204163716.GR3415@tamriel.snowman.net>
> Thanks to everyone for sharing their thoughts here, the goal is simply
> to try and have the comments as clear as we can for everyone.
> 
> Please find attached a larger rewording of the comment in xlogrecord.h,
> and a minor change to xloginsert.c to clarify that we're removing a hole
> and not doing compression at that point.
> 
> Other thoughts on this..?

Thank you for the complete rewriting. It makes the description
clearer at least for me, execpt the following:

>  * present is BLCKSZ - the length of "hole" bytes.

Maybe it's because I'm not so accustomed to punctuation marks but
I was confused for a second because the '-' didn't look to me a
minus sign, but a dash. If it is not specific to me, a word
'minus' seems better or, (BLCKSZ - <the length of "hole">) bytes
is clearer .... for me.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  11. Re: Minor typo

    Stephen Frost <sfrost@snowman.net> — 2018-12-06T16:07:00Z

    Greetings,

* Kyotaro HORIGUCHI (horiguchi.kyotaro@lab.ntt.co.jp) wrote:
> At Tue, 4 Dec 2018 11:37:16 -0500, Stephen Frost <sfrost@snowman.net> wrote in <20181204163716.GR3415@tamriel.snowman.net>
> > Thanks to everyone for sharing their thoughts here, the goal is simply
> > to try and have the comments as clear as we can for everyone.
> > 
> > Please find attached a larger rewording of the comment in xlogrecord.h,
> > and a minor change to xloginsert.c to clarify that we're removing a hole
> > and not doing compression at that point.
> > 
> > Other thoughts on this..?
> 
> Thank you for the complete rewriting. It makes the description
> clearer at least for me, execpt the following:
> 
> >  * present is BLCKSZ - the length of "hole" bytes.
> 
> Maybe it's because I'm not so accustomed to punctuation marks but
> I was confused for a second because the '-' didn't look to me a
> minus sign, but a dash. If it is not specific to me, a word
> 'minus' seems better or, (BLCKSZ - <the length of "hole">) bytes
> is clearer .... for me.

I agree- that threw me off a bit also when I was first reading it.  I've
updated that part a bit and pushed it.

Thanks!

Stephen