Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-25 Thread Andres Freund
On 2014-12-25 08:52:05 +0900, Michael Paquier wrote:
 On Wed, Dec 24, 2014 at 10:51 PM, Fujii Masao masao.fu...@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-25 Thread Michael Paquier
On Thu, Dec 25, 2014 at 7:48 PM, Andres Freund and...@2ndquadrant.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-25 Thread Andres Freund
On 2014-12-25 21:12:54 +0900, Michael Paquier wrote:
 On Thu, Dec 25, 2014 at 7:48 PM, Andres Freund and...@2ndquadrant.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-24 Thread Michael Paquier
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-24 Thread Fujii Masao
On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier
michael.paqu...@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-24 Thread Michael Paquier
On Wed, Dec 24, 2014 at 10:16 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier
 michael.paqu...@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-24 Thread Fujii Masao
On Wed, Dec 24, 2014 at 10:41 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 On Wed, Dec 24, 2014 at 10:16 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier
 michael.paqu...@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c

2014-12-24 Thread Michael Paquier
On Wed, Dec 24, 2014 at 10:51 PM, Fujii Masao masao.fu...@gmail.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers