Re: [proposal] de-TOAST'ing using a iterator

From: Binguo Bao <djydewang(at)gmail(dot)com>
To: John Naylor <john(dot)naylor(at)2ndquadrant(dot)com>
Cc: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, Atri Sharma <atri(dot)jiit(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Владимир Лесков <vladimirlesk(at)yandex-team(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [proposal] de-TOAST'ing using a iterator
Date: 2019-08-17 07:32:32
Message-ID: CAL-OGkuKBd4vtSZHzDKnGoaEZsRjZWJXCWKP+CADiQ-4pdt7rw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi John,

> >> Also, I don't think
> >> the new logic for the ctrl/c variables is an improvement:
> >>
> >> 1. iter->ctrlc is intialized with '8' (even in the uncompressed case,
> >> which is confusing). Any time you initialize with something not 0 or
> >> 1, it's a magic number, and here it's far from where the loop variable
> >> is used. This is harder to read.
> >
> > `iter->ctrlc` is used to record the value of `ctrl` in pglz_decompress
> at the end of
> > the last iteration(or loop). In the pglz_decompress, `ctrlc`’s valid
> value is 0~7,
> > When `ctrlc` reaches 8, a control byte is read from the source
> > buffer to `ctrl` then set `ctrlc` to 0. And a control bytes should be
> read from the
> > source buffer to `ctrlc` on the first iteration. So `iter->ctrlc` should
> be intialized with '8'.
>
> My point here is it looks strange out of context, but "0" looked
> normal. Maybe a comment in init_detoast_buffer(), something like "8
> means read a control byte from the source buffer on the first
> iteration, see pg_lzdecompress_iterate()".
>
> Or, possibly, we could have a macro like INVALID_CTRLC. That might
> even improve the readability of the original function. This is just an
> idea, and maybe others would disagree, so you don't need to change it
> for now.
>

All in all, the idea is much better than a magic number 8. So, I've
implemented it.

> At this point, there are no functional things that I think we need to
> change. It's close to ready-for-committer. For the next version, I'd
> like you go through the comments and edit for grammar, spelling, and
> clarity as you see fit. I know you're not a native speaker of English,
> so I can help you with anything that remains.

I've tried my best to improve the comments, but there should be room for
further improvement
I hope you can help me perfect it.

> Also note we use braces
> on their own lines
> {
> like this
> }
>
> Done.
--
Best regards,
Binguo Bao

Attachment Content-Type Size
0001-de-TOASTing-using-a-iterator-8.patch text/x-patch 20.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2019-08-17 08:11:27 some SCRAM read_any_attr() confusion
Previous Message Antonin Houska 2019-08-17 06:16:06 Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)