Re: [REVIEW] Re: Compression of full-page-writes

Hello,

>When I test the WAL or replication related features, I usually run
>"make installcheck" and pgbench against the master at the same time
>after setting up the replication environment.
I will conduct these tests before sending updated version.

>Seems this increases the header size of WAL record even if no backup block
image is included. Right?
Yes, this increases the header size of WAL record by 1 byte for every block
reference even if it has no backup block image.

>Isn't it better to add the flag info about backup block image into
XLogRecordBlockImageHeader rather than XLogRecordBlockHeader
Yes , this will make the code extensible,readable and will save couple of
bytes per record.
But the current approach is to provide a chunk ID identifying different
xlog record fragments like main data , block references etc.
Currently , block ID is used to identify record fragments which can be
either XLR_BLOCK_ID_DATA_SHORT , XLR_BLOCK_ID_DATA_LONG or actual block ID.
This can be replaced by chunk ID to separate it from block ID. Block ID can
be used to number the block fragments whereas chunk ID can be used to
distinguish between main data fragments and block references. Chunk ID of
block references can contain information about presence of data, image ,
hole and compression.
Chunk ID for main data fragments remains as it is . This approach provides
for readability and extensibility.

Thank you,
Rahila Syed

On Mon, Mar 2, 2015 at 3:43 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Fri, Feb 27, 2015 at 12:44 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Feb 27, 2015 at 8:01 AM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Fri, Feb 27, 2015 at 6:54 AM, Rahila Syed <rahilasyed90(at)gmail(dot)com>
> wrote:
> >>>>Even this patch doesn't work fine. The standby emit the following
> >>>>error messages.
> >>>
> >>> Yes this bug remains unsolved. I am still working on resolving this.
> >>>
> >>> Following chunk IDs have been added in the attached patch as suggested
> >>> upthread.
> >>> +#define XLR_CHUNK_BLOCK_REFERENCE 0x10
> >>> +#define XLR_CHUNK_BLOCK_HAS_IMAGE 0x04
> >>> +#define XLR_CHUNK_BLOCK_HAS_DATA 0x08
> >>>
> >>> XLR_CHUNK_BLOCK_REFERENCE denotes chunk ID of block references.
> >>> XLR_CHUNK_BLOCK_HAS_IMAGE is a replacement of BKPBLOCK_HAS_IMAGE
> >>> and XLR_CHUNK_BLOCK_HAS DATA a replacement of BKPBLOCK_HAS_DATA.
> >>
> >> Before sending a new version, be sure that this get fixed by for
> >> example building up a master with a standby replaying WAL, and running
> >> make installcheck-world or similar. If the standby does not complain
> >> at all, you have good chances to not have bugs. You could also build
> >> with WAL_DEBUG to check record consistency.
>
> +1
>
> When I test the WAL or replication related features, I usually run
> "make installcheck" and pgbench against the master at the same time
> after setting up the replication environment.
>
> typedef struct XLogRecordBlockHeader
> {
> + uint8 chunk_id; /* xlog fragment id */
> uint8 id; /* block reference ID */
>
> Seems this increases the header size of WAL record even if no backup block
> image is included. Right? Isn't it better to add the flag info about backup
> block image into XLogRecordBlockImageHeader rather than
> XLogRecordBlockHeader?
> Originally we borrowed one or two bits from its existing fields to minimize
> the header size, but we can just add new flag field if we prefer
> the extensibility and readability of the code.
>
> Regards,
>
> --
> Fujii Masao
>