Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c
On 2014-12-25 21:12:54 +0900, Michael Paquier wrote: > On Thu, Dec 25, 2014 at 7:48 PM, Andres Freund 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
Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c
On Thu, Dec 25, 2014 at 7:48 PM, Andres Freund 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
On 2014-12-25 08:52:05 +0900, Michael Paquier wrote: > On Wed, Dec 24, 2014 at 10:51 PM, Fujii Masao 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
On Wed, Dec 24, 2014 at 10:51 PM, Fujii Masao 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
Re: [HACKERS] Moving RestoreBlockImage from xlogreader.c to xlogutils.c
On Wed, Dec 24, 2014 at 10:41 PM, Michael Paquier wrote: > On Wed, Dec 24, 2014 at 10:16 PM, Fujii Masao wrote: >> On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier >> 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
On Wed, Dec 24, 2014 at 10:16 PM, Fujii Masao wrote: > On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier > 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
On Wed, Dec 24, 2014 at 9:42 PM, Michael Paquier 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
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers