On Tue, Jan 17, 2017 at 12:18 PM, Michael Paquier <michael.paqu...@gmail.com > wrote:
> On Sun, Jan 8, 2017 at 9:52 AM, Jim Nasby <jim.na...@bluetreble.com> > wrote: > >> I agree pretty_print_kb would have been a better for this function. > >> However, I have realised that using the show hook and this function is > >> not suitable and have found a better way of handling the removal of > >> GUC_UNIT_XSEGS which no longer needs this function : using the > >> GUC_UNIT_KB, convert the value in bytes to wal segment count instead in > >> the assign hook. The next version of patch will use this. > > > > > > ... it sounds like you're going back to exposing KB to users, and that's > all > > that really matters. > > > >> IMHO it'd be better to use the n & (n-1) check detailed at [3]. > > That would be better. > > So I am looking at the proposed patch, though there have been reviews > the patch was in "Needs Review" state, and as far as I can see it is a > couple of things for frontends. Just by grepping for XLOG_SEG_SIZE I > have spotted the following problems: > - pg_standby uses it to know about the next segment available. > Yes. I am aware of this and had mentioned it in my post. > - pg_receivexlog still uses it in segment handling. > It may be a good idea to just remove XLOG_SEG_SIZE and fix the code > paths that fail to compile without it, frontend utilities included > because a lot of them now rely on the value coded in xlog_internal.h, > but with this patch the value is set up in the context of initdb. And > this would induce major breakages in many backup tools, pg_rman coming > first in mind... We could replace it with for example a macro that > frontends could use to check if the size of the WAL segment is in a > valid range if the tool does not have direct access to the Postgres > instance (aka the size of the WAL segment used there) as there are as > well offline tools. > I will see whats the best way to do this. > > -#define XLogSegSize ((uint32) XLOG_SEG_SIZE) > + > +extern uint32 XLogSegSize; > +#define XLOG_SEG_SIZE XLogSegSize > This bit is really bad for frontend declaring xlog_internal.h... > > --- a/src/bin/pg_test_fsync/pg_test_fsync.c > +++ b/src/bin/pg_test_fsync/pg_test_fsync.c > @@ -62,7 +62,7 @@ static const char *progname; > > static int secs_per_test = 5; > static int needs_unlink = 0; > -static char full_buf[XLOG_SEG_SIZE], > +static char full_buf[DEFAULT_XLOG_SEG_SIZE], > This would make sense as a new option of pg_test_fsync. > > A performance study would be a good idea as well. Regarding the > generic SHOW command in the replication protocol, I may do it for next > CF, I have use cases for it in my pocket. > > Thank you for your review. I have already made patch for the generic SHOW replication command (attached) and am working on the new initdb patch based on that. I have not yet fixed the pg_standby issue. I am trying to address all the comments and bugs still. -- Beena Emerson Have a Great Day!
repl_show_cmd.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
