Re: [HACKERS] Performance Improvement by reducing WAL for Update Operation
On Friday, July 19, 2013 4:11 AM Greg Smith wrote: On 7/9/13 12:09 AM, Amit Kapila wrote: I think the first thing to verify is whether the results posted can be validated in some other environment setup by another person. The testcase used is posted at below link: http://www.postgresql.org/message-id/51366323.8070...@vmware.com That seems easy enough to do here, Heikki's test script is excellent. The latest patch Hari posted on July 2 has one hunk that doesn't apply anymore now. The Head code change from Heikki is correct. During the patch rebase to latest PG LZ optimization code, the above code change is missed. Apart from the above changed some more changes are done in the patch, those are. 1. corrected some comments in the code 2. Added a validity check as source and history length combined cannot be more than or equal to 8192. Thanks for the review, please find the latest patch attached in the mail. Regards, Hari babu. pglz-with-micro-optimization-compress-using-newdata-3.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
Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages
On Friday, July 05, 2013 6:48 PM Hari Babu wrote: On Thursday, July 04, 2013 11:19 PM Robert Haas wrote: The patch is updated with the following changes. 1.If the input data is data directory, all the errors occurred are displayed at once instead of one error at a time. 2.Fixed the problem of replacing result of the previous file or directory result with new one. 3.Update the docs. Thanks for the review, please find the updated patch attached in the mail. Regards, Hari babu. pg_computemaxlsn_v9.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
Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages
On Monday, July 08, 2013 5:16 PM Andres Freund wrote: On 2013-07-08 17:10:43 +0530, Amit Kapila wrote: On Monday, July 08, 2013 4:26 PM Andres Freund wrote: On 2013-07-08 16:17:54 +0530, Hari Babu wrote: +This utility can also be used to decide whether backup is required or not when the data page +in old-master precedes the last applied LSN in old-standby (i.e., new-master) at the +moment of the failover. + /para + /refsect1 I don't think this is safe in any interesting set of cases. Am I missing something? No, you are not missing anything. It can be only used to find max LSN in database which can avoid further corruption Why is the patch submitted documenting it as a use-case then? I find it rather scary if the *patch authors* document a known unsafe use case as one of the known use-cases. I got the problem which can occur with the specified use case. Removed the wrong use case specified above. Thanks for the review, please find the updated patch attached in the mail. Regards, Hari babu. pg_computemaxlsn_v10.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
Re: [HACKERS] WAL
On Saturday, July 06, 2013 10:00 AM mohsen soodkhah mohammadi wrote: hello. I am reading about WAL and XLOG records. I am beginner in them. can you direct me and give me some document? Please go through the README file which is present in the following folder for more details. Src/backend/access/transam/ Regards, Hari babu.
Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages
On Thursday, July 04, 2013 11:19 PM Robert Haas wrote: + fprintf(stderr, _(%s: .. file \%s\ for seeking: %s\n), + progname, filename, strerror(errno)); Weird error message style - what's with the ..? + fprintf(stderr, _(%s: .. file \%s\ length is more than segment size: %d.\n), + progname, filename, RELSEG_SIZE); Again. Corrected. + fprintf(stderr, _(%s: could not seek to next page \%s\: %s\n), + progname, filename, strerror(errno)); I think this should be written as: could not seek to offset NUMBER in file PATH + fprintf(stderr, _(%s: could not read file \%s\: %s\n), + progname, filename, strerror(errno)); And this one as: could not read file PATH at offset NUMBER + printf(File:%s Maximum LSN found is: %X/%X \nWAL segment file name (fileid,seg): %X/%X\n, I think that we don't need to display the WAL segment file name for the per-file progress updates. Instead, let's show the block number where that LSN was found, like this: Highest LSN for file %s is %X/%X in block %u. The overall output can stay as you have it. Changed as per your suggestion. + if (0 != result) Coding style. + static int + FindMaxLSNinFile(char *filename, XLogRecPtr *maxlsn) It seems that this function, and a few others, use -1 for a failure return, 0 for success, and all others undefined. Although this is a defensible choice, I think it would be more PG-like to make the return value bool and use true for success and false for failure. + if (S_ISREG(statbuf.st_mode)) + (void) FindMaxLSNinFile(pathbuf, maxlsn); + else + (void) FindMaxLSNinDir(pathbuf, maxlsn); I don't see why we're throwing away the return value here. I would expect the function to have a bool result = true at the top and sent result = false if one of these functions returns false. At the end, it returns result. Fixed. +This utility can only be run by the user who installed the server, because +it requires read/write access to the data directory. False. Removed. + LsnSearchPath = argv[optind]; + + if (LsnSearchPath == NULL) + LsnSearchPath = getenv(PGDATA); I think you should write the code so that the tool loops over its input arguments; if none, it processes $PGDATA. (Don't forget to update the syntax synopsis and documentation to match.) Added the functionality of multiple file or directories parsing and printing Max LSN for each input argument. I think the documentation should say something about the intended uses of this tool, including cautions against using it for things to which it is not well-suited. I guess the threshold question for this patch is whether those uses are enough to justify including the tool in the core distribution. Added some use cases and notes regarding the tool. Please suggest if any More information needs to be documented. Thanks for the review, please find the updated patch attached in the mail. Regards, Hari babu. pg_computemaxlsn_v8.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
Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages
as the PostgreSQL superuser.\n), + progname); + exit(1); + } + #endif This utility only reads files; it does not modify them. So this seems unnecessary. I assume it's blindly copied from somewhere else. + fprintf(stderr, _(%s: \%s\ not a valid data directory.\n), Extra space. + /* +* Check for a postmaster lock file --- if there is one, refuse to +* proceed, on grounds we might be interfering with a live installation. +*/ + snprintf(path, MAXPGPATH, %s/postmaster.pid, DataDir); Again, this might be appropriate for pg_resetxlog, but I see no reason for the restriction here. The output might be inaccurate in that case, but if you're using this tool you're required to know what you're doing. Fixed. +For safety reasons, you must specify the data directory on the command line. +commandpg_computemaxlsn/command does not use the environment variable +envarPGDATA/. Same thing here. I think using PGDATA would be quite appropriate for this utility. Fixed. + para +This command must not be used when the server is +running. commandpg_computemaxlsn/command will refuse to start up if +it finds a server lock file in the data directory. If the +server crashed then a lock file might have been left +behind; in that case you can remove the lock file to allow +commandpg_computemaxlsn/command to run. But before you do +so, make doubly certain that there is no server process still alive. + /para More of the same paranoia. Overall my feeling is that this can be simplified quite a lot. There's a lot of stuff in here that's really not needed. Corrected. Please find the updated patch attached. Regards, Hari babu. pg_computemaxlsn_v7.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
Re: [HACKERS] fixing pg_ctl with relative paths
On January 23, 2013 9:13 AM Josh Kupershmidt wrote:
There have been some complaints[1][2] in the past about pg_ctl not playing
nice with relative path specifications for the datadir. Here's a concise
illustration:
$ mkdir /tmp/mydata/ initdb /tmp/mydata/
$ cd /tmp/
$ pg_ctl -D ./mydata/ start
$ cd /
$ pg_ctl -D /tmp/mydata/ restart
IMO it's pretty hard to defend the behavior of the last step, where pg_ctl
knows exactly which datadir the user has specified, and succeeds in stopping
the server but not starting it.
Digging into this gripe, a related problem I noticed is that `pg_ctl ...
restart` doesn't always preserve the -D $DATADIR argument as the following
comment suggests it should[4]:
* We could pass PGDATA just in an environment
* variable but we do -D too for clearer postmaster
* 'ps' display
Specifically, if one passes in additional -o options, e.g.
$ pg_ctl -D /tmp/mydata/ -o -N 10 restart
after which postmaster.opts will be missing the -D ... argument which is
otherwise recorded, and the `ps` output is similarly abridged.
Anyway, Tom suggested[3] two possible ways of fixing the original gripe,
and I went with his latter suggestion,
| for pg_ctl restart to override that
| option with its freshly-derived idea of where the data directory is
mainly so we don't need to worry about the impact of changing the
appearance of postmaster.opts, plus it seems like this code fits better in
pg_ctl.c rather than the postmaster (where the postmaster.opts file is
actually written). The fix seems to be pretty simple, namely stripping
post_opts of the old -D ... portion and having the new specification, if
any, be used instead. I believe the attached patch should fix these two
infelicities.
Full disclosure: the strip_datadirs() function makes no attempt to properly
handle pathnames containing quotes. There seems to be some, uh, confusion in
the existing code about how these should be handled already. For instance,
$ mkdir /tmp/here's a \ quote
$ initdb /tmp/here's a \ quote
How to successfully start, restart, and stop the server with pg_ctl is left
as an exercise for the reader. So I tried to avoid that can of worms with
this patch, though I'm happy to robustify strip_datadirs() if we'd like to
properly support such pathnames, and there's consensus on how to standardize
the escaping.
[1]
http://www.postgresql.org/message-id/CAA-aLv72O+NegjAipHORmbqSMZTkZayaTxrd+C
9v60ybmmm...@mail.gmail.com
[2]
http://www.postgresql.org/message-id/CAK3UJRGABxWSOCXnAsSYw5BfR4D9ageXF+6Gts
RVm-LtfWfW=g...@mail.gmail.com
[3] http://www.postgresql.org/message-id/27233.1350234...@sss.pgh.pa.us
[4] Note, ps output and postmaster.opts will not include the datadir
specification if you rely solely on the PGDATA environment variable for
pg_ctl
Please find the review of the patch.
Basic stuff:
- Patch applies OK
- Compiles cleanly with no warnings
- Regression tests pass.
What it does:
-
The restart functionality of pg_ctl has problems with relative paths. This
patch removes the
problems arising during restart. This patch strips the data directory which
is present in the
postmaster options and keep the rest of the options already provided incase
if user not provided
any options during restart.
Code Review:
+if (orig_post_opts) {
+post_opts = strip_datadirs(orig_post_opts);
+}
No need of {} as the only one statement block is present in the if block.
+ free(tmp);
The above statement can be moved inside the if (*(trailing_quote + 1) !=
'\0') {
where it's exact usage is present.
Testing:
I tested this feature with different postmaster options and database folder
names, found no problem.
The database folder with quotes present in it is any way having problems
with pg_ctl.
I feel the strip_datadirs() function header explanation is providing good
understanding.
Overall the patch is good. It makes the pg_ctl restart functionality works
well.
Regards,
Hari babu
--
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] fixing pg_ctl with relative paths
On June 26, 2013 5:02 AM Josh Kupershmidt wrote: Thanks for the feedback. Attached is a rebased version of the patch with the two small issues noted fixed. Patch is good, I marked the patch as ready for committer. Regards, Hari babu -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] system catalog pg_rewrite column ev_attr document description problem
Hi, system catalog pg_rewrite column ev_attr document description as shown below ev_attr - The column this rule is for (currently, always zero to indicate the whole table) But In the code the column value is always set as -1. can we change the column description as below is fine? ev_attr - The column this rule is for, presently this value is -1. Regards, Hari babu. -- 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] pg_basebackup with -R option and start standby have problems with escaped password
On Monday, February 18, 2013 8:06 PM Boszormenyi Zoltan wrote:
On 2013-01-29 11:15 keltezéssel, Magnus Hagander írta:
On Thu, Jan 24, 2013 at 7:04 AM, Hari Babu wrote:
On Wed, Jan 23, 2013 11:48 PM, Magnus Hagander wrote:
On Wed, Jan 23, 2013 at 10:18 AM, Hari Babu wrote:
Test scenario to reproduce:
1. Start the server
2. create the user as follows
./psql postgres -c create user user1 superuser
login password 'use''1'
3. Take the backup with -R option as follows.
./pg_basebackup -D ../../data1 -R -U user1 -W
The following errors are occurring when the new standby on the
backup database starts.
FATAL: could not connect to the primary server: missing = after
1'
in
connection info string
What does the resulting recovery.conf file look like?
The recovery.conf which is generated is as follows
standby_mode = 'on'
primary_conninfo = 'user=''user1'' password=''use''1'' port=''5432'' '
I observed the problem is while reading primary_conninfo from the
recovery.conf file the function GUC_scanstr removes the quotes of
the string and also makes the continuos double quote('') as single
quote(').
By using the same connection string while connecting to primary
server the function conninfo_parse the escape quotes are not able
to parse properly and it is leading to problem.
please correct me if any thing wrong in my observation.
Well, it's clearly broken at least :O
So, there is a bug in generating recovery.conf by not double-escaping the
values and another bug in parsing the connection string in libpq when the
parameter value starts with a single-quote character.
Attached are two patches to fix these two bugs, the libpq part can be
back-patched.
With the attached patches I tested the defect and it is fixed.
Regards,
Hari babu.
--
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] Strange Windows problem, lock_timeout test request
On Wednesday, January 30, 2013 7:59 PM Zoltán Böszörményi wrote: 2013-01-28 15:20 keltezéssel, Hari Babu írta: 2. regress check failed because the expected .out file is not updated properly. Which regress check failed? The .out file was updated in the patch for prepared_xacts.sql where the regression tests for lock_timeout were added. Or do you mean the one for the sql file you sent? During regress test, prepared_xacts_1.out expected file used for comparing with the result file. Which is not updated by the patch. Because of this reason the regress check is failing. Regards, Hari babu. -- 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] Strange Windows problem, lock_timeout test request
On Saturday, January 19, 2013 11:23 AM Amit kapila wrote: On Saturday, January 19, 2013 4:13 AM Boszormenyi Zoltan wrote: Hi, Unfortunately, I won't have time to do anything with my lock_timeout patch for about 3 weeks. Does anyone have a little spare time to test it on Windows? I shall try to do it, probably next week. Others are also welcome to test the patch. I have carried out some windows testing of the lock timeout patch. The extra tests which are carried out on the patch are attached in the mail. Some observations on the patch: 1. Patch needs a rebase as it causing some rejections. 2. regress check failed because the expected .out file is not updated properly. Regards, Hari babu. lock_timeout_test.sql 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
Re: [HACKERS] Passing connection string to pg_basebackup
On Tue, Jan 22, 2013 3:27 PM Hari Babu wrote: On Saturday, January 19, 2013 5:49 PM Magnus Hagander wrote: On Fri, Jan 18, 2013 at 1:05 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 18.01.2013 13:41, Amit Kapila wrote: On Friday, January 18, 2013 3:46 PM Heikki Linnakangas wrote: On 18.01.2013 08:50, Amit Kapila wrote: So to solve this problem below can be done: 1. Support connection string in pg_basebackup and mention keepalives or connection_timeout 2. Support recv_timeout separately to provide a way to users who are not comfortable tcp keepalives a. 1 can be done alone b. 2 can be done alone c. both 1 and 2. Right. Let's do just 1 for now. An general application level, non-TCP, keepalive message at the libpq level might be a good idea, but that's a much larger patch, definitely not 9.3 material. +1 for doing 1 now. But actually, I think we can just keep it that way in the future as well. If you need to specify these fairly advanced options, using a connection string really isn't a problem. I think it would be more worthwhile to go through the rest of the tools in bin/ and make sure they *all* support connection strings. And, an important point, do it the same way. Presently I am trying to implement the option-1 by adding an extra command line Option -C connection_string to pg_basebackup and pg_receivexlog. This option can be used with all the tools in bin folder. The existing command line options to the tools are not planned to remove as of now. To handle both options, we can follow these approaches. 1. To make the code simpler, the connection string is formed inside with the existing command line options, if the user is not provided the connection_string option. which is used for further processing. 2. The connection_string and existing command line options are handled separately. I feel approach-1 is better. Please provide your suggestions on the same. Here is the patch which handles taking of connection string as an argument to pg_basebackup and pg_receivexlog. Description of changes: 1. New command line -C connection-stringoption is added for passing the connection string. 2. Used PQconnectdb function for connecting to server instead of existing function PQconnectdbParams. 3. The existing command line parameters are formed in a string and passed to PQconnectdb function. 4. With the connection string, if user provides additional options with existing command line options, higher priority is given for the additional options. 5. conninfo_parse function is modified to handle of single quote in the password provided as input. please provide your suggestions. Regards, Hari babu. pg_basebkup_recvxlog_conn_string_v1.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
[HACKERS] pg_basebackup with -R option and start standby have problems with escaped password
Test scenario to reproduce: 1. Start the server 2. create the user as follows ./psql postgres -c create user user1 superuser login password 'use''1' 3. Take the backup with -R option as follows. ./pg_basebackup -D ../../data1 -R -U user1 -W The following errors are occurring when the new standby on the backup database starts. FATAL: could not connect to the primary server: missing = after 1' in connection info string Regards, Hari babu. -- 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] pg_basebackup with -R option and start standby have problems with escaped password
On Wed, Jan 23, 2013 11:48 PM, Magnus Hagander wrote:
On Wed, Jan 23, 2013 at 10:18 AM, Hari Babu haribabu.ko...@huawei.com
wrote:
Test scenario to reproduce:
1. Start the server
2. create the user as follows
./psql postgres -c create user user1 superuser login
password 'use''1'
3. Take the backup with -R option as follows.
./pg_basebackup -D ../../data1 -R -U user1 -W
The following errors are occurring when the new standby on the backup
database starts.
FATAL: could not connect to the primary server: missing = after 1'
in
connection info string
What does the resulting recovery.conf file look like?
The recovery.conf which is generated is as follows
standby_mode = 'on'
primary_conninfo = 'user=''user1'' password=''use''1'' port=''5432'' '
I observed the problem is while reading primary_conninfo from the
recovery.conf file
the function GUC_scanstr removes the quotes of the string and also makes
the
continuos double quote('') as single quote(').
By using the same connection string while connecting to primary server the
function conninfo_parse the escape quotes are not able to parse properly
and it is leading
to problem.
please correct me if any thing wrong in my observation.
Regards,
Hari babu.
--
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] Passing connection string to pg_basebackup
On Saturday, January 19, 2013 5:49 PM Magnus Hagander wrote: On Fri, Jan 18, 2013 at 1:05 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 18.01.2013 13:41, Amit Kapila wrote: On Friday, January 18, 2013 3:46 PM Heikki Linnakangas wrote: On 18.01.2013 08:50, Amit Kapila wrote: So to solve this problem below can be done: 1. Support connection string in pg_basebackup and mention keepalives or connection_timeout 2. Support recv_timeout separately to provide a way to users who are not comfortable tcp keepalives a. 1 can be done alone b. 2 can be done alone c. both 1 and 2. Right. Let's do just 1 for now. An general application level, non-TCP, keepalive message at the libpq level might be a good idea, but that's a much larger patch, definitely not 9.3 material. +1 for doing 1 now. But actually, I think we can just keep it that way in the future as well. If you need to specify these fairly advanced options, using a connection string really isn't a problem. I think it would be more worthwhile to go through the rest of the tools in bin/ and make sure they *all* support connection strings. And, an important point, do it the same way. Presently I am trying to implement the option-1 by adding an extra command line Option -C connection_string to pg_basebackup and pg_receivexlog. This option can be used with all the tools in bin folder. The existing command line options to the tools are not planned to remove as of now. To handle both options, we can follow these approaches. 1. To make the code simpler, the connection string is formed inside with the existing command line options, if the user is not provided the connection_string option. which is used for further processing. 2. The connection_string and existing command line options are handled separately. I feel approach-1 is better. Please provide your suggestions on the same. Regards, Hari babu. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review of pg_basebackup and pg_receivexlog to use non-blocking socket communication, was: Re: [HACKERS] Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown
On January 07, 2013 7:53 PM Boszormenyi Zoltan wrote: Since my other patch against pg_basebackup is now committed, this patch doesn't apply cleanly, patch rejects 2 hunks. The fixed up patch is attached. Patch is verified. Thanks for rebasing the patch. Regards, Hari babu. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review of pg_basebackup and pg_receivexlog to use non-blocking socket communication, was: Re: [HACKERS] Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown
On January 02, 2013 12:41 PM Hari Babu wrote:
On January 01, 2013 10:19 PM Boszormenyi Zoltan wrote:
I am reviewing your patch.
Is the patch in context diff format?
Yes.
Thanks for reviewing the patch.
Does it apply cleanly to the current git master?
Not quite cleanly but it doesn't produce rejects or fuzz, only offset
warnings:
Will rebase the patch to head.
Does it include reasonable tests, necessary doc patches, etc?
The test cases are not applicable. There is no test framework for
testing network outage in make check.
There are no documentation patches for the new --recvtimeout=INTERVAL
and --conntimeout=INTERVAL options for either pg_basebackup or
pg_receivexlog.
I will add the documentation for the same.
Per the previous comment, no. But those are for the backend
to notice network breakdowns and as such, they need a
separate patch.
I also think it is better to handle it as a separate patch for walsender.
Are the comments sufficient and accurate?
This chunk below removes a comment which seems obvious enough
so it's not needed:
***
*** 518,524 ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos,
uint32 timeline,
goto error;
}
! /* Check the message type. */
if (copybuf[0] == 'k')
{
int pos;
--- 559,568
goto error;
}
! /* Set the last reply timestamp */
! last_recv_timestamp = localGetCurrentTimestamp();
! ping_sent = false;
!
if (copybuf[0] == 'k')
{
int pos;
***
Other comments are sufficient and accurate.
I will fix and update the patch.
The attached V2 patch in the mail handles all the review comments identified
above.
Regards,
Hari babu.
pg_basebkup_recvxlog_noblock_comm_v2.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
Re: Review of pg_basebackup and pg_receivexlog to use non-blocking socket communication, was: Re: [HACKERS] Re: [BUGS] BUG #7534: walreceiver takes long time to detect n/w breakdown
On January 01, 2013 10:19 PM Boszormenyi Zoltan wrote:
I am reviewing your patch.
Is the patch in context diff format?
Yes.
Thanks for reviewing the patch.
Does it apply cleanly to the current git master?
Not quite cleanly but it doesn't produce rejects or fuzz, only offset
warnings:
Will rebase the patch to head.
Does it include reasonable tests, necessary doc patches, etc?
The test cases are not applicable. There is no test framework for
testing network outage in make check.
There are no documentation patches for the new --recvtimeout=INTERVAL
and --conntimeout=INTERVAL options for either pg_basebackup or
pg_receivexlog.
I will add the documentation for the same.
Per the previous comment, no. But those are for the backend
to notice network breakdowns and as such, they need a
separate patch.
I also think it is better to handle it as a separate patch for walsender.
Are the comments sufficient and accurate?
This chunk below removes a comment which seems obvious enough
so it's not needed:
***
*** 518,524 ReceiveXlogStream(PGconn *conn, XLogRecPtr startpos,
uint32 timeline,
goto error;
}
! /* Check the message type. */
if (copybuf[0] == 'k')
{
int pos;
--- 559,568
goto error;
}
! /* Set the last reply timestamp */
! last_recv_timestamp = localGetCurrentTimestamp();
! ping_sent = false;
!
if (copybuf[0] == 'k')
{
int pos;
***
Other comments are sufficient and accurate.
I will fix and update the patch.
Please let me know if anything apart from above needs to be taken care.
Regards,
Hari babu.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] compliation error in 9.3 devel
The following commit is causing some compilation errors. c504513f83a9ee8dce4a719746ca73102cae9f13 Error: ../../../src/include/catalog/pg_aggregate.h:246: error: previous declaration of AggregateCreate was here make: *** [pg_aggregate.o] Error 1 Author: Robert Haas rh...@postgresql.org 2012-12-24 04:55:03 Committer: Robert Haas rh...@postgresql.org 2012-12-24 05:07:58 Parent: 31bc839724439440b2e94ea616b28ce5be94e19c (Prevent failure when RowExpr or XmlExpr is parse-analyzed twice.) Child: (Local uncommitted changes, not checked in to index) Branches: Truncate-trailing-null-columns, master Follows: REL9_2_BETA2 Precedes: Adjust many backend functions to return OID rather than void. Extracted from a larger patch by Dimitri Fontaine. It is hoped that this will provide infrastructure for enriching the new event trigger functionality, but it seems possibly useful for other purposes as well. Regards, Hari babu.
Re: [HACKERS] WIP patch for hint bit i/o mitigation
On Thu, Dec 7, 2012 at 7:56 PM, Hari babu haribabu(dot)kommi(at)Huawei(dot)com wrote: On Thu, Dec 6, 2012 at 8:52 PM, Merlin Moncure mmonc...@gmail.com wrote: Thanks for that -- that's fairly comprehensive I'd say. I'm quite interested in that benchmarking framework as well. Do you need help setting up the scripts? Presently I am testing with pgbench custom query option taking IO VM statistics in parallel. Following way I had written the test script for case -1. ./psql -f bench_test_1_init.sql postgres iostat -t 1 -d hint.test1.iostat.reading_3.txt vmstat -n 1 hint.test1.vmstat.reading_3.txt ./pgbench -f bench_test_1.sql -T 300 -c 8 -j 8 -n postgres killall -s SIGINT iostat killall -s SIGINT vmstat Where the sql files are as follows: -- bench_test_1.sql select count(*) from bench where f1 is not null; --bench_test_1_init.sql drop table if exists bench; create table bench(f0 int primary key, f1 char(50)); insert into bench values (generate_series(1, 10), 'a'); insert into bench values (generate_series(11, 20), 'a'); ... insert into bench values (generate_series(981, 990), 'a'); insert into bench values (generate_series(991, 1000), 'a'); checkpoint; I will provide the test results later. Please find the review of the patch. Basic stuff: - Patch applies with offsets. - Compiles cleanly with no warnings - Regression Test pass. Code Review: - 1. Better to set the hint bits for the tuples in a page, if the page is already dirty. Test cases: -- Test cases are already described in the following link. http://archives.postgresql.org/message-id/00d301cdd398$6e3fff30$4abffd90$@k ap...@huawei.com http://archives.postgresql.org/message-id/00d301cdd398$6e3fff30$4abffd90$@ka p...@huawei.com Test Results: Machine details: CPU cores : 4 RAM : 24GB OS: Suse Linux 10 SP2 Configuration: shared_buffers = 500MB autovacuum = off checkpoint_segments = 256 checkpoint_timeout = 10min Following result are average of 3 runs each run is of 5min through pgbench custom query. Test case - 1: Init: None Run: create temp table atri1 as select v from generate_series(1,100) v; select * from atri1; VACUUM atri1; DROP TABLE atri1; Test case - 2: Init: create table atri1 as select v from generate_series(1,100) v; Run: select * from atri1; Test case - 3: (without pgbench) connect two sessions s1, s2 s1 : start transaction; s2 : create table atri1 as select v from generate_series(1,100) v; \timing select * from atri1; VACUUM atri1; Results: 9.3devel(Tps)HintbitIO(Tps) --- Test case - 1: 1.7629461.922219 Test case - 2: 4.0386734.044356 Pgbench without vacuum scale factor 75, 8 threads 8 client. (refer reports attached) Default tables select : 64980.73614964550.118693 Unlogged tables select: 64874.97433464550.118693 Multiple transaction bulk inserts: Select performance (refer script -1 2 which attached) sequential scan: 6.4926806.382014 Index scan: 1.3868511.36234 Single transaction bulk inserts: Select performance (refer script - 3 4 which attached) sequential scan: 6.49319 6.3800147 Index scan: 1.3841211.3615277 Long transaction open then Vacuum select performance in milli seconds. (refer reports output) Testcase - 3: Single Vacuum Perf : 128.302 ms 181.292 ms Single select perf : 214.107 ms 177.057 ms Total: 342.409 ms 358.349 ms I was not able to find the reason why in some of cases results are low so please use the attached scripts to validate the same. I was not able to provide the IO statistics as IOSTAT VMSTAT was giving the current snapshot but not the cumulative from start to end of test execution. Documentation: - No user visible changes, so no documentation is need to update. Regards, Hari babu. results.tar.gz Description: Binary data scripts.tar.gz 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
Re: [HACKERS] WIP patch for hint bit i/o mitigation
On Thu, Dec 6, 2012 at 8:52 PM, Merlin Moncure mmonc...@gmail.com wrote: Thanks for that -- that's fairly comprehensive I'd say. I'm quite interested in that benchmarking framework as well. Do you need help setting up the scripts? Presently I am testing with pgbench custom query option taking IO VM statistics in parallel. Following way I had written the test script for case -1. ./psql -f bench_test_1_init.sql postgres iostat -t 1 -d hint.test1.iostat.reading_3.txt vmstat -n 1 hint.test1.vmstat.reading_3.txt ./pgbench -f bench_test_1.sql -T 300 -c 8 -j 8 -n postgres killall -s SIGINT iostat killall -s SIGINT vmstat Where the sql files are as follows: -- bench_test_1.sql select count(*) from bench where f1 is not null; --bench_test_1_init.sql drop table if exists bench; create table bench(f0 int primary key, f1 char(50)); insert into bench values (generate_series(1, 10), 'a'); insert into bench values (generate_series(11, 20), 'a'); ... insert into bench values (generate_series(981, 990), 'a'); insert into bench values (generate_series(991, 1000), 'a'); checkpoint; I will provide the test results later. Any suggestions/comments? Regards, Hari babu. -- 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] pg_basebackup is taking backup of extra files inside a tablespace directory
On Wed, Nov 28, 2012 at 12:43 PM, Michael Paquier michael(dot)paquier(at)gmail(dot)comwrote: On Wed, Nov 28, 2012 at 3:55 PM, Hari Babu haribabu(dot)kommi(at)huawei(dot)comwrote: pg_basebackup is taking backup of extra files other than database related files in side a TABLESPACE directory. And why do you have files not related to your PG server inside a folder dedicated to a PG server? Incase of pg_upgrade old version to new version, new tablespace will be created in same tablespace path with different tablespace version number. So while taking backup need to skip the other data directory files. ls opt/tbs1 PG_9.2_201204301 PG_9.3_201210071 Regards, Hari babu. From: Michael Paquier [mailto:michael.paqu...@gmail.com] Sent: Wednesday, November 28, 2012 12:43 PM To: Hari Babu Cc: PostgreSQL-development Subject: Re: [HACKERS] pg_basebackup is taking backup of extra files inside a tablespace directory On Wed, Nov 28, 2012 at 3:55 PM, Hari Babu haribabu.ko...@huawei.com wrote: pg_basebackup is taking backup of extra files other than database related files in side a TABLESPACE directory. And why do you have files not related to your PG server inside a folder dedicated to a PG server? -- Michael Paquier http://michael.otacoo.com
[HACKERS] pg_basebackup is taking backup of extra files inside a tablespace directory
pg_basebackup is taking backup of extra files other than database related files in side a TABLESPACE directory. Scenario: 1) Create tablespace in existing directory '/opt/tblspc' having some extra files and folders. create tablespace tbs1 location '/opt/tblspc'; 2) Now execute the pg_basebackup command; We can see it will copy the extra files in '/opt/tblspc' directory I think backup should be done only files and folders present inside '/opt/tblspc/PG_*' directory (TABLESPACE_VERSION_DIRECTORY). Not all the files and folders in side '/opt/tblspc.' directory. Is it ok to fix in the following way? In function perform_base_backup, while sending the tablespaces one by one we can send the header for Linkpath/TABLESPACE_VERSION_DIRECTORY as separate header and sendDir for Linkpath/TABLESPACE_VERSION_DIRECTORY as path. Regards, Hari babu.
[HACKERS] pg_resetxlog defect with relative database path
When I was testing pg_resetxlog with relative database path,
The pg_resetxlog is doing the transaction log reset even when the server is
running.
Steps to reproduce:
1. ./pg_ctl -D ../../data start
2. ./pg_resetxlog -f ../../data -- is not able to detect as server
is already running.
Please find the patch for the same:
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
***
*** 254,260 main(int argc, char *argv[])
*/
snprintf(path, MAXPGPATH, %s/postmaster.pid, DataDir);
! if ((fd = open(path, O_RDONLY, 0)) 0)
{
if (errno != ENOENT)
{
--- 254,260
*/
snprintf(path, MAXPGPATH, %s/postmaster.pid, DataDir);
! if ((fd = open(postmaster.pid, O_RDONLY, 0)) 0)
{
if (errno != ENOENT)
{
Any suggestions/comments?
Regards,
Hari babu.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
