Обсуждение: Minor typo
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
Вложения
> 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
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
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
Вложения
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
Вложения
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
Вложения
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
Вложения
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
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
Вложения
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
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