Moving RestoreBlockImage from xlogreader.c to xlogutils.c

Hi all,

Commit 2c03216d has introduced RestoreBlockImage to restore a page
from a given decoding state. ISTM that this is a backend-only
operation but it has been added in xlogreader.c which could be used as
well by frontend utilities like pg_xlogdump.
Wouldn't it be better to declare it as a static routine in
xlogutils.c? If we keep it in xlogreader.c, I think that we should at
least wrap it with ifndef FRONTEND.
Thoughts?
--
Michael

On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Hi all,
>
> Commit 2c03216d has introduced RestoreBlockImage to restore a page
> from a given decoding state. ISTM that this is a backend-only
> operation but it has been added in xlogreader.c which could be used as
> well by frontend utilities like pg_xlogdump.
> Wouldn't it be better to declare it as a static routine in
> xlogutils.c? If we keep it in xlogreader.c, I think that we should at
> least wrap it with ifndef FRONTEND.
> Thoughts?

If we do this, pg_lzcompress.c doesn't need to be moved to common for
FPW compression patch which we're talking about in other thread. Right?

DecodeXLogRecord() seems also a backend-only, so we should treat it
in the same way as you proposed? Or pg_rewind uses that?

Regards,

--
Fujii Masao

On Wed, Dec 24, 2014 at 10:16 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> Wouldn't it be better to declare it as a static routine in
>> xlogutils.c? If we keep it in xlogreader.c, I think that we should at
>> least wrap it with ifndef FRONTEND.
>
> If we do this, pg_lzcompress.c doesn't need to be moved to common for
> FPW compression patch which we're talking about in other thread. Right?
Yes. This refactoring came to my mind while re-thinking about the WAL
compression. This would also make more straight-forward the
implementation of hooks for compression and decompression.

> DecodeXLogRecord() seems also a backend-only, so we should treat it
> in the same way as you proposed? Or pg_rewind uses that?
DecodeXLogRecord is used by XLogReadRecord, the latter being called by
pg_xlogdump and also pg_rewind, so it is not backend-only. IMO, only
exposing to the frontends the pointer to the beginning of the block
image in the decoder with its extra data, like hole length and hole
offset (or block length in record with WAL compression, be it
compressed or uncompressed), is just but fine. It looks weird to keep
in the xlog reader facility something that performs other operations
than reading a WAL record and organizing it in a readable state for
caller.
--
Michael

On Wed, Dec 24, 2014 at 10:41 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Dec 24, 2014 at 10:16 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Wouldn't it be better to declare it as a static routine in
>>> xlogutils.c? If we keep it in xlogreader.c, I think that we should at
>>> least wrap it with ifndef FRONTEND.
>>
>> If we do this, pg_lzcompress.c doesn't need to be moved to common for
>> FPW compression patch which we're talking about in other thread. Right?
> Yes. This refactoring came to my mind while re-thinking about the WAL
> compression. This would also make more straight-forward the
> implementation of hooks for compression and decompression.

Fair enough. Anyway I wait for applying the patch which moves pg_lzcompress.c
until we will have reached any consensus about this.

>> DecodeXLogRecord() seems also a backend-only, so we should treat it
>> in the same way as you proposed? Or pg_rewind uses that?
> DecodeXLogRecord is used by XLogReadRecord, the latter being called by
> pg_xlogdump and also pg_rewind, so it is not backend-only.

Yeah, you're right.

Regards,

--
Fujii Masao

On Wed, Dec 24, 2014 at 10:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Fair enough. Anyway I wait for applying the patch which moves pg_lzcompress.c
> until we will have reached any consensus about this.
Just to be clear (after sleeping on it), we still need pglz stuff in
src/common to offer to the frontends the possibility to uncompress
block data. My point is simply that we should only provide in the xlog
reader facility enough data to do operations on them, but not directly
APIs to operate them. So ISTM that you could still push the patch to
have pglz in common library to clear the way, and let's use this
thread to discuss if we want the API to rebuild blocks in the reader
facility or not.
--
Michael

On 2014-12-25 08:52:05 +0900, Michael Paquier wrote:
> On Wed, Dec 24, 2014 at 10:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > Fair enough. Anyway I wait for applying the patch which moves pg_lzcompress.c
> > until we will have reached any consensus about this.
> Just to be clear (after sleeping on it), we still need pglz stuff in
> src/common to offer to the frontends the possibility to uncompress
> block data. My point is simply that we should only provide in the xlog
> reader facility enough data to do operations on them, but not directly
> APIs to operate them. So ISTM that you could still push the patch to
> have pglz in common library to clear the way, and let's use this
> thread to discuss if we want the API to rebuild blocks in the reader
> facility or not.

I think it's a bad idea to move it away - the current placement provides
a API that allows to get at the image data without having to deal with
the low level details. E.g. the in_use, has_image and hole
logic. *Especially* when we add compression that's quite a useful
abstraction. Having it it in place allows us to restructure internals
without disturbing clients - and i don't see any price in this case.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Dec 25, 2014 at 7:48 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I think it's a bad idea to move it away - the current placement provides
> a API that allows to get at the image data without having to deal with
> the low level details. E.g. the in_use, has_image and hole
> logic. *Especially* when we add compression that's quite a useful
> abstraction. Having it it in place allows us to restructure internals
> without disturbing clients - and i don't see any price in this case.

There is no price as long as we keep the compression algorithm fixed
in core, but I do foresee one regarding the pluggability of the
decompression API knowing that RestoreBlockImage is the natural place
where block decompression should occur, and that now it is shared
between frontend and backend layers. I am digressing here, but what I
had in mind was to add exactly there a hook to allow our users to
plugin a custom compression algorithm, something that could be useful
for us and for our users in terms of flexibility for the WAL
compression, particularly for algorithms that are not compatible with
the Postgres license.
--
Michael

On 2014-12-25 21:12:54 +0900, Michael Paquier wrote:
> On Thu, Dec 25, 2014 at 7:48 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I think it's a bad idea to move it away - the current placement provides
> > a API that allows to get at the image data without having to deal with
> > the low level details. E.g. the in_use, has_image and hole
> > logic. *Especially* when we add compression that's quite a useful
> > abstraction. Having it it in place allows us to restructure internals
> > without disturbing clients - and i don't see any price in this case.
>
> There is no price as long as we keep the compression algorithm fixed
> in core, but I do foresee one regarding the pluggability of the
> decompression API knowing that RestoreBlockImage is the natural place
> where block decompression should occur, and that now it is shared
> between frontend and backend layers.
> I am digressing here, but what I had in mind was to add exactly there
> a hook to allow our users to plugin a custom compression algorithm,
> something that could be useful for us and for our users in terms of
> flexibility for the WAL compression, particularly for algorithms that
> are not compatible with the Postgres license. -- Michael

I personally think that's a bad idea and we shouldn't provide
functionality for that. But even if we added it, something that provides
the decompression for frontend code seems critical.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services