Re: [HACKERS] Bug with pg_basebackup and 'shared' tablespace
On Tuesday, September 19, 2017 12:52:37 PM CEST you wrote: > On Thu, Sep 14, 2017 at 2:17 AM, Pierre Ducroquet wrote: > > All my apologies for the schockingly long time with no answer on this > > topic. > No problem. That's the concept called life I suppose. > > > I will do my best to help review some patches in the current CF. > > Thanks for the potential help! > > > Attached to this email is the new version of the patch, checked against > > HEAD and REL_10_STABLE, with the small change required by Michael (affect > > true/ false to the boolean instead of 1/0) applied. > > Thanks for the new version. > > So I have been pondering about this patch, and allowing multiple > versions of Postgres to run in the same tablespace is mainly here for > upgrade purposes, so I don't think that we should encourage such > practices for performance reasons. There is as well > --tablespace-mapping which is here to cover a similar need and we know > that this solution works, at least people have spent time to make sure > it does. > > Another problem that I have with this patch is that on HEAD, > permission checks are done before receiving any data. I think that > this is really saner to do any checks like that first, so as you can > know if the tablespace is available for write access before writing > any data to it. With this patch, you may finish by writing a bunch of > data to a tablespace path, but then fail on another tablespace because > of permission issues so the backup becomes useless after transferring > and writing potentially hundreds of gigabytes. This is no fun to > detect that potentially too late, and can impact the responsiveness of > instances to get backups and restore from them. > > All in all, this patch gets a -1 from me in its current shape. If > Horiguchi-san or yourself want to process further with this patch, of > course feel free to do so, but I don't feel that we are actually > trying to solve a new problem here. I am switching the patch to > "waiting on author". Hi The point of this patch has never been to 'promote' sharing tablespaces between PostgreSQL versions. This is not a documented feature, and it would be imho a bad idea to promote it because of bugs like this one. But that bug is a problem for people who are migrating between postgresql releases one database at a time on the same server (maybe not a best practice, but a real use case nevertheless). That's how I met this bug, and that's why I wanted to patch it. I know there is a workaround, but to me it seems counter- intuitive that with no warning I can create a postgresql cluster that can not be restored without additional pg_basebackup settings. If it is indeed forbidden to share a tablespace between postgresql releases, why don't we enforce it ? Or at least show a warning when CREATE TABLESPACE encounter a non-empty folder ? Regarding the permission check, I don't really see how this is a real problem with the patch. I have to check on master, but it seems to me that the permission check can still be done before starting writing data on disc. We could just delay the 'empty' folder check, and keep checking the folder permissions. And I will do the pgindent run, thanks for the nitpick :) Pierre -- 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] Small patch for pg_basebackup argument parsing
On Monday, September 18, 2017 5:13:38 PM CEST Tom Lane wrote: > Ryan Murphy writes: > > Looked thru the diffs and it looks fine to me. > > Changing a lot of a integer/long arguments that were being read directly > > via atoi / atol to be read as strings first, to give better error > > handling. > > > > This looks good to go to me. Reviewing this as "Ready for Committer". > > Thanks for the patch, Pierre! > > I took a quick look through this and had a few thoughts: I agree with your suggestions, I will try to produce a new patch before the end of the week. > * If we're taking the trouble to sanity-check the input, I think we should > also check for ERANGE (i.e., overflow). > > * I concur with Robert that you might as well just test for "*endptr != > '\0'" instead of adding a strlen() call. > > * Replicating fiddly little code sequences like this all over the place > makes me itch. There's too much chance to get it wrong, and even more > risk of someone taking shortcuts. It has to be not much more complicated > than using atoi(), or we'll just be doing this exercise again in a few > years. So I'm thinking we need to introduce a convenience function, > perhaps located in src/common/, or else src/fe_utils/. > > * Changing variables from int to long int is likely to have unpleasant > consequences on platforms where they're different; or at least, I don't > particularly feel like auditing the patch to ensure that that's not a > problem anywhere. So I think we should not do that but just make the > convenience function return int, with a suitable overflow test for that > result size. There's existing logic you can copy in > src/backend/nodes/read.c: > > errno = 0; > val = strtol(token, &endptr, 10); > if (endptr != token + length || errno == ERANGE > #ifdef HAVE_LONG_INT_64 > /* if long > 32 bits, check for overflow of int4 */ > > || val != (long) ((int32) val) > > #endif > ) > ... bad integer ... > > If there are places where we actually do want a long result, we > could have two convenience functions, one returning int and one long. > > > The exact form of the convenience function is up for grabs, but one > way we could do it is > > bool pg_int_convert(const char *str, int *value) > > (where a true result indicates a valid integer value). > Then typical usage would be like > > if (!pg_int_convert(optarg, &compresslevel) || > compresslevel < 0 || compresslevel > 9) > ... complain-and-exit ... > > There might be something to be said for folding the range checks > into the convenience function: > > bool pg_int_convert(const char *str, int min, int max, int *value) > > if (!pg_int_convert(optarg, 0, 9, &compresslevel)) > ... complain-and-exit ... > > That way you could protect code like > > standby_message_timeout = atoi(optarg) * 1000; > > fairly easily: > > if (!pg_int_convert(optarg, 0, INT_MAX/1000, > &standby_message_timeout)) > ... complain-and-exit ... > standby_message_timeout *= 1000; > > regards, tom lane -- 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] Small patch for pg_basebackup argument parsing
On Wednesday, September 13, 2017 2:06:50 AM CEST Daniel Gustafsson wrote:
> > On 05 Jul 2017, at 08:32, Michael Paquier
> > wrote:>
> > On Wed, Jul 5, 2017 at 2:57 PM, Ryan Murphy wrote:
> >> I tried to apply your patch to test it (though reading Robert's last
> >> comment it seems we wish to have it adjusted before committing)... but
> >> in any case I was not able to apply your patch to the tip of the master
> >> branch (my git apply failed). I'm setting this to Waiting On Author for
> >> a new patch, and I also agree with Robert that the test can be simpler
> >> and can go in the other order. If you don't have time to make another
> >> patch, let me know, I may be able to make one.>
> > git is unhappy even if forcibly applied with patch -p1. You should
> > check for whitespaces at the same time:
> > $ git diff --check
> > src/bin/pg_basebackup/pg_receivewal.c:483: indent with spaces.
> > +char *strtol_endptr = NULL
> > And there are more than this one.
>
> Like Michael said above, this patch no longer applies and have some
> whitespace issues. The conflicts in applying are quite trivial though, so
> it should be easy to create a new version. Do you plan to work on this
> during this Commitfest so we can move this patch forward?
Yes, my bad. Attached to this email is the new version, that now applies on
master.
Sorry for the delay :(
>From 7efc1573c1bcc07c0eaa80912e6e035f2e0d203d Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet
Date: Wed, 13 Sep 2017 19:51:09 +0200
Subject: [PATCH] Port most calls of atoi(optarg) to strcol
atoi does not allow any kind of data check. If the input data is invalid,
the result will be silently truncated to the valid part of input, or just
0 if no digit was found at the beginning of input.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++-
src/bin/pg_basebackup/pg_receivewal.c | 11 ++-
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++-
src/bin/pg_ctl/pg_ctl.c| 9 -
src/bin/pg_dump/pg_dump.c | 12 +---
src/bin/pg_dump/pg_restore.c | 8 +++-
src/bin/pg_upgrade/option.c| 5 +++--
src/bin/pgbench/pgbench.c | 33 +
8 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 51509d150e..b51b62cc21 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2072,6 +2072,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2212,8 +2213,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+compresslevel = strtol(optarg, &strtol_endptr, 10);
+if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2251,8 +2252,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
-standby_message_timeout = atoi(optarg) * 1000;
-if (standby_message_timeout < 0)
+standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 710a33ab4d..c1651961b5 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -494,6 +494,7 @@ main(int argc, char **argv)
int c;
int option_index;
char *db_name;
+ char *strtol_endptr = NULL;
uint32 hi, lo;
progname = get_progname(argv[0]);
@@ -529,7 +530,7 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
-if (atoi(optarg) <= 0)
+if ((strtol(optarg, &strtol_endptr, 10) <= 0) || (strtol_endptr != optar
Re: [HACKERS] Bug with pg_basebackup and 'shared' tablespace
On Wednesday, September 13, 2017 6:01:43 PM CEST you wrote:
> > On 15 May 2017, at 07:26, Michael Paquier
> > wrote:>
> > On Tue, May 2, 2017 at 2:28 AM, Pierre Ducroquet
wrote:
> >> I will submit this patch in the current commit fest.
> >
> > I have not spotted any flaws in the refactored logic.
>
> This patch no longer applies, could you take a look at it and submit a new
> version rebased on top of HEAD?
>
> cheers ./daniel
Hi
All my apologies for the schockingly long time with no answer on this topic.
Attached to this email is the new version of the patch, checked against HEAD
and REL_10_STABLE, with the small change required by Michael (affect true/
false to the boolean instead of 1/0) applied.
I will do my best to help review some patches in the current CF.
Pierre>From cfb47eb5db398c1a30c5f83c680b59b4aa3d196a Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet
Date: Wed, 13 Sep 2017 19:09:32 +0200
Subject: [PATCH] Allow a pg_basebackup when a tablespace is shared between two
versions
When a tablespace folder is shared between two PostgreSQL versions,
pg_basebackup fails because its tablespace folder checking is stricter
than what is done in the server.
That behaviour makes it possible to create clusters that will then be
complicated to replicate whithout playing with symlinks.
This patch fixes this by delaying the tablespace folder verification.
The folder name is using the PG catalog version, that can not be
obtained from the server. It is a compile-time constant that is not
exposed publicly.
The fix is thus to simply delay the checking of folders and use the
folder name from the tablespace tarball.
---
src/bin/pg_basebackup/pg_basebackup.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index dfb9b5ddcb..080468d846 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1310,6 +1310,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
pgoff_t current_len_left = 0;
int current_padding = 0;
bool basetablespace;
+ boolfirstfile = 1;
char *copybuf = NULL;
FILE *file = NULL;
@@ -1403,7 +1404,15 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
* Directory
*/
filename[strlen(filename) - 1] = '\0'; /* Remove trailing slash */
- if (mkdir(filename, S_IRWXU) != 0)
+ if (firstfile && !basetablespace)
+ {
+ /*
+ * The first file in the tablespace is its main folder, whose name can not be guessed (PG_MAJORVER_CATVER)
+ * So we must check here that this folder can be created or is empty.
+ */
+ verify_dir_is_empty_or_create(filename, &made_tablespace_dirs, &found_tablespace_dirs);
+ }
+ else if (mkdir(filename, S_IRWXU) != 0)
{
/*
* When streaming WAL, pg_wal (or pg_xlog for pre-9.6
@@ -1534,6 +1543,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
continue;
}
} /* continuing data in existing file */
+ firstfile = 0; /* mark that we are done with the first file of the tarball */
} /* loop over all data blocks */
progress_report(rownum, filename, true);
@@ -1848,18 +1858,6 @@ BaseBackup(void)
for (i = 0; i < PQntuples(res); i++)
{
totalsize += atol(PQgetvalue(res, i, 2));
-
- /*
- * Verify tablespace directories are empty. Don't bother with the
- * first once since it can be relocated, and it will be checked before
- * we do anything anyway.
- */
- if (format == 'p' && !PQgetisnull(res, i, 1))
- {
- char *path = (char *) get_tablespace_mapping(PQgetvalue(res, i, 1));
-
- verify_dir_is_empty_or_create(path, &made_tablespace_dirs, &found_tablespace_dirs);
- }
}
/*
--
2.14.1
--
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] Bug with pg_basebackup and 'shared' tablespace
On Friday, April 7, 2017 3:12:58 AM CEST, Kyotaro HORIGUCHI wrote:
Hi, Pierre.
Maybe you're the winner:p
At Thu, 06 Apr 2017 12:34:09 +0200, Pierre Ducroquet
wrote in <1714428.BHRm6e8A2D@peanuts2>
On Thursday, April 6, 2017 2:00:55 PM CEST Kyotaro HORIGUCHI wrote: ...
That being said, it is a different matter if the behavior is
preferable. The discussion on the behavior is continued here.
https://www.postgresql.org/message-id/20170406.160844.120459562.horiguchi.kyot...@lab.ntt.co.jp
Right now, there is a conflict between pg_basebackup and the
server since they
do not allow the same behaviour. I can submit a patch either
way, but I won't
decide what is the right way to do it.
I know tricks will allow to work around that issue, I found
them hopefully and
I guess most people affected by this issue would be able to
find and use them,
but nevertheless being able to build a server that can no longer be base-
backuped does not seem right.
regards,
Hi
I didn't have much time to spend on that issue until today, and I found a
way to solve it that seems acceptable to me.
The biggest drawback will be that if the backup is interrupted, previous
tablespaces already copied will stay on disk, but since that issue could
already happen, I don't think it's too much a drawback.
The patch simply delays the empty folder checking until we start reading
the tablespace tarball. The first entry of the tarball being the
PG_MAJOR_CATVER folder, that can not be found otherwise, there is no real
alternative to that.
I will submit this patch in the current commit fest.
Regards
Pierre
From f3e6b078d4159c90237d966c56289cb59b54ede1 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet
Date: Mon, 1 May 2017 11:38:52 +0200
Subject: [PATCH] Allow a pg_basebackup when a tablespace is shared between two
versions
When a tablespace folder is shared between two PostgreSQL versions,
pg_basebackup fails because its tablespace folder checking is stricter
than what is done in the server.
That behaviour makes it possible to create clusters that will then be
complicated to replicate whithout playing with symlinks.
This patch fixes this by delaying the tablespace folder verification.
The folder name is using the PG catalog version, that can not be
obtained from the server. It is a compile-time constant that is not
exposed publicly.
The fix is thus to simply delay the checking of folders and use the
folder name from the tablespace tarball.
---
src/bin/pg_basebackup/pg_basebackup.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index e2a2ebb30f..2e687a2880 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1306,6 +1306,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
pgoff_t current_len_left = 0;
int current_padding = 0;
bool basetablespace;
+ bool firstfile = 1;
char *copybuf = NULL;
FILE *file = NULL;
@@ -1399,7 +1400,15 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
* Directory
*/
filename[strlen(filename) - 1] = '\0'; /* Remove trailing slash */
- if (mkdir(filename, S_IRWXU) != 0)
+ if (firstfile && !basetablespace)
+ {
+ /*
+ * The first file in the tablespace is its main folder, whose name can not be guessed (PG_MAJORVER_CATVER)
+ * So we must check here that this folder can be created or is empty.
+ */
+ verify_dir_is_empty_or_create(filename, &made_tablespace_dirs, &found_tablespace_dirs);
+ }
+ else if (mkdir(filename, S_IRWXU) != 0)
{
/*
* When streaming WAL, pg_wal (or pg_xlog for pre-9.6
@@ -1530,6 +1539,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
continue;
}
} /* continuing data in existing file */
+ firstfile = 0; /* mark that we are done with the first file of the tarball */
} /* loop over all data blocks */
progress_report(rownum, filename, true);
@@ -1844,18 +1854,6 @@ BaseBackup(void)
for (i = 0; i < PQntuples(res); i++)
{
totalsize += atol(PQgetvalue(res, i, 2));
-
- /*
- * Verify tablespace directories are empty. Don't bother with the
- * first once since it can be relocated, and it will be checked before
- * we do anything anyway.
- */
- if (format == 'p' && !PQgetisnull(res, i, 1))
- {
- char *path = (char *) get_tablespace_mapping(PQgetvalue(res, i, 1));
-
- verify_dir_is_empty_or_create(path, &made_tablespace_dirs, &found_tablespace_dirs);
- }
}
/*
--
2.11.0
--
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] Small patch for pg_basebackup argument parsing
On Saturday, April 22, 2017 11:31:58 PM CEST Michael Paquier wrote: > On Sat, Apr 22, 2017 at 11:12 PM, Pierre Ducroquet wrote: > > Following your advice, I went through the source tree and cleaned up most > > instances of that pattern. > > I have attached the corresponding patch to this mail. > > If you think this patch is indeed interesting, what would be the next way > > to have it reviewed ? > > Here are the general guidelines about patch submission: > https://wiki.postgresql.org/wiki/Submitting_a_Patch > And the best thing would be to register it to the next commit fest so > as it does not get lost: > https://commitfest.postgresql.org/ Thank you, I added an entry in the next commit fest. signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] Small patch for pg_basebackup argument parsing
On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote:
> On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet
wrote:
> > On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote:
> >> On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet
> >
> > wrote:
> >> > Yesterday while doing a few pg_basebackup, I realized that the integer
> >> > parameters were not properly checked against invalid input.
> >> > It is not a critical issue, but this could be misleading for an user
> >> > who
> >> > writes -z max or -s 0.5…
> >> > I've attached the patch to this mail. Should I add it to the next
> >> > commit
> >> > fest or is it not needed for such small patches ?
> >>
> >> A call to atoi is actually equivalent to strtol with the rounding:
> >> (int)strtol(str, (char **)NULL, 10);
> >> So I don't think this is worth caring.
> >
> > The problem with atoi is that it simply ignores any invalid input and
> > returns 0 instead.
> > That's why I did this patch, because I did a typo when calling
> > pg_basebackup and was not warned for an invalid input.
>
> I agree. I think it would be worth going through and cleaning up
> every instance of this in the source tree.
Hi
Following your advice, I went through the source tree and cleaned up most
instances of that pattern.
I have attached the corresponding patch to this mail.
If you think this patch is indeed interesting, what would be the next way to
have it reviewed ?
Thanks
Pierre
From da448337bd050518323b45fd5d2929d04b552802 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet
Date: Tue, 18 Apr 2017 09:40:40 +0200
Subject: [PATCH] Port most calls of atoi(optarg) to strcol
atoi does not allow any kind of data check. If the input data is invalid,
the result will be silently truncated to the valid part of input, or just
0 if no digit was found at the beginning of input.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++-
src/bin/pg_basebackup/pg_receivewal.c | 11 ++-
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++-
src/bin/pg_ctl/pg_ctl.c| 8 +++-
src/bin/pg_dump/pg_dump.c | 12 +---
src/bin/pg_dump/pg_restore.c | 8 +++-
src/bin/pg_upgrade/option.c| 5 +++--
src/bin/pgbench/pgbench.c | 34 ++
8 files changed, 62 insertions(+), 38 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..34884fce4f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+compresslevel = strtol(optarg, &strtol_endptr, 10);
+if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
-standby_message_timeout = atoi(optarg) * 1000;
-if (standby_message_timeout < 0)
+standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 1a9fe81be1..f470406678 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -479,6 +479,7 @@ main(int argc, char **argv)
int c;
int option_index;
char *db_name;
+char *strtol_endptr = NULL;
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));
@@ -513,7 +514,7 @@ main(int argc, char **argv)
dbhos
Re: [HACKERS] Small patch for pg_basebackup argument parsing
On Friday, April 14, 2017 8:59:03 AM CEST Robert Haas wrote: > On Fri, Apr 14, 2017 at 2:32 AM, Pierre Ducroquet wrote: > > On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote: > >> On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet > > > > wrote: > >> > Yesterday while doing a few pg_basebackup, I realized that the integer > >> > parameters were not properly checked against invalid input. > >> > It is not a critical issue, but this could be misleading for an user > >> > who > >> > writes -z max or -s 0.5… > >> > I've attached the patch to this mail. Should I add it to the next > >> > commit > >> > fest or is it not needed for such small patches ? > >> > >> A call to atoi is actually equivalent to strtol with the rounding: > >> (int)strtol(str, (char **)NULL, 10); > >> So I don't think this is worth caring. > > > > The problem with atoi is that it simply ignores any invalid input and > > returns 0 instead. > > That's why I did this patch, because I did a typo when calling > > pg_basebackup and was not warned for an invalid input. > > I agree. I think it would be worth going through and cleaning up > every instance of this in the source tree. Well, I don't have much to do during a train travel next week. I'll look into it and post my results here. signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] Small patch for pg_basebackup argument parsing
On Friday, April 14, 2017 8:44:37 AM CEST Michael Paquier wrote: > On Fri, Apr 14, 2017 at 6:32 AM, Pierre Ducroquet wrote: > > Yesterday while doing a few pg_basebackup, I realized that the integer > > parameters were not properly checked against invalid input. > > It is not a critical issue, but this could be misleading for an user who > > writes -z max or -s 0.5… > > I've attached the patch to this mail. Should I add it to the next commit > > fest or is it not needed for such small patches ? > > A call to atoi is actually equivalent to strtol with the rounding: > (int)strtol(str, (char **)NULL, 10); > So I don't think this is worth caring. The problem with atoi is that it simply ignores any invalid input and returns 0 instead. That's why I did this patch, because I did a typo when calling pg_basebackup and was not warned for an invalid input. But it doesn't matter that much, so if you don't think that's interesting, no problem. signature.asc Description: This is a digitally signed message part.
[HACKERS] Small patch for pg_basebackup argument parsing
Hi
Yesterday while doing a few pg_basebackup, I realized that the integer
parameters were not properly checked against invalid input.
It is not a critical issue, but this could be misleading for an user who
writes -z max or -s 0.5…
I've attached the patch to this mail. Should I add it to the next commit fest
or is it not needed for such small patches ?
Thanks
PierreFrom c5301085514b8e5acd9ffa5b10ae6521677b4d72 Mon Sep 17 00:00:00 2001
From: Pierre Ducroquet
Date: Thu, 13 Apr 2017 23:25:51 +0200
Subject: [PATCH] Check the results of atoi in pg_basebackup
Passing invalid strings in integer parameters does not trigger any error
in pg_basebackup right now. This behaviour could be misleading and give
the impression that a setting is considered (-z max, -s 0.5) while it
was reset to 0 instead.
---
src/bin/pg_basebackup/pg_basebackup.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e17dc..e69dbd1ed6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -87,7 +87,7 @@ static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
static bool do_sync = true;
-static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+static long int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
static char *replication_slot = NULL;
@@ -2063,6 +2063,7 @@ BaseBackup(void)
int
main(int argc, char **argv)
{
+ char *strtol_endptr = NULL;
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
@@ -2203,8 +2204,8 @@ main(int argc, char **argv)
#endif
break;
case 'Z':
-compresslevel = atoi(optarg);
-if (compresslevel < 0 || compresslevel > 9)
+compresslevel = strtol(optarg, &strtol_endptr, 10);
+if (compresslevel < 0 || compresslevel > 9 || (strtol_endptr != optarg + strlen(optarg))
{
fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
progname, optarg);
@@ -2242,8 +2243,8 @@ main(int argc, char **argv)
dbgetpassword = 1;
break;
case 's':
-standby_message_timeout = atoi(optarg) * 1000;
-if (standby_message_timeout < 0)
+standby_message_timeout = strtol(optarg, &strtol_endptr, 10) * 1000;
+if ((standby_message_timeout < 0) || (strtol_endptr != optarg + strlen(optarg)))
{
fprintf(stderr, _("%s: invalid status interval \"%s\"\n"),
progname, optarg);
--
2.11.0
signature.asc
Description: This is a digitally signed message part.
Re: [HACKERS] Bug with pg_basebackup and 'shared' tablespace
On Thursday, April 6, 2017 2:00:55 PM CEST Kyotaro HORIGUCHI wrote: > At Thu, 06 Apr 2017 00:59:49 +0200, Pierre Ducroquet > wrote in <2008148.rxBNyNRHPZ@peanuts2> > > But it all gets messy when we want to create a streaming standby server > > using pg_basebackup. When backuping Pg 9.5, there is no issue, but > > backuping Pg 9.6 afterwards will say "directory "/mnt/ssd/postgres" > > exists but is not empty". > The documentation says as follows. > > https://www.postgresql.org/docs/9.6/static/manage-ag-tablespaces.html > > | The location must be an existing, empty directory that is owned > | by the PostgreSQL operating system user. > > This explicitly prohibits to share one tablespace directory among > multiple servers. The code is just missing the case of multiple > servers with different versions. I think the bug is rather that > Pg9.6 in the case allowed to create the tablespace. > > The current naming rule of tablespace directory was introduced as > of 9.0 so that pg_upgrade (or pg_migrator at the time) can > perform in-place migration. It is not intended to share a > directory among multiple instances with different versions. > > That being said, an additional trick in the attached file will > work for you. Thanks for your answer. Indeed, either PostgreSQL should enforce that empty folder restriction, or pg_basebackup should lift it and the documentation should reflect this. Right now, there is a conflict between pg_basebackup and the server since they do not allow the same behaviour. I can submit a patch either way, but I won't decide what is the right way to do it. I know tricks will allow to work around that issue, I found them hopefully and I guess most people affected by this issue would be able to find and use them, but nevertheless being able to build a server that can no longer be base- backuped does not seem right. Pierre -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug with pg_basebackup and 'shared' tablespace
Hi On our servers, we are running different PostgreSQL versions because we can not migrate every application at the same time to Pg 9.6… Since we have several disks, we use tablespaces and, for historical reasons, we used the same folder for both Pg versions, say /mnt/ssd/postgres The server has absolutely no issue with that, there is not a single warning when running CREATE TABLESPACE. But it all gets messy when we want to create a streaming standby server using pg_basebackup. When backuping Pg 9.5, there is no issue, but backuping Pg 9.6 afterwards will say "directory "/mnt/ssd/postgres" exists but is not empty". The attached script to this mail will easily reproduce that issue if you did not understand me. Just... read it before launching it, it's a dirty script to reproduce the issue. :) I have looked a bit at the pg_basebackup code and the replication protocol, and I did not find any simple way to solve the issue. PostgreSQL use the CATALOG_VERSION_NO to have one folder per Pg version, but that constant is not accessible out of the PostgreSQL code and is not exposed in the replication protocol as described in the documentation https://www.postgresql.org/docs/current/static/protocol-replication.html The only easy way I see to fix that issue is to alter the replication protocol to expose the CATALOG_VERSION_NO constant, making it possible for pg_basebackup to know the proper path to check. Another way would be to change the pg_basebackup logic and backup the main data folder in order to be able to read the pg_control file… but this seems ugly too. I am willing to write the patch for this issue, but I would appreciate some help to find a proper way to fix it. Thanks Pierre reproduce-bug-tblspace.sh Description: application/shellscript signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki
On Friday, March 31, 2017 7:17:08 AM CEST, Jan Michálek wrote: 2017-03-30 21:53 GMT+02:00 Pavel Stehule : 2017-03-29 20:11 GMT+02:00 Jan Michálek : 2017-03-27 19:41 GMT+02:00 Jan Michálek : 2017-03-23 17:26 GMT+01:00 Pierre Ducroquet : The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Hi This is my first review (Magnus said in his presentation in PGDay Paris that volunteers should just come and help, so here I am), so please notify me for any mistake I do when using the review tools... The feature seems to work as expected, but I don't claim to be a markdown and rst expert. Some minor issues with the code itself : - some indentation issues (documentation and code itself with mix between space based and tab based indentation) and a few trailing spaces in code corrected - typographic issues in the documentation : - "The html, asciidoc, latex, latex-longtable, troff-ms, and markdown and rst formats" ==> duplicated and corrected - "Sets the output format to one of unaligned, aligned, wrapped, html, asciidoc, latex (uses tabular), latex-longtable, rst, markdown, or troff-ms." ==> extra comma at the end of the list - the comment " dont add line after last row, because line is added after every row" is misleading, it should warn that it's only for rst - there is a block of commented out code left - in the print_aligned_vertical function, there is a mix between "cont->opt->format == PRINT_RST" and "format == &pg_rst" and I don't see any obvious reason for that corrected - the documentation doesn't mention (but ok, it's kind of obvious) that the linestyle option will not work with rst and markdown In this patch are corrected (i hope, i had correct changes in vimrc) indentation issues. Plese, look at this if it is OK (i men indentats) and some minor errors. And it should work on current master (probably). Added \x option form markdown In markdown works multiline cels (newline replaced by ) regre tests passed \pset format rst \x select 10 crash on segfault Program received signal SIGSEGV, Segmentation fault. 0x7f77673a866c in vfprintf () from /lib64/libc.so.6 (gdb) bt #0 0x7f77673a866c in vfprintf () from /lib64/libc.so.6 #1 0x7f77673b1574 in fprintf () from /lib64/libc.so.6 #2 0x00437bc5 in print_aligned_vertical (cont=0x7fffade43da0, fout=, is_pager=) at print.c:1755 #3 0x0043a70d in printTable (cont=cont@entry=0x7fffade43da0, fout=, fout@entry=0x7f77677255e0 <_IO_2_1_stdout_>, is_pager=, is_pager@entry=0 '\000', flog=flog@entry=0x0) at print.c:3466 #4 0x0043c37f in printQuery (result=result@entry=0x9c4b60, opt=opt@entry=0x7fffade43f00, fout=0x7f77677255e0 <_IO_2_1_stdout_>, is_pager=is_pager@entry=0 '\000', flog=0x0) at print.c:3551 #5 0x0040da6d in PrintQueryTuples (results=0x9c4b60) at common.c:808 #6 PrintQueryResults (results=0x9c4b60) at common.c:1140 #7 SendQuery (query=0x9c1700 "select 10;") at common.c:1317 #8 0x0041c3d4 in MainLoop (source=0x7f77677248a0 <_IO_2_1_stdin_>) at mainloop.c:319 #9 0x00405d5d in main (argc=, argv=) at startup.c:396 Regards On source from monday it works (last commit on master I have is from 27.3 14:30). Or, maybe I didn`t generate diff well, or some gitt issue. I agree with Pavel, there is a segfault when you do these with your current patch. The current patch does not pass make check-world. How did you generate the diff ? Basically, the simplest way to generate a patch serie is through git format-patch. For instance, say you have a rstFormat branch freshly rebased upon origin/master, just do git format-patch origin/master..rstFormat and you will have one patch file per commit. And don't forget to commit everything :) I hope this helps, I don't have enough time to go through the patch and find out what is causing the segfault right now. -- 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] Other formats in pset like markdown, rst, mediawiki
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed Hi This is my first review (Magnus said in his presentation in PGDay Paris that volunteers should just come and help, so here I am), so please notify me for any mistake I do when using the review tools... The feature seems to work as expected, but I don't claim to be a markdown and rst expert. Some minor issues with the code itself : - some indentation issues (documentation and code itself with mix between space based and tab based indentation) and a few trailing spaces in code - typographic issues in the documentation : - "The html, asciidoc, latex, latex-longtable, troff-ms, and markdown and rst formats" ==> duplicated and - "Sets the output format to one of unaligned, aligned, wrapped, html, asciidoc, latex (uses tabular), latex-longtable, rst, markdown, or troff-ms." ==> extra comma at the end of the list - the comment " dont add line after last row, because line is added after every row" is misleading, it should warn that it's only for rst - there is a block of commented out code left - in the print_aligned_vertical function, there is a mix between "cont->opt->format == PRINT_RST" and "format == &pg_rst" and I don't see any obvious reason for that - the documentation doesn't mention (but ok, it's kind of obvious) that the linestyle option will not work with rst and markdown Thanks ! The new status of this patch is: Waiting on Author -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
