Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Thu, Jan 3, 2013 at 1:33 PM, Boszormenyi Zoltan z...@cybertec.at wrote: 2013-01-02 11:54 keltezéssel, Boszormenyi Zoltan írta: 2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta: 2013-01-02 10:12 keltezéssel, Magnus Hagander írta: Attached is the quotes-v2 patch, the function is renamed and the comment is modified plus the pg_basebackup v21 patch that uses this function. Rebased after pgtar.h was added. The quotes.c comment was modified even more so it doesn't say this function is used for SQL string literals. Quotes patch applied with a few small changes: 1) Support for the msvc build (needs an entry added for new files that go in src/port if they should be used on Windows) 2) Makefile entries should be alphabetical (yes, that's really trivial nitpicking :D) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Sat, Jan 5, 2013 at 3:41 PM, Magnus Hagander mag...@hagander.net wrote: On Thu, Jan 3, 2013 at 1:33 PM, Boszormenyi Zoltan z...@cybertec.at wrote: 2013-01-02 11:54 keltezéssel, Boszormenyi Zoltan írta: 2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta: 2013-01-02 10:12 keltezéssel, Magnus Hagander írta: Attached is the quotes-v2 patch, the function is renamed and the comment is modified plus the pg_basebackup v21 patch that uses this function. Rebased after pgtar.h was added. The quotes.c comment was modified even more so it doesn't say this function is used for SQL string literals. Quotes patch applied with a few small changes: 1) Support for the msvc build (needs an entry added for new files that go in src/port if they should be used on Windows) 2) Makefile entries should be alphabetical (yes, that's really trivial nitpicking :D) After some further modifications, I've applied the pg_basebackup patch as well. Thanks! And again, apologies for dragging the process out longer than should've been necessary. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-05 16:58 keltezéssel, Magnus Hagander írta: On Sat, Jan 5, 2013 at 3:41 PM, Magnus Hagander mag...@hagander.net wrote: On Thu, Jan 3, 2013 at 1:33 PM, Boszormenyi Zoltan z...@cybertec.at wrote: 2013-01-02 11:54 keltezéssel, Boszormenyi Zoltan írta: 2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta: 2013-01-02 10:12 keltezéssel, Magnus Hagander írta: Attached is the quotes-v2 patch, the function is renamed and the comment is modified plus the pg_basebackup v21 patch that uses this function. Rebased after pgtar.h was added. The quotes.c comment was modified even more so it doesn't say this function is used for SQL string literals. Quotes patch applied with a few small changes: 1) Support for the msvc build (needs an entry added for new files that go in src/port if they should be used on Windows) 2) Makefile entries should be alphabetical (yes, that's really trivial nitpicking :D) After some further modifications, I've applied the pg_basebackup patch as well. Thank you very much! Thanks! And again, apologies for dragging the process out longer than should've been necessary. I blamed it on me not having done reviews earlier in the commitfest, which I finally did last week. Now, if only Tom could find some time to review the lock_timeout patch... ;-) Thanks again and best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-02 11:54 keltezéssel, Boszormenyi Zoltan írta:
2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta:
2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan z...@cybertec.at
mailto:z...@cybertec.at wrote:
2013-01-02 01:24 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at mailto:z...@cybertec.at writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory
allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either
explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.
Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.
I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.
Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's
good or bad features.
I like Magnus' version a lot better than that idea.
OK, I will post the core patch building on his code.
Thanks.
A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions. If we're going to be
putting it into frontend programs, how safe is that going to be?
regards, tom lane
The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?
We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.
There are quite a few far-eastern encodings that aren't available as server
encondings, and I believe it's all for this reason.
I see, thanks.
That said, do we need to care *in this specific case*? We use it in initdb to parse
config strings, I believe. And we'd use it to parse a conninfo string in
pg_basebackup, correct?
Correct.
Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii
safe encodings, or rename it to indicate that it's limited in this?
If you send a new patch with the function renamed accordingly, I will modify
my code to use it.
Attached is the quotes-v2 patch, the function is renamed and
the comment is modified plus the pg_basebackup v21 patch
that uses this function.
Rebased after pgtar.h was added. The quotes.c comment was modified
even more so it doesn't say this function is used for SQL string literals.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff -durpN postgresql/src/bin/initdb/initdb.c postgresql.1/src/bin/initdb/initdb.c
--- postgresql/src/bin/initdb/initdb.c 2013-01-02 09:19:03.855521804 +0100
+++ postgresql.1/src/bin/initdb/initdb.c 2013-01-03 11:31:10.819564769 +0100
@@ -354,6 +354,18 @@ pg_strdup(const char *s)
return result;
}
+static char *
+escape_quotes(const char *src)
+{
+ char *result = escape_single_quotes_ascii(src);
+ if (!result)
+ {
+ fprintf(stderr, _(%s: out of memory\n), progname);
+ exit(1);
+ }
+ return result;
+}
+
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
@@ -2415,35 +2427,6 @@ check_ok(void)
}
}
-/*
- * Escape (by doubling) any single quotes or backslashes in given string
- *
- * Note: this is used to process both postgresql.conf entries and SQL
- * string literals. Since postgresql.conf strings are defined to treat
- * backslashes as escapes, we have to double
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-02 01:24 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at writes: 2013-01-01 17:18 keltezéssel, Magnus Hagander írta: That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached. Sure it's simpler but then the consistent look of the code is lost. What about the other patch to unify pg_malloc and friends? Basically all client code boils down to fprintf(stderr, ...) in different disguise in their error reporting, so that patch can also be simplified but it seems that the atexit() - either explicitly or hidden behind InitPostgresFrontend() - cannot be avoided. Meh. I find it seriously wrongheaded that something as minor as an escape_quotes() function should get to dictate both malloc wrappers and error recovery handling throughout every program that might use it. Actually, the unification of pg_malloc and friends wasn't dictated by this little code, it was just that pg_basebackup doesn't provide a pg_malloc implementation (only pg_malloc0) that is used by initdb's escape_quotes() function. Then I noticed how wide these almost identical functions have spread into client apps already. I would say this unification patch is completely orthogonal to the patch in $SUBJECT. I will post it in a different thread if it's wanted at all. The extra atexit() handler is not needed if a simple fprintf(stderr, ...) error reporting is enough in all clients. As far as I saw, all clients do exactly this but some of them hide this behind #define's. I like Magnus' version a lot better than that idea. OK, I will post the core patch building on his code. A bigger issue that I notice with this code is that it's only correct in backend-safe encodings, as the comment mentions. If we're going to be putting it into frontend programs, how safe is that going to be? regards, tom lane The question in a different form is: does PostgreSQL support non-ASCII-safe encodings at all (or on the client side)? Forgive my ignorance and enlighten me: how many such encodings exist besides EBCDIC? Is UTF-16 non-ASCII-safe? Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2013-01-02 01:24 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at writes: 2013-01-01 17:18 keltezéssel, Magnus Hagander írta: That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached. Sure it's simpler but then the consistent look of the code is lost. What about the other patch to unify pg_malloc and friends? Basically all client code boils down to fprintf(stderr, ...) in different disguise in their error reporting, so that patch can also be simplified but it seems that the atexit() - either explicitly or hidden behind InitPostgresFrontend() - cannot be avoided. Meh. I find it seriously wrongheaded that something as minor as an escape_quotes() function should get to dictate both malloc wrappers and error recovery handling throughout every program that might use it. Actually, the unification of pg_malloc and friends wasn't dictated by this little code, it was just that pg_basebackup doesn't provide a pg_malloc implementation (only pg_malloc0) that is used by initdb's escape_quotes() function. Then I noticed how wide these almost identical functions have spread into client apps already. I would say this unification patch is completely orthogonal to the patch in $SUBJECT. I will post it in a different thread if it's wanted at all. The extra atexit() handler is not needed if a simple fprintf(stderr, ...) error reporting is enough in all clients. As far as I saw, all clients do exactly this but some of them hide this behind #define's. Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features. I like Magnus' version a lot better than that idea. OK, I will post the core patch building on his code. Thanks. A bigger issue that I notice with this code is that it's only correct in backend-safe encodings, as the comment mentions. If we're going to be putting it into frontend programs, how safe is that going to be? regards, tom lane The question in a different form is: does PostgreSQL support non-ASCII-safe encodings at all (or on the client side)? Forgive my ignorance and enlighten me: how many such encodings exist besides EBCDIC? Is UTF-16 non-ASCII-safe? We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html. There are quite a few far-eastern encodings that aren't available as server encondings, and I believe it's all for this reason. That said, do we need to care *in this specific case*? We use it in initdb to parse config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, correct? Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe encodings, or rename it to indicate that it's limited in this? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-02 10:12 keltezéssel, Magnus Hagander írta: On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan z...@cybertec.at mailto:z...@cybertec.at wrote: 2013-01-02 01:24 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at mailto:z...@cybertec.at writes: 2013-01-01 17:18 keltezéssel, Magnus Hagander írta: That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached. Sure it's simpler but then the consistent look of the code is lost. What about the other patch to unify pg_malloc and friends? Basically all client code boils down to fprintf(stderr, ...) in different disguise in their error reporting, so that patch can also be simplified but it seems that the atexit() - either explicitly or hidden behind InitPostgresFrontend() - cannot be avoided. Meh. I find it seriously wrongheaded that something as minor as an escape_quotes() function should get to dictate both malloc wrappers and error recovery handling throughout every program that might use it. Actually, the unification of pg_malloc and friends wasn't dictated by this little code, it was just that pg_basebackup doesn't provide a pg_malloc implementation (only pg_malloc0) that is used by initdb's escape_quotes() function. Then I noticed how wide these almost identical functions have spread into client apps already. I would say this unification patch is completely orthogonal to the patch in $SUBJECT. I will post it in a different thread if it's wanted at all. The extra atexit() handler is not needed if a simple fprintf(stderr, ...) error reporting is enough in all clients. As far as I saw, all clients do exactly this but some of them hide this behind #define's. Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's good or bad features. I like Magnus' version a lot better than that idea. OK, I will post the core patch building on his code. Thanks. A bigger issue that I notice with this code is that it's only correct in backend-safe encodings, as the comment mentions. If we're going to be putting it into frontend programs, how safe is that going to be? regards, tom lane The question in a different form is: does PostgreSQL support non-ASCII-safe encodings at all (or on the client side)? Forgive my ignorance and enlighten me: how many such encodings exist besides EBCDIC? Is UTF-16 non-ASCII-safe? We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html. There are quite a few far-eastern encodings that aren't available as server encondings, and I believe it's all for this reason. I see, thanks. That said, do we need to care *in this specific case*? We use it in initdb to parse config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup, correct? Correct. Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii safe encodings, or rename it to indicate that it's limited in this? If you send a new patch with the function renamed accordingly, I will modify my code to use it. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-02 10:37 keltezéssel, Boszormenyi Zoltan írta:
2013-01-02 10:12 keltezéssel, Magnus Hagander írta:
On Wed, Jan 2, 2013 at 9:59 AM, Boszormenyi Zoltan z...@cybertec.at
mailto:z...@cybertec.at wrote:
2013-01-02 01:24 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at mailto:z...@cybertec.at writes:
2013-01-01 17:18 keltezéssel, Magnus Hagander írta:
That way we can get around the whole need for changing memory
allocation across all the
frontends, no? Like the attached.
Sure it's simpler but then the consistent look of the code is lost.
What about the other patch to unify pg_malloc and friends?
Basically all client code boils down to
fprintf(stderr, ...)
in different disguise in their error reporting, so that patch can
also be simplified but it seems that the atexit() - either
explicitly
or hidden behind InitPostgresFrontend() - cannot be avoided.
Meh. I find it seriously wrongheaded that something as minor as an
escape_quotes() function should get to dictate both malloc wrappers
and error recovery handling throughout every program that might use it.
Actually, the unification of pg_malloc and friends wasn't dictated
by this little code, it was just that pg_basebackup doesn't provide
a pg_malloc implementation (only pg_malloc0) that is used by
initdb's escape_quotes() function. Then I noticed how wide these
almost identical functions have spread into client apps already.
I would say this unification patch is completely orthogonal to
the patch in $SUBJECT. I will post it in a different thread if it's
wanted at all. The extra atexit() handler is not needed if a simple
fprintf(stderr, ...) error reporting is enough in all clients.
As far as I saw, all clients do exactly this but some of them hide
this behind #define's.
Please do keep that one separate - let's avoid unnecessary feature-creep, whether it's
good or bad features.
I like Magnus' version a lot better than that idea.
OK, I will post the core patch building on his code.
Thanks.
A bigger issue that I notice with this code is that it's only correct in
backend-safe encodings, as the comment mentions. If we're going to be
putting it into frontend programs, how safe is that going to be?
regards, tom lane
The question in a different form is: does PostgreSQL support
non-ASCII-safe encodings at all (or on the client side)? Forgive
my ignorance and enlighten me: how many such encodings
exist besides EBCDIC? Is UTF-16 non-ASCII-safe?
We do. See http://www.postgresql.org/docs/9.2/static/multibyte.html.
There are quite a few far-eastern encodings that aren't available as server encondings,
and I believe it's all for this reason.
I see, thanks.
That said, do we need to care *in this specific case*? We use it in initdb to parse
config strings, I believe. And we'd use it to parse a conninfo string in pg_basebackup,
correct?
Correct.
Perhaps all we need to do is to clearly comment that it doesn't work with non-ascii
safe encodings, or rename it to indicate that it's limited in this?
If you send a new patch with the function renamed accordingly, I will modify
my code to use it.
Attached is the quotes-v2 patch, the function is renamed and
the comment is modified plus the pg_basebackup v21 patch
that uses this function.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff -durpN postgresql/src/bin/initdb/initdb.c postgresql.1/src/bin/initdb/initdb.c
--- postgresql/src/bin/initdb/initdb.c 2013-01-02 09:19:03.855521804 +0100
+++ postgresql.1/src/bin/initdb/initdb.c 2013-01-02 11:26:10.008494149 +0100
@@ -354,6 +354,18 @@ pg_strdup(const char *s)
return result;
}
+static char *
+escape_quotes(const char *src)
+{
+ char *result = escape_single_quotes_ascii(src);
+ if (!result)
+ {
+ fprintf(stderr, _(%s: out of memory\n), progname);
+ exit(1);
+ }
+ return result;
+}
+
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
@@ -2415,35 +2427,6 @@ check_ok(void)
}
}
-/*
- * Escape (by doubling) any single quotes or backslashes in given string
- *
- * Note: this is used to process both postgresql.conf entries and SQL
- * string literals. Since postgresql.conf strings are defined to treat
- * backslashes as escapes, we have to double backslashes here. Hence,
- * when using this for a SQL string literal, use E'' syntax.
- *
- * We do not need to worry about encoding considerations because all
- * valid backend encodings are ASCII-safe.
-
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan z...@cybertec.at wrote: Hi, now that PQconninfo() was committed, I rebased the subsequent patches. Actual code change was only in the last patch, to conform to the committed PQconninfo() API. So, finally coming back to this one. What happened to Alvaro's suggestion of: It seems simpler to have the escape_quotes utility function in port just not use pg_malloc at all, have it return NULL or similar failure indicator when malloc() fails, and then the caller decides what to do. That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ quotes.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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan z...@cybertec.at wrote: Hi, now that PQconninfo() was committed, I rebased the subsequent patches. Actual code change was only in the last patch, to conform to the committed PQconninfo() API. I've applied a modified version of the tar unification patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now). I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last. Given that the code there is already fairly difficult to track, I think just keeping it simpler is definitely worth doing one of those two. I would vote for just leaving two copies of recovery.conf in there. Comments/thoughts from others? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-01 18:25 keltezéssel, Magnus Hagander írta: On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan z...@cybertec.at mailto:z...@cybertec.at wrote: Hi, now that PQconninfo() was committed, I rebased the subsequent patches. Actual code change was only in the last patch, to conform to the committed PQconninfo() API. I've applied a modified version of the tar unification patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now). Thanks. I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last. IIRC, Fujii Masao's wish was to remove the recovery.conf from the tar stream. I know that the tar format is perfectly happy with two files with the same path name but his reasoning was that GUIs (like WinRar) may get confused by two such files and cannot decide which one to extract. I didn't actually test this but it is a reasonable suspicion. Passing a parameter to the walsender may be a better solution. It's a bad idea only because it wasn't mine. ;-) The only problem with this idea is that currently there's nothing that stops pg_basebackup to be a generic and backwards-compatible tool. It simply receives a TAR stream via plain PQgetCopyData() and optionally extracts it. The client-side solution to skip the recovery.conf file keeps this backward compatible feature. I tested this and pg_basebackup from 9.3dev happily backs up a 9.2.2 database... Given that the code there is already fairly difficult to track, I think just keeping it simpler is definitely worth doing one of those two. I would vote for just leaving two copies of recovery.conf in there. Comments/thoughts from others? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ Best regards, Zoltán Böszörmény -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Tue, Jan 1, 2013 at 7:13 PM, Boszormenyi Zoltan z...@cybertec.at wrote: 2013-01-01 18:25 keltezéssel, Magnus Hagander írta: On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan z...@cybertec.atwrote: Hi, now that PQconninfo() was committed, I rebased the subsequent patches. Actual code change was only in the last patch, to conform to the committed PQconninfo() API. I've applied a modified version of the tar unification patch to master. It didn't apply cleanly so I ended up redoing a number of the things manually, but the end result is fairly similar. I don't think it'll cause anything but really trivial merge conflicts against the 04 patch, but I didn't actually check that (e.g. it's tarCreateHeader() not _tarCreateHeader() now). Thanks. I took at look at the basebackup patch as well. Easier to get now that it's commented :) There's quite a lot of code in there whereby it tries to remove recovery.conf from the tar stream if it's already in there. That seems like an ugly way to do it. I see two other options, that I think both are better. If we do need it to be removed, we should probably pass that as a parameter up to the walsender, and make sure the file isn't included in the first place. But we can also leave it in there - the tar format is perfectly happy to have multiple copies of the same file in the archive - it'll just use whichever copy comes last. IIRC, Fujii Masao's wish was to remove the recovery.conf from the tar stream. I know that the tar format is perfectly happy with two files with the same path name but his reasoning was that GUIs (like WinRar) may get confused by two such files and cannot decide which one to extract. I didn't actually test this but it is a reasonable suspicion. Hmm. Somehow, I missed that part of the discussion. Sorry, didn't realize it had been mentioned already. Passing a parameter to the walsender may be a better solution. It's a bad idea only because it wasn't mine. ;-) The only problem with this idea is that currently there's nothing that stops pg_basebackup to be a generic and backwards-compatible tool. It simply receives a TAR stream via plain PQgetCopyData() and optionally extracts it. The client-side solution to skip the recovery.conf file keeps this backward compatible feature. I tested this and pg_basebackup from 9.3dev happily backs up a 9.2.2 database... I thought commit add6c3179a4d4fa3e62dd3e86a00f23303336bac at leasdt broke that? In particular, did you test where any of those values were different between client and server? Or maybe just didn't test in -x mode? I actually thought there was something else that broke the compatibility, but I can't seem to find it so perhaps that was something that was never committed. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2013-01-01 17:18 keltezéssel, Magnus Hagander írta: On Fri, Nov 30, 2012 at 10:13 AM, Boszormenyi Zoltan z...@cybertec.at mailto:z...@cybertec.at wrote: Hi, now that PQconninfo() was committed, I rebased the subsequent patches. Actual code change was only in the last patch, to conform to the committed PQconninfo() API. So, finally coming back to this one. What happened to Alvaro's suggestion of: It seems simpler to have the escape_quotes utility function in port just not use pg_malloc at all, have it return NULL or similar failure indicator when malloc() fails, and then the caller decides what to do. I seem to have skipped it while reading my mails, I don't remember this suggestion at all. Sorry. That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached. Sure it's simpler but then the consistent look of the code is lost. What about the other patch to unify pg_malloc and friends? Basically all client code boils down to fprintf(stderr, ...) in different disguise in their error reporting, so that patch can also be simplified but it seems that the atexit() - either explicitly or hidden behind InitPostgresFrontend() - cannot be avoided. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
Boszormenyi Zoltan z...@cybertec.at writes: 2013-01-01 17:18 keltezéssel, Magnus Hagander írta: That way we can get around the whole need for changing memory allocation across all the frontends, no? Like the attached. Sure it's simpler but then the consistent look of the code is lost. What about the other patch to unify pg_malloc and friends? Basically all client code boils down to fprintf(stderr, ...) in different disguise in their error reporting, so that patch can also be simplified but it seems that the atexit() - either explicitly or hidden behind InitPostgresFrontend() - cannot be avoided. Meh. I find it seriously wrongheaded that something as minor as an escape_quotes() function should get to dictate both malloc wrappers and error recovery handling throughout every program that might use it. I like Magnus' version a lot better than that idea. A bigger issue that I notice with this code is that it's only correct in backend-safe encodings, as the comment mentions. If we're going to be putting it into frontend programs, how safe is that going to be? 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-11-20 20:32 keltezéssel, Boszormenyi Zoltan írta: 2012-11-20 17:03 keltezéssel, Boszormenyi Zoltan írta: 2012-11-18 17:20 keltezéssel, Magnus Hagander írta: Much of the tar stuff is very similar (I haven't looked to see if it's identical) to the stuff in backend/replication/basebackup.c. Perhaps we should have a src/port/tarutil.c? I will implement it as a separate patch. I implemented it, all 3 patches are attached now. Use this patchset, the previously sent 1st patch had a bug, it called conninit_storeval() with value == NULL arguments and it crashes on strdup(NULL). escape_string() - already exists as escape_quotes() in initdb, AFAICT. We should either move it to src/port/, or at least copy it so it's exactly the same. A copy of escape_quotes() is now in pg_basebackup.c and is used. I will also unify the copies of it in a separate patch. This one is not done yet, but a suggestion on which existing file it can fit into or for a new src/port/filename is welcome. I experimented with unifying escape_quotes() shortly. The problem is that it calls pg_malloc() which is an executable-specific function. Some of the bin/* executables define it as calling exit(1) when malloc() returns NULL, some call it with exit(EXIT_FAILURE) which happens to be 1 but still can be different from the constant 1. Some executables only define pg_malloc0() but not pg_malloc(). pg_basebackup needs pg_malloc() to call disconnect_and_exit(1) instead to quit cleanly and not leave an unexpected EOF from client message in the server log. Which is a macro at the moment, but has to be turned into a real function for the reasons below. To unify escape_quotes(), pg_malloc() need to be also unified. But the diverse requirements for pg_malloc() from different executables means that both the escape_quotes() and the pg_malloc() interface need to be extended with the exit function they have to call and the argument to pass to the exit function. Unless we don't care about the bug reports about unexpected EOF from client messages. Frankly, it doesn't worth the effort. Let's just keep the two separate copies of escape_quotes(). BTW, it seems to me that this unify even the least used functions mentality was not considered to be a great feature during the introduction of pg_malloc(), which is used in far more code than the TAR functions. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
Boszormenyi Zoltan wrote: The problem is that it calls pg_malloc() which is an executable-specific function. Some of the bin/* executables define it as calling exit(1) when malloc() returns NULL, some call it with exit(EXIT_FAILURE) which happens to be 1 but still can be different from the constant 1. Some executables only define pg_malloc0() but not pg_malloc(). It seems simpler to have the escape_quotes utility function in port just not use pg_malloc at all, have it return NULL or similar failure indicator when malloc() fails, and then the caller decides what to do. -- Álvaro Herrerahttp://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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-11-21 14:19 keltezéssel, Alvaro Herrera írta: Boszormenyi Zoltan wrote: The problem is that it calls pg_malloc() which is an executable-specific function. Some of the bin/* executables define it as calling exit(1) when malloc() returns NULL, some call it with exit(EXIT_FAILURE) which happens to be 1 but still can be different from the constant 1. Some executables only define pg_malloc0() but not pg_malloc(). It seems simpler to have the escape_quotes utility function in port just not use pg_malloc at all, have it return NULL or similar failure indicator when malloc() fails, and then the caller decides what to do. $ find . -name *.c | xargs grep pg_malloc | grep -v ^pg_malloc | wc -l 277 Too much work for little gain. Also: - pg_upgrade calls pg_log(PG_FATAL, ...) - pgbench logs a line then calls exit(1) What I can imagine is to introduce a new src/port/ function, InitPostgresFrontend() or something like that. Every executable then calls this function first inside their main() it they want to use pg_malloc() from libpgport.a. This function would accept a pointer to a structure, and the struct contains the pointer to the exit function and the argument too call it with. Other data for different use cases can be added later if needed. This way, the pg_malloc() and friends can be unified and their call interfaces don't have to change. InitPostgresFrontend() needs to be added to only 9 places instead of changing 277 callers of pg_malloc() or pg_malloc0(). BTW, the unified pg_malloc() and friends must be inside #ifdef FRONTEND ... #endif to not leak into the backend code. Opinions? Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
Boszormenyi Zoltan wrote: 2012-11-21 14:19 keltezéssel, Alvaro Herrera írta: Boszormenyi Zoltan wrote: The problem is that it calls pg_malloc() which is an executable-specific function. Some of the bin/* executables define it as calling exit(1) when malloc() returns NULL, some call it with exit(EXIT_FAILURE) which happens to be 1 but still can be different from the constant 1. Some executables only define pg_malloc0() but not pg_malloc(). It seems simpler to have the escape_quotes utility function in port just not use pg_malloc at all, have it return NULL or similar failure indicator when malloc() fails, and then the caller decides what to do. $ find . -name *.c | xargs grep pg_malloc | grep -v ^pg_malloc | wc -l 277 Too much work for little gain. I probably wrote the above in a confusing way. I am not suggesting that pg_malloc is changed in any way. -- Álvaro Herrerahttp://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] [PATCH] Make pg_basebackup configure and start standby [Review]
Boszormenyi Zoltan z...@cybertec.at writes: pg_basebackup needs pg_malloc() to call disconnect_and_exit(1) instead to quit cleanly and not leave an unexpected EOF from client message in the server log. Which is a macro at the moment, but has to be turned into a real function for the reasons below. man 2 atexit 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-11-21 15:29 keltezéssel, Tom Lane írta: Boszormenyi Zoltan z...@cybertec.at writes: pg_basebackup needs pg_malloc() to call disconnect_and_exit(1) instead to quit cleanly and not leave an unexpected EOF from client message in the server log. Which is a macro at the moment, but has to be turned into a real function for the reasons below. man 2 atexit Aww, crap. I knew I forgot about something. :-) Thanks. regards, tom lane -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-11-18 17:20 keltezéssel, Magnus Hagander írta: On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander mag...@hagander.net wrote: On Oct 23, 2012 4:52 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Boszormenyi Zoltan escribió: Also, the check for conflict between -R and -x/-X is now removed. The documentation for option -R has changed to reflect this and there is no different error code 2 now: it would make the behaviour inconsistent between -Fp and -Ft. The PQconninfo patch is also attached but didn't change since the last mail. Magnus, This patch is all yours to handle. I'm guessing nothing will happen until pgconf.eu is done and over, but hopefully you can share a few beers with Zoltan over the whole subject (and maybe with Peter about the PQconninfo stuff?) I'm not closing this just yet, but if you're not able to handle this soon, maybe it'd be better to punt it to the November commitfest. It's on my to do list for when I get back, but correct, won't get to it until after the conference. I finally got around to looking at this patch now. Sorry about the way too long delay. A few thoughts: First, on the libpq patch: I'm not sure I like the for_replication flag to PQconninfo(). It seems we're it's a quite limited API, and not very future proof. What's to say that an app would only be interested in filtering based on for_replication? One idea might be to have a bitmap field there, and assign *all* conninfo options to a category. We could then have categories for NORMAL and REPLICATION. But we could also for example have a category for PASSWORD (or similar), so that you could get with and without those. Passing in a 32-bit integer would allow us to have 32 different categories, and we could then use a bitmask to pick things out. It might sound a bit like overengineering, but it's also an API and it's a PITA to change it in the future if more needs show up.. Check. Second, I wonder if we really need to add the code for requiressl=1, or if we should just remove it. The spelling requiressl=1 was deprecated back in 7.4 - which has obviously been out of support for a long time now. I removed this option, the code is simpler, thanks to this. Third, in fillPGconn. If mapping has both conninfoValue and connvalue, it does a free() on the old value in memberaddr, but if it doesn't it just overwrites memberaddr with strdup(). Shouldn't those two paths be the same, meaning shouldn't the if (*memberaddr) free(*memberaddr); check be outside the if block? This point is now moot, see above. Fourth, I'm not sure I like the memberaddr term. It's not wrong, but it threw me off a couple of times while reading it. It's not all that important, and I'm not sure about another idea for it though - maybe just connmember? The variable is now connmember. Also, I noticed that there was already a conninfo_storeval(), the new patch uses it and there's no need to introduce a new conninfo_setval() function. Then, about the pg_basebackup patch: What's the reason for the () around 512 for TARCHUNKSZ? Removed the () from around the value. We have a lot of hardcoded entries of the 512 for tar in that file. We should either keep using 512 as a constant, or change all those to *also* use the #define. Right now, the code will obviously break if you change the #define (for example, it compares to 512, but then uses hdrleft = TARCHUNKSZ - tarhdrsz; to do calculation). All 512 constants are now using the #define. The name choice of basebackup for the bool in ReceiveTarFile() is unfortunate, since we use the term base backup to refer to the complete thing, not just the main tablespace. Probably basetablespcae instead. And it should then be assigned at the top of the function (to the result of PQgetisnull()), and used in the main if() branch as well. Done without your typo, so the variable is basetablespace. ;-) Should we really print the status message even if not in verbose mode? We only print the base backup complete messages in verbose mode, for example. The message is written only in verbose mode now. It seems wrong to free() recoveryconf in ReceiveTarFile(). It's allocated globally at the beginning. While that loop should only be called once (since only one tablespace can be the main one), it's a confusing location for the free. See below. The whole tar writing part of the code needs a lot more comments. It's entirely unclear what the code does there. Why does the recovery.conf writing code need to be split up in multiple locations inside ReceiveTarFile(), for example? It either needs to be simplified, or explained why it can't be simplified in comments. _tarCreateHeader() is really badly named, since it specifically creates a tar header for recovery.conf only. Either that needs to be a parameter, or it needs to have a name that indicates this is the only thing it does. The former is probably better. _tarCreateHeader() now accepts the file name and the file size arguments. Much
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-11-20 17:03 keltezéssel, Boszormenyi Zoltan írta: 2012-11-18 17:20 keltezéssel, Magnus Hagander írta: The whole tar writing part of the code needs a lot more comments. It's entirely unclear what the code does there. Why does the recovery.conf writing code need to be split up in multiple locations inside ReceiveTarFile(), for example? It either needs to be simplified, or explained why it can't be simplified in comments. I also simplified (the multiple #ifdef blocks are moved out into a function to make the code shorter) and added comments to this function. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander mag...@hagander.net wrote: On Oct 23, 2012 4:52 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Boszormenyi Zoltan escribió: Also, the check for conflict between -R and -x/-X is now removed. The documentation for option -R has changed to reflect this and there is no different error code 2 now: it would make the behaviour inconsistent between -Fp and -Ft. The PQconninfo patch is also attached but didn't change since the last mail. Magnus, This patch is all yours to handle. I'm guessing nothing will happen until pgconf.eu is done and over, but hopefully you can share a few beers with Zoltan over the whole subject (and maybe with Peter about the PQconninfo stuff?) I'm not closing this just yet, but if you're not able to handle this soon, maybe it'd be better to punt it to the November commitfest. It's on my to do list for when I get back, but correct, won't get to it until after the conference. I finally got around to looking at this patch now. Sorry about the way too long delay. A few thoughts: First, on the libpq patch: I'm not sure I like the for_replication flag to PQconninfo(). It seems we're it's a quite limited API, and not very future proof. What's to say that an app would only be interested in filtering based on for_replication? One idea might be to have a bitmap field there, and assign *all* conninfo options to a category. We could then have categories for NORMAL and REPLICATION. But we could also for example have a category for PASSWORD (or similar), so that you could get with and without those. Passing in a 32-bit integer would allow us to have 32 different categories, and we could then use a bitmask to pick things out. It might sound a bit like overengineering, but it's also an API and it's a PITA to change it in the future if more needs show up.. Second, I wonder if we really need to add the code for requiressl=1, or if we should just remove it. The spelling requiressl=1 was deprecated back in 7.4 - which has obviously been out of support for a long time now. Third, in fillPGconn. If mapping has both conninfoValue and connvalue, it does a free() on the old value in memberaddr, but if it doesn't it just overwrites memberaddr with strdup(). Shouldn't those two paths be the same, meaning shouldn't the if (*memberaddr) free(*memberaddr); check be outside the if block? Fourth, I'm not sure I like the memberaddr term. It's not wrong, but it threw me off a couple of times while reading it. It's not all that important, and I'm not sure about another idea for it though - maybe just connmember? Then, about the pg_basebackup patch: What's the reason for the () around 512 for TARCHUNKSZ? We have a lot of hardcoded entries of the 512 for tar in that file. We should either keep using 512 as a constant, or change all those to *also* use the #define. Right now, the code will obviously break if you change the #define (for example, it compares to 512, but then uses hdrleft = TARCHUNKSZ - tarhdrsz; to do calculation). The name choice of basebackup for the bool in ReceiveTarFile() is unfortunate, since we use the term base backup to refer to the complete thing, not just the main tablespace. Probably basetablespcae instead. And it should then be assigned at the top of the function (to the result of PQgetisnull()), and used in the main if() branch as well. Should we really print the status message even if not in verbose mode? We only print the base backup complete messages in verbose mode, for example. It seems wrong to free() recoveryconf in ReceiveTarFile(). It's allocated globally at the beginning. While that loop should only be called once (since only one tablespace can be the main one), it's a confusing location for the free. The whole tar writing part of the code needs a lot more comments. It's entirely unclear what the code does there. Why does the recovery.conf writing code need to be split up in multiple locations inside ReceiveTarFile(), for example? It either needs to be simplified, or explained why it can't be simplified in comments. _tarCreateHeader() is really badly named, since it specifically creates a tar header for recovery.conf only. Either that needs to be a parameter, or it needs to have a name that indicates this is the only thing it does. The former is probably better. Much of the tar stuff is very similar (I haven't looked to see if it's identical) to the stuff in backend/replication/basebackup.c. Perhaps we should have a src/port/tarutil.c? escape_string() - already exists as escape_quotes() in initdb, AFAICT. We should either move it to src/port/, or at least copy it so it's exactly the same. CreateRecoveryConf() should just use PQExpBuffer (in libpq), I think - that does away with a lot of code. We already use this from e.g. pg_dump, so there's a precedent for using internal code from libpq in frontends. Again, my apologies for this review taking so long. I will try to be more
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
Hi, 2012-11-18 17:20 keltezéssel, Magnus Hagander írta: On Tue, Oct 23, 2012 at 5:08 PM, Magnus Hagander mag...@hagander.net wrote: On Oct 23, 2012 4:52 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Boszormenyi Zoltan escribió: Also, the check for conflict between -R and -x/-X is now removed. The documentation for option -R has changed to reflect this and there is no different error code 2 now: it would make the behaviour inconsistent between -Fp and -Ft. The PQconninfo patch is also attached but didn't change since the last mail. Magnus, This patch is all yours to handle. I'm guessing nothing will happen until pgconf.eu is done and over, but hopefully you can share a few beers with Zoltan over the whole subject (and maybe with Peter about the PQconninfo stuff?) I'm not closing this just yet, but if you're not able to handle this soon, maybe it'd be better to punt it to the November commitfest. It's on my to do list for when I get back, but correct, won't get to it until after the conference. I finally got around to looking at this patch now. Sorry about the way too long delay. No problem, thanks for looking at it. A few thoughts: First, on the libpq patch: I'm not sure I like the for_replication flag to PQconninfo(). It seems we're it's a quite limited API, and not very future proof. What's to say that an app would only be interested in filtering based on for_replication? One idea might be to have a bitmap field there, and assign *all* conninfo options to a category. We could then have categories for NORMAL and REPLICATION. But we could also for example have a category for PASSWORD (or similar), so that you could get with and without those. Passing in a 32-bit integer would allow us to have 32 different categories, and we could then use a bitmask to pick things out. It might sound a bit like overengineering, but it's also an API and it's a PITA to change it in the future if more needs show up.. You are right, I will change this accordingly. Second, I wonder if we really need to add the code for requiressl=1, or if we should just remove it. The spelling requiressl=1 was deprecated back in 7.4 - which has obviously been out of support for a long time now. This needs opinions from more people, I am not the one to decide it. The code would be definitely cleaner without processing this extra non-1:1 mapping. Third, in fillPGconn. If mapping has both conninfoValue and connvalue, it does a free() on the old value in memberaddr, but if it doesn't it just overwrites memberaddr with strdup(). Shouldn't those two paths be the same, meaning shouldn't the if (*memberaddr) free(*memberaddr); check be outside the if block? Yes, and set it to NULL too. Otherwise there might be a case when the free() leaves a stale pointer value if the extra mapping fails the strcmp() check. This is all unnecessary if the extra mapping for requiressl=1 is removed, the code would be straight. Fourth, I'm not sure I like the memberaddr term. It's not wrong, but it threw me off a couple of times while reading it. It's not all that important, and I'm not sure about another idea for it though - maybe just connmember? I am not attached to this variable name, I will change it. Then, about the pg_basebackup patch: What's the reason for the () around 512 for TARCHUNKSZ? It's simply a habit, to not forget it for more complex macros. We have a lot of hardcoded entries of the 512 for tar in that file. We should either keep using 512 as a constant, or change all those to *also* use the #define. Right now, the code will obviously break if you change the #define (for example, it compares to 512, but then uses hdrleft = TARCHUNKSZ - tarhdrsz; to do calculation). Yes, I left 5 pieces of the hardcoded value of 512, because I (maybe erroneously) distinguished between a file header and file chunks inside a TAR file, they are all 512. Is it okay to change every hardcoded 512 to TARCHUNKSZ, maybe adding a comment to the #define that it must not be modified ever? The name choice of basebackup for the bool in ReceiveTarFile() is unfortunate, since we use the term base backup to refer to the complete thing, not just the main tablespace. Probably basetablespcae instead. And it should then be assigned at the top of the function (to the result of PQgetisnull()), and used in the main if() branch as well. Will change it. Should we really print the status message even if not in verbose mode? We only print the base backup complete messages in verbose mode, for example. OK. It seems wrong to free() recoveryconf in ReceiveTarFile(). It's allocated globally at the beginning. While that loop should only be called once (since only one tablespace can be the main one), it's a confusing location for the free. The whole tar writing part of the code needs a lot more comments. It's entirely unclear what the code does there. Why does the recovery.conf writing code need to be split up in multiple locations inside
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
Boszormenyi Zoltan escribió: Also, the check for conflict between -R and -x/-X is now removed. The documentation for option -R has changed to reflect this and there is no different error code 2 now: it would make the behaviour inconsistent between -Fp and -Ft. The PQconninfo patch is also attached but didn't change since the last mail. Magnus, This patch is all yours to handle. I'm guessing nothing will happen until pgconf.eu is done and over, but hopefully you can share a few beers with Zoltan over the whole subject (and maybe with Peter about the PQconninfo stuff?) I'm not closing this just yet, but if you're not able to handle this soon, maybe it'd be better to punt it to the November commitfest. -- Álvaro Herrerahttp://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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Oct 23, 2012 4:52 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Boszormenyi Zoltan escribió: Also, the check for conflict between -R and -x/-X is now removed. The documentation for option -R has changed to reflect this and there is no different error code 2 now: it would make the behaviour inconsistent between -Fp and -Ft. The PQconninfo patch is also attached but didn't change since the last mail. Magnus, This patch is all yours to handle. I'm guessing nothing will happen until pgconf.eu is done and over, but hopefully you can share a few beers with Zoltan over the whole subject (and maybe with Peter about the PQconninfo stuff?) I'm not closing this just yet, but if you're not able to handle this soon, maybe it'd be better to punt it to the November commitfest. It's on my to do list for when I get back, but correct, won't get to it until after the conference. /Magnus
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-14 22:31 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 22:26 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta: Hi, 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 18:02 keltezéssel, Fujii Masao írta: Thanks for updating the patch! On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan z...@cybertec.at wrote: Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. The tar file is always extracted such way in all platform which PostgreSQL supports? I'm just concerned about that some tool in some platform might prefer the original recovery.conf when extracting tar file. If the spec of tar format specifies such behavior (i.e., the last written file of the same name is always preferred), it's OK. Since tar is a sequential archive format, I think this is the behaviour of every tar extractor. But I will look at adding code to skip the original recovery.conf if it exists in the tar file. I found the bug that recovery.conf is included in the tar file of the tablespace instead of base.tar, when there are tablespaces in the server. You are right, I am looking into this. But I don't know how it got there, I check for (rownum == 0 writerecoveryconf) and rownum == 0 supposedly means that it's the base.tar. Looking again. I made a mistake in the previous check, rownum is not reliable. The tablespaces are sent first and base backup as the last. Now recovery.conf is written into base.tar. Maybe this is nitpicky problem but... If port number is not explicitly specified in pg_basebackup, the port number is not included to primary_conninfo in recovery.conf which is created during the backup. That is, the standby server using such recovery.conf tries to connect to the default port number because the port number is not supplied in primary_conninfo. This assumes that the default port number is the same between the master and standby. But this is not true. The default port number can be changed in --with-pgport configure option, so the default port number might be different between the master and standby. To avoid this uncertainty, pg_basebackup -R should always include the port number in primary_conninfo? I think you are right. But, I wouldn't restrict it only to the port setting. Any of the values that are set and equal to the compiled-in default, it should be written into recovery.conf. Now all values that are set (even those being equal to the compiled-in default) are put into recovery.conf. When the password is required to connect to the server, pg_basebackup -R always writes the password setting into primary_conninfo in recovery.conf. But if the password is supplied from .pgpass, ISTM that the password setting doesn't need to be written into primary_conninfo. Right? How can you deduce it from the PQconninfoOption structure? Also, if the machine you take the base backup on is different from the one where you actually use the backup on, it can be different not only in the --with-pgport compilation option but in the presence of .pgpass or the PGPASSWORD envvar, too. The administrator is there for a reason or there is no .pgpass or PGPASSWORD at all. +The password written into recovery.conf is not escaped even if special +characters appear in it. The administrator must review recovery.conf +to ensure proper escaping. Is it difficult to make pg_basebackup escape the special characters in the password? It's better if we can remove this restriction. It's not difficult. What other characters need to be escaped besides single quotes? All written values are escaped. Other changes: the recovery.conf in base.tar is correctly skipped if it exists and -R is given. The new recovery.conf is written with padding to round up to 512, the TAR chunk size. Also, the check for conflict between -R and -x/-X is now removed. The documentation for option -R has changed to reflect this and there is no different error code 2 now: it would make the behaviour inconsistent between -Fp and -Ft. The PQconninfo patch is also attached but didn't change since the last mail. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml --- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200 +++ postgresql.1/doc/src/sgml/libpq.sgml
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-12 22:21 keltezéssel, Robert Haas írta: On Wed, Oct 10, 2012 at 8:02 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Oct 11, 2012 at 3:36 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2012-10-10 18:23 keltezéssel, Fujii Masao írta: When tar output format is specified together with -R option, recovery.conf is not included in base.tar. I think it should. Why? This patch only promises to write the recovery.conf into the directory specified with -D. Because it's more user-friendly. If recovery.conf is not included in base.tar, when base.tar is extracted to disk to use the backup, a user always needs to copy recovery.conf to the extracted directory. OTOH if it's included in base.tar, such copy operation is not required and we can simplify the procedures to use the backup a bit. +1. OK, out of popular demand, I implemented writing into the base.tar if both -R and -Ft was specified. The code to create a tar header and the checksum inside the header was copied from bin/pg_dump/pg_backup_tar.c. I tested these scenarios (server can be either a master and standby): - backup a server with -Fp (no recovery.conf in the target directory was written) - backup a server with -Ftar (no recovery.conf was written into the target directory or base.tar) - backup a server with -Ftar -z (no recovery.conf was written into the target directory or base.tar.gz) - backup a server with -R -Fp (the new recovery.conf was written into the target directory) - backup a server with -R -Ftar (the new recovery.conf was written into the base.tar) - backup a server with -R -Ftar -z (the new recovery.conf was written into the base.tar.gz) Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml --- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200 +++ postgresql.1/doc/src/sgml/libpq.sgml 2012-10-14 11:12:07.049196259 +0200 @@ -496,6 +496,37 @@ typedef struct /listitem /varlistentry +varlistentry id=libpq-pqconninfo + termfunctionPQconninfo/functionindextermprimaryPQconninfo///term + listitem + para + Returns the connection options used by a live connection. +synopsis +PQconninfoOption *PQconninfo(PGconn *conn, bool for_replication); +/synopsis + /para + + para + Returns a connection options array. This can be used to determine + all possible functionPQconnectdb/function options and their + current values that were used to connect to the server. The return + value points to an array of structnamePQconninfoOption/structname + structures, which ends with an entry having a null structfieldkeyword/ + pointer. Every notes above for functionPQconndefaults/function also apply. + /para + + para + The literalfor_replication/ argument can be used to exclude some + options from the list which are used by the walreceiver module. + applicationpg_basebackup/application uses this pre-filtered list + to construct literalprimary_conninfo/ in the automatically generated + recovery.conf file. + /para + + /listitem +/varlistentry + + varlistentry id=libpq-pqconninfoparse termfunctionPQconninfoParse/functionindextermprimaryPQconninfoParse///term listitem diff -durpN postgresql/src/interfaces/libpq/exports.txt postgresql.1/src/interfaces/libpq/exports.txt --- postgresql/src/interfaces/libpq/exports.txt 2012-10-09 09:58:14.342782974 +0200 +++ postgresql.1/src/interfaces/libpq/exports.txt 2012-10-14 11:12:07.049196259 +0200 @@ -164,3 +164,4 @@ PQsetSingleRowMode161 lo_lseek64162 lo_tell64 163 lo_truncate64 164 +PQconninfo165 diff -durpN postgresql/src/interfaces/libpq/fe-connect.c postgresql.1/src/interfaces/libpq/fe-connect.c --- postgresql/src/interfaces/libpq/fe-connect.c 2012-09-09 08:11:09.470401480 +0200 +++ postgresql.1/src/interfaces/libpq/fe-connect.c 2012-10-14 11:12:07.053196284 +0200 @@ -137,6 +137,9 @@ static int ldapServiceLookup(const char * PQconninfoOptions[] *must* be NULL. In a working copy, non-null val * fields point to malloc'd strings that should be freed when the working * array is freed (see PQconninfoFree). + * + * If
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-11 02:02 keltezéssel, Fujii Masao írta: On Thu, Oct 11, 2012 at 3:36 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2012-10-10 18:23 keltezéssel, Fujii Masao írta: When tar output format is specified together with -R option, recovery.conf is not included in base.tar. I think it should. Why? This patch only promises to write the recovery.conf into the directory specified with -D. Because it's more user-friendly. If recovery.conf is not included in base.tar, when base.tar is extracted to disk to use the backup, a user always needs to copy recovery.conf to the extracted directory. OTOH if it's included in base.tar, such copy operation is not required and we can simplify the procedures to use the backup a bit. It's implemented now. +setting up the standby. Since creating a backup for a standalone +server with option-x/option or option-X/option and adding +a recovery.conf to it wouldn't make a working standby, these options +naturally conflict. Is this right? ISTM that basically we can use pg_basebackup -x to take a base backup for starting the standby for now. No? I don't know. Pointers? There is no restriction that the backup which was taken by using pg_basebackup -x cannot be used to start the standby. So I wonder why -R option cannot work together with -x. It's useful if recovery.conf is automatically written even with pg_basebackup -x. There was a problem with 9.0.x (maybe even with 9.1) that the backup failed to come up as a standby if -x or -X was specified. I don't know if it was a bug, limitation or intended behaviour. Before removing the check for conflicting options, I would like to ask: is there such a known conflict with --xlog-method=stream? And I found another problem: when -(stdout) is specified in -D option, recovery.conf fails to be written. $ pg_basebackup -D- -F t -c fast -R hoge.tar NOTICE: WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup pg_basebackup: cannot create -/recovery.conf: No such file or directory Now it works with recovery.conf written into the tar itself. I also tried $ pg_basebackup -D- -Ft -R and the directory named - was created and of course the recovery.conf inside it. Is this the intended behaviour regarding stdout is to be treated as a directory? Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
Thanks for updating the patch! On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan z...@cybertec.at wrote: Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. The tar file is always extracted such way in all platform which PostgreSQL supports? I'm just concerned about that some tool in some platform might prefer the original recovery.conf when extracting tar file. If the spec of tar format specifies such behavior (i.e., the last written file of the same name is always preferred), it's OK. I found the bug that recovery.conf is included in the tar file of the tablespace instead of base.tar, when there are tablespaces in the server. Maybe this is nitpicky problem but... If port number is not explicitly specified in pg_basebackup, the port number is not included to primary_conninfo in recovery.conf which is created during the backup. That is, the standby server using such recovery.conf tries to connect to the default port number because the port number is not supplied in primary_conninfo. This assumes that the default port number is the same between the master and standby. But this is not true. The default port number can be changed in --with-pgport configure option, so the default port number might be different between the master and standby. To avoid this uncertainty, pg_basebackup -R should always include the port number in primary_conninfo? When the password is required to connect to the server, pg_basebackup -R always writes the password setting into primary_conninfo in recovery.conf. But if the password is supplied from .pgpass, ISTM that the password setting doesn't need to be written into primary_conninfo. Right? +The password written into recovery.conf is not escaped even if special +characters appear in it. The administrator must review recovery.conf +to ensure proper escaping. Is it difficult to make pg_basebackup escape the special characters in the password? It's better if we can remove this restriction. I've not reviewed PQconninfo patch yet. Will review. 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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Mon, Oct 15, 2012 at 12:57 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2012-10-11 02:02 keltezéssel, Fujii Masao írta: On Thu, Oct 11, 2012 at 3:36 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2012-10-10 18:23 keltezéssel, Fujii Masao írta: When tar output format is specified together with -R option, recovery.conf is not included in base.tar. I think it should. Why? This patch only promises to write the recovery.conf into the directory specified with -D. Because it's more user-friendly. If recovery.conf is not included in base.tar, when base.tar is extracted to disk to use the backup, a user always needs to copy recovery.conf to the extracted directory. OTOH if it's included in base.tar, such copy operation is not required and we can simplify the procedures to use the backup a bit. It's implemented now. Thanks a lot! +setting up the standby. Since creating a backup for a standalone +server with option-x/option or option-X/option and adding +a recovery.conf to it wouldn't make a working standby, these options +naturally conflict. Is this right? ISTM that basically we can use pg_basebackup -x to take a base backup for starting the standby for now. No? I don't know. Pointers? There is no restriction that the backup which was taken by using pg_basebackup -x cannot be used to start the standby. So I wonder why -R option cannot work together with -x. It's useful if recovery.conf is automatically written even with pg_basebackup -x. There was a problem with 9.0.x (maybe even with 9.1) that the backup failed to come up as a standby if -x or -X was specified. I don't know if it was a bug, limitation or intended behaviour. It sounds a bug to me. It's helpful if you provide the self-contained test case. Before removing the check for conflicting options, I would like to ask: is there such a known conflict with --xlog-method=stream? AFAIK, No, we can use the backup which pg_basebackup --xlog-method=stream took, to start the standby. BTW, --xlog-method=stream cannot be specified together with -F tar. And I found another problem: when -(stdout) is specified in -D option, recovery.conf fails to be written. $ pg_basebackup -D- -F t -c fast -R hoge.tar NOTICE: WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup pg_basebackup: cannot create -/recovery.conf: No such file or directory Now it works with recovery.conf written into the tar itself. I also tried $ pg_basebackup -D- -Ft -R and the directory named - was created and of course the recovery.conf inside it. Is this the intended behaviour regarding stdout is to be treated as a directory? Yes. Thanks. 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-14 18:02 keltezéssel, Fujii Masao írta: Thanks for updating the patch! On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan z...@cybertec.at wrote: Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. The tar file is always extracted such way in all platform which PostgreSQL supports? I'm just concerned about that some tool in some platform might prefer the original recovery.conf when extracting tar file. If the spec of tar format specifies such behavior (i.e., the last written file of the same name is always preferred), it's OK. Since tar is a sequential archive format, I think this is the behaviour of every tar extractor. But I will look at adding code to skip the original recovery.conf if it exists in the tar file. I found the bug that recovery.conf is included in the tar file of the tablespace instead of base.tar, when there are tablespaces in the server. You are right, I am looking into this. But I don't know how it got there, I check for (rownum == 0 writerecoveryconf) and rownum == 0 supposedly means that it's the base.tar. Looking again. Maybe this is nitpicky problem but... If port number is not explicitly specified in pg_basebackup, the port number is not included to primary_conninfo in recovery.conf which is created during the backup. That is, the standby server using such recovery.conf tries to connect to the default port number because the port number is not supplied in primary_conninfo. This assumes that the default port number is the same between the master and standby. But this is not true. The default port number can be changed in --with-pgport configure option, so the default port number might be different between the master and standby. To avoid this uncertainty, pg_basebackup -R should always include the port number in primary_conninfo? I think you are right. But, I wouldn't restrict it only to the port setting. Any of the values that are set and equal to the compiled-in default, it should be written into recovery.conf. When the password is required to connect to the server, pg_basebackup -R always writes the password setting into primary_conninfo in recovery.conf. But if the password is supplied from .pgpass, ISTM that the password setting doesn't need to be written into primary_conninfo. Right? How can you deduce it from the PQconninfoOption structure? Also, if the machine you take the base backup on is different from the one where you actually use the backup on, it can be different not only in the --with-pgport compilation option but in the presence of .pgpass or the PGPASSWORD envvar, too. The administrator is there for a reason or there is no .pgpass or PGPASSWORD at all. +The password written into recovery.conf is not escaped even if special +characters appear in it. The administrator must review recovery.conf +to ensure proper escaping. Is it difficult to make pg_basebackup escape the special characters in the password? It's better if we can remove this restriction. It's not difficult. What other characters need to be escaped besides single quotes? I've not reviewed PQconninfo patch yet. Will review. Thanks in advance. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
Hi, 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 18:02 keltezéssel, Fujii Masao írta: Thanks for updating the patch! On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan z...@cybertec.at wrote: Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. The tar file is always extracted such way in all platform which PostgreSQL supports? I'm just concerned about that some tool in some platform might prefer the original recovery.conf when extracting tar file. If the spec of tar format specifies such behavior (i.e., the last written file of the same name is always preferred), it's OK. Since tar is a sequential archive format, I think this is the behaviour of every tar extractor. But I will look at adding code to skip the original recovery.conf if it exists in the tar file. I found the bug that recovery.conf is included in the tar file of the tablespace instead of base.tar, when there are tablespaces in the server. You are right, I am looking into this. But I don't know how it got there, I check for (rownum == 0 writerecoveryconf) and rownum == 0 supposedly means that it's the base.tar. Looking again. I made a mistake in the previous check, rownum is not reliable. The tablespaces are sent first and base backup as the last. Now recovery.conf is written into base.tar. Maybe this is nitpicky problem but... If port number is not explicitly specified in pg_basebackup, the port number is not included to primary_conninfo in recovery.conf which is created during the backup. That is, the standby server using such recovery.conf tries to connect to the default port number because the port number is not supplied in primary_conninfo. This assumes that the default port number is the same between the master and standby. But this is not true. The default port number can be changed in --with-pgport configure option, so the default port number might be different between the master and standby. To avoid this uncertainty, pg_basebackup -R should always include the port number in primary_conninfo? I think you are right. But, I wouldn't restrict it only to the port setting. Any of the values that are set and equal to the compiled-in default, it should be written into recovery.conf. Now all values that are set (even those being equal to the compiled-in default) are put into recovery.conf. When the password is required to connect to the server, pg_basebackup -R always writes the password setting into primary_conninfo in recovery.conf. But if the password is supplied from .pgpass, ISTM that the password setting doesn't need to be written into primary_conninfo. Right? How can you deduce it from the PQconninfoOption structure? Also, if the machine you take the base backup on is different from the one where you actually use the backup on, it can be different not only in the --with-pgport compilation option but in the presence of .pgpass or the PGPASSWORD envvar, too. The administrator is there for a reason or there is no .pgpass or PGPASSWORD at all. +The password written into recovery.conf is not escaped even if special +characters appear in it. The administrator must review recovery.conf +to ensure proper escaping. Is it difficult to make pg_basebackup escape the special characters in the password? It's better if we can remove this restriction. It's not difficult. What other characters need to be escaped besides single quotes? All written values are escaped. Other changes: the recovery.conf in base.tar is correctly skipped if it exists and -R is given. The new recovery.conf is written with padding to round up to 512, the TAR chunk size. The PQconninfo patch is also attached but didn't change since the last mail. I've not reviewed PQconninfo patch yet. Will review. Thanks in advance. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml --- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200 +++ postgresql.1/doc/src/sgml/libpq.sgml 2012-10-14 11:12:07.049196259 +0200 @@ -496,6 +496,37 @@ typedef struct /listitem /varlistentry +varlistentry id=libpq-pqconninfo + termfunctionPQconninfo/functionindextermprimaryPQconninfo///term + listitem + para + Returns the connection options used by a live connection. +synopsis
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta: Hi, 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 18:02 keltezéssel, Fujii Masao írta: Thanks for updating the patch! On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan z...@cybertec.at wrote: Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. The tar file is always extracted such way in all platform which PostgreSQL supports? I'm just concerned about that some tool in some platform might prefer the original recovery.conf when extracting tar file. If the spec of tar format specifies such behavior (i.e., the last written file of the same name is always preferred), it's OK. Since tar is a sequential archive format, I think this is the behaviour of every tar extractor. But I will look at adding code to skip the original recovery.conf if it exists in the tar file. I found the bug that recovery.conf is included in the tar file of the tablespace instead of base.tar, when there are tablespaces in the server. You are right, I am looking into this. But I don't know how it got there, I check for (rownum == 0 writerecoveryconf) and rownum == 0 supposedly means that it's the base.tar. Looking again. I made a mistake in the previous check, rownum is not reliable. The tablespaces are sent first and base backup as the last. Now recovery.conf is written into base.tar. Maybe this is nitpicky problem but... If port number is not explicitly specified in pg_basebackup, the port number is not included to primary_conninfo in recovery.conf which is created during the backup. That is, the standby server using such recovery.conf tries to connect to the default port number because the port number is not supplied in primary_conninfo. This assumes that the default port number is the same between the master and standby. But this is not true. The default port number can be changed in --with-pgport configure option, so the default port number might be different between the master and standby. To avoid this uncertainty, pg_basebackup -R should always include the port number in primary_conninfo? I think you are right. But, I wouldn't restrict it only to the port setting. Any of the values that are set and equal to the compiled-in default, it should be written into recovery.conf. Now all values that are set (even those being equal to the compiled-in default) are put into recovery.conf. When the password is required to connect to the server, pg_basebackup -R always writes the password setting into primary_conninfo in recovery.conf. But if the password is supplied from .pgpass, ISTM that the password setting doesn't need to be written into primary_conninfo. Right? How can you deduce it from the PQconninfoOption structure? Also, if the machine you take the base backup on is different from the one where you actually use the backup on, it can be different not only in the --with-pgport compilation option but in the presence of .pgpass or the PGPASSWORD envvar, too. The administrator is there for a reason or there is no .pgpass or PGPASSWORD at all. +The password written into recovery.conf is not escaped even if special +characters appear in it. The administrator must review recovery.conf +to ensure proper escaping. Is it difficult to make pg_basebackup escape the special characters in the password? It's better if we can remove this restriction. It's not difficult. What other characters need to be escaped besides single quotes? All written values are escaped. Other changes: the recovery.conf in base.tar is correctly skipped if it exists and -R is given. The new recovery.conf is written with padding to round up to 512, the TAR chunk size. Also, the check for conflict between -R and -x/-X is now removed. The PQconninfo patch is also attached but didn't change since the last mail. I've not reviewed PQconninfo patch yet. Will review. Thanks in advance. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-14 22:26 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 22:23 keltezéssel, Boszormenyi Zoltan írta: Hi, 2012-10-14 18:41 keltezéssel, Boszormenyi Zoltan írta: 2012-10-14 18:02 keltezéssel, Fujii Masao írta: Thanks for updating the patch! On Sun, Oct 14, 2012 at 8:41 PM, Boszormenyi Zoltan z...@cybertec.at wrote: Backing up a standby server without -R preserves the original recovery.conf of the standby, it points to the standby's source server. Backing up a standby server with -R overwrites the original recovery.conf with the new one pointing to the standby instead of the standby's source server. Without -Ft, it is obvious. With -Ft, there are two recovery.conf files in the tar file and upon extracting it, the last written one (the one generated via -R) overwrites the original. The tar file is always extracted such way in all platform which PostgreSQL supports? I'm just concerned about that some tool in some platform might prefer the original recovery.conf when extracting tar file. If the spec of tar format specifies such behavior (i.e., the last written file of the same name is always preferred), it's OK. Since tar is a sequential archive format, I think this is the behaviour of every tar extractor. But I will look at adding code to skip the original recovery.conf if it exists in the tar file. I found the bug that recovery.conf is included in the tar file of the tablespace instead of base.tar, when there are tablespaces in the server. You are right, I am looking into this. But I don't know how it got there, I check for (rownum == 0 writerecoveryconf) and rownum == 0 supposedly means that it's the base.tar. Looking again. I made a mistake in the previous check, rownum is not reliable. The tablespaces are sent first and base backup as the last. Now recovery.conf is written into base.tar. Maybe this is nitpicky problem but... If port number is not explicitly specified in pg_basebackup, the port number is not included to primary_conninfo in recovery.conf which is created during the backup. That is, the standby server using such recovery.conf tries to connect to the default port number because the port number is not supplied in primary_conninfo. This assumes that the default port number is the same between the master and standby. But this is not true. The default port number can be changed in --with-pgport configure option, so the default port number might be different between the master and standby. To avoid this uncertainty, pg_basebackup -R should always include the port number in primary_conninfo? I think you are right. But, I wouldn't restrict it only to the port setting. Any of the values that are set and equal to the compiled-in default, it should be written into recovery.conf. Now all values that are set (even those being equal to the compiled-in default) are put into recovery.conf. When the password is required to connect to the server, pg_basebackup -R always writes the password setting into primary_conninfo in recovery.conf. But if the password is supplied from .pgpass, ISTM that the password setting doesn't need to be written into primary_conninfo. Right? How can you deduce it from the PQconninfoOption structure? Also, if the machine you take the base backup on is different from the one where you actually use the backup on, it can be different not only in the --with-pgport compilation option but in the presence of .pgpass or the PGPASSWORD envvar, too. The administrator is there for a reason or there is no .pgpass or PGPASSWORD at all. +The password written into recovery.conf is not escaped even if special +characters appear in it. The administrator must review recovery.conf +to ensure proper escaping. Is it difficult to make pg_basebackup escape the special characters in the password? It's better if we can remove this restriction. It's not difficult. What other characters need to be escaped besides single quotes? All written values are escaped. Other changes: the recovery.conf in base.tar is correctly skipped if it exists and -R is given. The new recovery.conf is written with padding to round up to 512, the TAR chunk size. Also, the check for conflict between -R and -x/-X is now removed. Really the last one, for today at least. The buffer for recovery.conf is freed in both the -Fp and -Ft cases. The PQconninfo patch is also attached but didn't change since the last mail. I've not reviewed PQconninfo patch yet. Will review. Thanks in advance. Best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml --- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200 +++ postgresql.1/doc/src/sgml/libpq.sgml 2012-10-14
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Wed, Oct 10, 2012 at 8:02 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Oct 11, 2012 at 3:36 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2012-10-10 18:23 keltezéssel, Fujii Masao írta: When tar output format is specified together with -R option, recovery.conf is not included in base.tar. I think it should. Why? This patch only promises to write the recovery.conf into the directory specified with -D. Because it's more user-friendly. If recovery.conf is not included in base.tar, when base.tar is extracted to disk to use the backup, a user always needs to copy recovery.conf to the extracted directory. OTOH if it's included in base.tar, such copy operation is not required and we can simplify the procedures to use the backup a bit. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
2012-10-04 16:43 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and
PQconninfo() are symmetric and work for requiressl.
That's incredibly ugly. I'm not sure where we should keep the R
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable. Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need to
put that info somewhere else.
I hope only handling the new flag part is ugly. It can be hidden in the
PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
list as in the attached version.
Please find the review of the patch.
Basic stuff:
- patch apply failed at exports.txt file. Needs rebase to the current
master.
- Compiles cleanly with no warnings
What it does:
--
pg_basebackup does the base backup from the primary machine and also writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.
Testing:
-
1. Test pg_basebackup with option -R to check that the recovery.conf file is
written to data directory.
--recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
verify the recovery.conf which is created in data directory.
--All the primary connection info parameters are working fine.
4. Test pg_basebackup with conflict options (-x or -X and -R).
--Error is given when the conflict options are provided to
pg_basebackup.
5. Test pg_basebackup with option -R from a standby server.
--recovery.conf file is created in data directory.
Code Review:
-
1.
typedef struct PQconninfoMapping {
+char*keyword;
+size_tmember_offset;
+boolfor_replication;
+char*conninfoValue;/* Special value mapping
*/
+char*connValue;
+} PQconninfoMapping;
Provide the better explanation of conninfoValue and connValue, how and where
these are used?
2. if (tmp strncmp(tmp, mapping-conninfoValue, len) == 0)
In any case if the above condition is not satisfied then the PGconn data is
not filled with PQconninfoOption.
Is it correct?
Documentation:
-
1.
+para
+ The literalfor_replication/ argument can be used to exclude some
+ options from the list which are used by the walreceiver module.
+ applicationpg_basebackup/application uses this pre-filtered list
+ to construct literalprimary_conninfo/ in the automatically
generated
+ recovery.conf file.
+ /para
I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?
Observations/Issues:
---
1. If the password contains any escape sequence characters, It is leading to
problems while walreceiver connecting to primary
by using the primary conninfo from recovery.conf
please log an warning message or a note in document to handle such a case
manually by the user.
With Regards,
Amit Kapila.
--
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] [PATCH] Make pg_basebackup configure and start standby [Review]
Hi,
thanks for the new review.
2012-10-10 08:58 keltezéssel, Amit Kapila írta:
On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
2012-10-04 16:43 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and
PQconninfo() are symmetric and work for requiressl.
That's incredibly ugly. I'm not sure where we should keep the R
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable. Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need to
put that info somewhere else.
I hope only handling the new flag part is ugly. It can be hidden in the
PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
list as in the attached version.
Please find the review of the patch.
Basic stuff:
- patch apply failed at exports.txt file. Needs rebase to the current
master.
Done.
- Compiles cleanly with no warnings
What it does:
--
pg_basebackup does the base backup from the primary machine and also writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.
Testing:
-
1. Test pg_basebackup with option -R to check that the recovery.conf file is
written to data directory.
--recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
verify the recovery.conf which is created in data directory.
--All the primary connection info parameters are working fine.
4. Test pg_basebackup with conflict options (-x or -X and -R).
--Error is given when the conflict options are provided to
pg_basebackup.
5. Test pg_basebackup with option -R from a standby server.
--recovery.conf file is created in data directory.
Code Review:
-
1.
typedef struct PQconninfoMapping {
+char*keyword;
+size_tmember_offset;
+boolfor_replication;
+char*conninfoValue;/* Special value mapping
*/
+char*connValue;
+} PQconninfoMapping;
Provide the better explanation of conninfoValue and connValue, how and where
these are used?
OK. This is only used for requiressl='1' (in the connection string)
and if '1' is set, PGconn.sslmode will be set to require. All other
values are treated as it's being unset. This simplistic mapping
is used because there is only one such setting where different values
are used on the conninfo and the PGconn sides.
2. if (tmp strncmp(tmp, mapping-conninfoValue, len) == 0)
In any case if the above condition is not satisfied then the PGconn data is
not filled with PQconninfoOption.
Is it correct?
Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only happens
with the requiressl setting with its special mapping. If you set requiressl =
'0'
then it means that requiressl='1' was not set so the PGconn side stays as
NULL.
The special casing was there in the old code too and behaves the same.
Documentation:
-
1.
+para
+ The literalfor_replication/ argument can be used to exclude some
+ options from the list which are used by the walreceiver module.
+ applicationpg_basebackup/application uses this pre-filtered list
+ to construct literalprimary_conninfo/ in the automatically
generated
+ recovery.conf file.
+ /para
I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?
Err, no. The list excludes those[1] that *are* used (would be
overridden) by the walreceiver module:
88888
static bool
libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
{
...
snprintf(conninfo_repl, sizeof(conninfo_repl),
%s dbname=replication replication=true
fallback_application_name=walreceiver,
conninfo);
88888
[1] Actually, more than these 3 options are excluded. The deprecated
ones are also excluded.
Observations/Issues:
---
1. If the password contains any escape sequence characters, It is leading to
problems while walreceiver connecting to primary
by using the primary conninfo from recovery.conf
please log an warning message or a note in document to handle such a case
manually by the user.
Done at both places.
Also, to adapt to the style of other error messages, now all my fprintf()
statements
are prefixed with: %s: ..., progname.
Best regards,
Zoltán Böszörményi
--
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Wed, Oct 10, 2012 at 10:12 PM, Boszormenyi Zoltan z...@cybertec.at wrote:
Hi,
thanks for the new review.
2012-10-10 08:58 keltezéssel, Amit Kapila írta:
On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
2012-10-04 16:43 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and
PQconninfo() are symmetric and work for requiressl.
That's incredibly ugly. I'm not sure where we should keep the R
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable. Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need to
put that info somewhere else.
I hope only handling the new flag part is ugly. It can be hidden in the
PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
list as in the attached version.
Please find the review of the patch.
Basic stuff:
- patch apply failed at exports.txt file. Needs rebase to the current
master.
Done.
- Compiles cleanly with no warnings
What it does:
--
pg_basebackup does the base backup from the primary machine and also
writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.
Testing:
-
1. Test pg_basebackup with option -R to check that the recovery.conf file
is
written to data directory.
--recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf
file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
verify the recovery.conf which is created in data directory.
--All the primary connection info parameters are working fine.
4. Test pg_basebackup with conflict options (-x or -X and -R).
--Error is given when the conflict options are provided to
pg_basebackup.
5. Test pg_basebackup with option -R from a standby server.
--recovery.conf file is created in data directory.
Code Review:
-
1.
typedef struct PQconninfoMapping {
+char*keyword;
+size_tmember_offset;
+boolfor_replication;
+char*conninfoValue;/* Special value
mapping
*/
+char*connValue;
+} PQconninfoMapping;
Provide the better explanation of conninfoValue and connValue, how and
where
these are used?
OK. This is only used for requiressl='1' (in the connection string)
and if '1' is set, PGconn.sslmode will be set to require. All other
values are treated as it's being unset. This simplistic mapping
is used because there is only one such setting where different values
are used on the conninfo and the PGconn sides.
2. if (tmp strncmp(tmp, mapping-conninfoValue, len) == 0)
In any case if the above condition is not satisfied then the PGconn data
is
not filled with PQconninfoOption.
Is it correct?
Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only
happens
with the requiressl setting with its special mapping. If you set
requiressl = '0'
then it means that requiressl='1' was not set so the PGconn side stays
as NULL.
The special casing was there in the old code too and behaves the same.
Documentation:
-
1.
+para
+ The literalfor_replication/ argument can be used to exclude
some
+ options from the list which are used by the walreceiver module.
+ applicationpg_basebackup/application uses this pre-filtered
list
+ to construct literalprimary_conninfo/ in the automatically
generated
+ recovery.conf file.
+ /para
I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?
Err, no. The list excludes those[1] that *are* used (would be
overridden) by the walreceiver module:
88888
static bool
libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
{
...
snprintf(conninfo_repl, sizeof(conninfo_repl),
%s dbname=replication replication=true
fallback_application_name=walreceiver,
conninfo);
88888
[1] Actually, more than these 3 options are excluded. The deprecated
ones are also excluded.
Observations/Issues:
---
1. If the password contains any escape sequence characters, It is leading
to
problems while walreceiver connecting to primary
by using the primary conninfo from recovery.conf
please log an warning message or a note in document to handle such a
case
manually by the user.
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-10 18:23 keltezéssel, Fujii Masao írta:
On Wed, Oct 10, 2012 at 10:12 PM, Boszormenyi Zoltan z...@cybertec.at wrote:
Hi,
thanks for the new review.
2012-10-10 08:58 keltezéssel, Amit Kapila írta:
On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
2012-10-04 16:43 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and
PQconninfo() are symmetric and work for requiressl.
That's incredibly ugly. I'm not sure where we should keep the R
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable. Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need to
put that info somewhere else.
I hope only handling the new flag part is ugly. It can be hidden in the
PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
list as in the attached version.
Please find the review of the patch.
Basic stuff:
- patch apply failed at exports.txt file. Needs rebase to the current
master.
Done.
- Compiles cleanly with no warnings
What it does:
--
pg_basebackup does the base backup from the primary machine and also
writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.
Testing:
-
1. Test pg_basebackup with option -R to check that the recovery.conf file
is
written to data directory.
--recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf
file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
verify the recovery.conf which is created in data directory.
--All the primary connection info parameters are working fine.
4. Test pg_basebackup with conflict options (-x or -X and -R).
--Error is given when the conflict options are provided to
pg_basebackup.
5. Test pg_basebackup with option -R from a standby server.
--recovery.conf file is created in data directory.
Code Review:
-
1.
typedef struct PQconninfoMapping {
+char*keyword;
+size_tmember_offset;
+boolfor_replication;
+char*conninfoValue;/* Special value
mapping
*/
+char*connValue;
+} PQconninfoMapping;
Provide the better explanation of conninfoValue and connValue, how and
where
these are used?
OK. This is only used for requiressl='1' (in the connection string)
and if '1' is set, PGconn.sslmode will be set to require. All other
values are treated as it's being unset. This simplistic mapping
is used because there is only one such setting where different values
are used on the conninfo and the PGconn sides.
2. if (tmp strncmp(tmp, mapping-conninfoValue, len) == 0)
In any case if the above condition is not satisfied then the PGconn data
is
not filled with PQconninfoOption.
Is it correct?
Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only
happens
with the requiressl setting with its special mapping. If you set
requiressl = '0'
then it means that requiressl='1' was not set so the PGconn side stays
as NULL.
The special casing was there in the old code too and behaves the same.
Documentation:
-
1.
+para
+ The literalfor_replication/ argument can be used to exclude
some
+ options from the list which are used by the walreceiver module.
+ applicationpg_basebackup/application uses this pre-filtered
list
+ to construct literalprimary_conninfo/ in the automatically
generated
+ recovery.conf file.
+ /para
I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?
Err, no. The list excludes those[1] that *are* used (would be
overridden) by the walreceiver module:
88888
static bool
libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
{
...
snprintf(conninfo_repl, sizeof(conninfo_repl),
%s dbname=replication replication=true
fallback_application_name=walreceiver,
conninfo);
88888
[1] Actually, more than these 3 options are excluded. The deprecated
ones are also excluded.
Observations/Issues:
---
1. If the password contains any escape sequence characters, It is leading
to
problems while walreceiver connecting to primary
by using the primary conninfo from recovery.conf
please log an warning message or a note in document to handle such a
case
manually by the user.
Done at both places.
Also, to adapt to the
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-10 20:36 keltezéssel, Boszormenyi Zoltan írta:
2012-10-10 18:23 keltezéssel, Fujii Masao írta:
On Wed, Oct 10, 2012 at 10:12 PM, Boszormenyi Zoltan z...@cybertec.at wrote:
Hi,
thanks for the new review.
2012-10-10 08:58 keltezéssel, Amit Kapila írta:
On Thursday, October 04, 2012 9:50 PM Boszormenyi Zoltan
2012-10-04 16:43 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and
PQconninfo() are symmetric and work for requiressl.
That's incredibly ugly. I'm not sure where we should keep the R
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable. Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need to
put that info somewhere else.
I hope only handling the new flag part is ugly. It can be hidden in the
PQconninfoMapping[] array and PQconninfo(conn, true) pre-filters the
list as in the attached version.
Please find the review of the patch.
Basic stuff:
- patch apply failed at exports.txt file. Needs rebase to the current
master.
Done.
- Compiles cleanly with no warnings
What it does:
--
pg_basebackup does the base backup from the primary machine and also
writes
the recovery.conf file with the option -R,
which is used for the standby to connect to primary for streaming
replication.
Testing:
-
1. Test pg_basebackup with option -R to check that the recovery.conf file
is
written to data directory.
--recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf
file is
not able to create because of disk full.
--Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -R including -h, -U, -p, -w and -W.
verify the recovery.conf which is created in data directory.
--All the primary connection info parameters are working fine.
4. Test pg_basebackup with conflict options (-x or -X and -R).
--Error is given when the conflict options are provided to
pg_basebackup.
5. Test pg_basebackup with option -R from a standby server.
--recovery.conf file is created in data directory.
Code Review:
-
1.
typedef struct PQconninfoMapping {
+char*keyword;
+size_tmember_offset;
+boolfor_replication;
+char*conninfoValue;/* Special value
mapping
*/
+char*connValue;
+} PQconninfoMapping;
Provide the better explanation of conninfoValue and connValue, how and
where
these are used?
OK. This is only used for requiressl='1' (in the connection string)
and if '1' is set, PGconn.sslmode will be set to require. All other
values are treated as it's being unset. This simplistic mapping
is used because there is only one such setting where different values
are used on the conninfo and the PGconn sides.
2. if (tmp strncmp(tmp, mapping-conninfoValue, len) == 0)
In any case if the above condition is not satisfied then the PGconn data
is
not filled with PQconninfoOption.
Is it correct?
Yes, it stays NULL as makeEmptyPGconn() initialized it. This case only
happens
with the requiressl setting with its special mapping. If you set
requiressl = '0'
then it means that requiressl='1' was not set so the PGconn side stays
as NULL.
The special casing was there in the old code too and behaves the same.
Documentation:
-
1.
+para
+ The literalfor_replication/ argument can be used to exclude
some
+ options from the list which are used by the walreceiver module.
+ applicationpg_basebackup/application uses this pre-filtered
list
+ to construct literalprimary_conninfo/ in the automatically
generated
+ recovery.conf file.
+ /para
I feel the explanation should be as follows,
exclude some options from the list which are not used by the walreceiver
module?
Err, no. The list excludes those[1] that *are* used (would be
overridden) by the walreceiver module:
88888
static bool
libpqrcv_connect(char *conninfo, XLogRecPtr startpoint)
{
...
snprintf(conninfo_repl, sizeof(conninfo_repl),
%s dbname=replication replication=true
fallback_application_name=walreceiver,
conninfo);
88888
[1] Actually, more than these 3 options are excluded. The deprecated
ones are also excluded.
Observations/Issues:
---
1. If the password contains any escape sequence characters, It is leading
to
problems while walreceiver connecting to primary
by using the primary conninfo from recovery.conf
please log an warning message or a note in document to handle such a
case
manually by the
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Thu, Oct 11, 2012 at 3:36 AM, Boszormenyi Zoltan z...@cybertec.at wrote: 2012-10-10 18:23 keltezéssel, Fujii Masao írta: When tar output format is specified together with -R option, recovery.conf is not included in base.tar. I think it should. Why? This patch only promises to write the recovery.conf into the directory specified with -D. Because it's more user-friendly. If recovery.conf is not included in base.tar, when base.tar is extracted to disk to use the backup, a user always needs to copy recovery.conf to the extracted directory. OTOH if it's included in base.tar, such copy operation is not required and we can simplify the procedures to use the backup a bit. +setting up the standby. Since creating a backup for a standalone +server with option-x/option or option-X/option and adding +a recovery.conf to it wouldn't make a working standby, these options +naturally conflict. Is this right? ISTM that basically we can use pg_basebackup -x to take a base backup for starting the standby for now. No? I don't know. Pointers? There is no restriction that the backup which was taken by using pg_basebackup -x cannot be used to start the standby. So I wonder why -R option cannot work together with -x. It's useful if recovery.conf is automatically written even with pg_basebackup -x. And I found another problem: when -(stdout) is specified in -D option, recovery.conf fails to be written. $ pg_basebackup -D- -F t -c fast -R hoge.tar NOTICE: WAL archiving is not enabled; you must ensure that all required WAL segments are copied through other means to complete the backup pg_basebackup: cannot create -/recovery.conf: No such file or directory 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] [PATCH] Make pg_basebackup configure and start standby [Review]
On Thu, Oct 11, 2012 at 4:58 AM, Boszormenyi Zoltan z...@cybertec.at wrote: I was able to test it on OSX and I found my bug. Attached is the new code. The problem was in conninfo_init(), the last entry in the filtered list was not initialized to 0. It seems that for some reason, my Linux machine gave me pre-initialized memory. Thanks. Will test. 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-04 06:47 keltezéssel, Boszormenyi Zoltan írta:
2012-10-04 05:24 keltezéssel, Peter Eisentraut írta:
On Wed, 2012-10-03 at 18:15 +0200, Boszormenyi Zoltan wrote:
The second generation of this work is now attached and contains a new
feature as was discussed and suggested by Magnus Hagander, Fujii Masao
and Peter Eisentraut. So libpq has grown a new function:
+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn);
This copies all the connection parameters back from the live PGconn
itself
so everything that's needed to connect is already validated.
I don't like that this code maintains a second list of all possible
libpq connection parameters.
Where does it do that? In PQconninfo() itself? Why is it a problem?
Or to put it bluntly: the same problem is in fillPGconn() too, as it also
has the same set of parameters listed. So there is already code
that you don't like. :-)
How about a static mapping between option names and
offsetof(struct pg_conn, member) values? This way both fillPGconn()
and PQconninfo() can avoid maintaining the list of parameter names.
Did you think about something like the attached code?
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff -durpN postgresql/src/interfaces/libpq/exports.txt postgresql.1/src/interfaces/libpq/exports.txt
--- postgresql/src/interfaces/libpq/exports.txt 2012-08-03 09:39:30.118266598 +0200
+++ postgresql.1/src/interfaces/libpq/exports.txt 2012-10-04 11:29:24.864309200 +0200
@@ -161,3 +161,5 @@ PQping158
PQpingParams 159
PQlibVersion 160
PQsetSingleRowMode161
+PQconninfo162
+PQconninfoForReplication 163
diff -durpN postgresql/src/interfaces/libpq/fe-connect.c postgresql.1/src/interfaces/libpq/fe-connect.c
--- postgresql/src/interfaces/libpq/fe-connect.c 2012-09-09 08:11:09.470401480 +0200
+++ postgresql.1/src/interfaces/libpq/fe-connect.c 2012-10-04 12:30:56.705605588 +0200
@@ -131,12 +131,17 @@ static int ldapServiceLookup(const char
*Normal input field
* * Password field - hide value
* D Debug option - don't show by default
+ * A special usable for replication connections (R) flag is
+ * added to Disp-Char in a backwards compatible fashion.
*
* PQconninfoOptions[] is a constant static array that we use to initialize
* a dynamically allocated working copy. All the val fields in
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null val
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * If you add a new connection option to this list, remember to add it to
+ * PQconninfoMappings[] below.
* --
*/
static const PQconninfoOption PQconninfoOptions[] = {
@@ -146,62 +151,62 @@ static const PQconninfoOption PQconninfo
* still try to set it.
*/
{authtype, PGAUTHTYPE, DefaultAuthtype, NULL,
- Database-Authtype, D, 20},
+ Database-Authtype, D\0, 20},
{service, PGSERVICE, NULL, NULL,
- Database-Service, , 20},
+ Database-Service, \0, 20},
{user, PGUSER, NULL, NULL,
- Database-User, , 20},
+ Database-User, \0R, 20},
{password, PGPASSWORD, NULL, NULL,
- Database-Password, *, 20},
+ Database-Password, *\0R, 20},
{connect_timeout, PGCONNECT_TIMEOUT, NULL, NULL,
- Connect-timeout, , 10}, /* strlen(INT32_MAX) == 10 */
+ Connect-timeout, \0R, 10}, /* strlen(INT32_MAX) == 10 */
{dbname, PGDATABASE, NULL, NULL,
- Database-Name, , 20},
+ Database-Name, \0, 20},
{host, PGHOST, NULL, NULL,
- Database-Host, , 40},
+ Database-Host, \0R, 40},
{hostaddr, PGHOSTADDR, NULL, NULL,
- Database-Host-IP-Address, , 45},
+ Database-Host-IP-Address, \0R, 45},
{port, PGPORT, DEF_PGPORT_STR, NULL,
- Database-Port, , 6},
+ Database-Port, \0R, 6},
{client_encoding, PGCLIENTENCODING, NULL, NULL,
- Client-Encoding, , 10},
+ Client-Encoding, \0, 10},
/*
* tty is no longer used either, but keep it present for backwards
* compatibility.
*/
{tty, PGTTY, DefaultTty, NULL,
- Backend-Debug-TTY, D, 40},
+ Backend-Debug-TTY, D\0, 40},
{options, PGOPTIONS, DefaultOption, NULL,
- Backend-Debug-Options, D, 40},
+ Backend-Debug-Options, D\0R, 40},
{application_name, PGAPPNAME, NULL, NULL,
- Application-Name, , 64},
+ Application-Name, \0, 64},
{fallback_application_name, NULL, NULL, NULL,
- Fallback-Application-Name, , 64},
+ Fallback-Application-Name, \0, 64},
{keepalives, NULL, NULL, NULL,
- TCP-Keepalives, , 1}, /* should be just '0' or '1' */
+ TCP-Keepalives, \0R, 1}, /* should be just '0' or '1' */
{keepalives_idle, NULL, NULL, NULL,
- TCP-Keepalives-Idle, , 10}, /* strlen(INT32_MAX) == 10 */
+ TCP-Keepalives-Idle, \0R, 10}, /* strlen(INT32_MAX) == 10 */
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-04 12:42 keltezéssel, Boszormenyi Zoltan írta:
2012-10-04 06:47 keltezéssel, Boszormenyi Zoltan írta:
2012-10-04 05:24 keltezéssel, Peter Eisentraut írta:
On Wed, 2012-10-03 at 18:15 +0200, Boszormenyi Zoltan wrote:
The second generation of this work is now attached and contains a new
feature as was discussed and suggested by Magnus Hagander, Fujii Masao
and Peter Eisentraut. So libpq has grown a new function:
+/* return the connection options used by a live connection */
+extern PQconninfoOption *PQconninfo(PGconn *conn);
This copies all the connection parameters back from the live PGconn
itself
so everything that's needed to connect is already validated.
I don't like that this code maintains a second list of all possible
libpq connection parameters.
Where does it do that? In PQconninfo() itself? Why is it a problem?
Or to put it bluntly: the same problem is in fillPGconn() too, as it also
has the same set of parameters listed. So there is already code
that you don't like. :-)
How about a static mapping between option names and
offsetof(struct pg_conn, member) values? This way both fillPGconn()
and PQconninfo() can avoid maintaining the list of parameter names.
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and PQconninfo() are
symmetric and work for requiressl.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff -durpN postgresql/src/interfaces/libpq/exports.txt postgresql.1/src/interfaces/libpq/exports.txt
--- postgresql/src/interfaces/libpq/exports.txt 2012-08-03 09:39:30.118266598 +0200
+++ postgresql.1/src/interfaces/libpq/exports.txt 2012-10-04 11:29:24.864309200 +0200
@@ -161,3 +161,5 @@ PQping158
PQpingParams 159
PQlibVersion 160
PQsetSingleRowMode161
+PQconninfo162
+PQconninfoForReplication 163
diff -durpN postgresql/src/interfaces/libpq/fe-connect.c postgresql.1/src/interfaces/libpq/fe-connect.c
--- postgresql/src/interfaces/libpq/fe-connect.c 2012-09-09 08:11:09.470401480 +0200
+++ postgresql.1/src/interfaces/libpq/fe-connect.c 2012-10-04 13:10:58.737418824 +0200
@@ -131,12 +131,17 @@ static int ldapServiceLookup(const char
*Normal input field
* * Password field - hide value
* D Debug option - don't show by default
+ * A special usable for replication connections (R) flag is
+ * added to Disp-Char in a backwards compatible fashion.
*
* PQconninfoOptions[] is a constant static array that we use to initialize
* a dynamically allocated working copy. All the val fields in
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null val
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * If you add a new connection option to this list, remember to add it to
+ * PQconninfoMappings[] below.
* --
*/
static const PQconninfoOption PQconninfoOptions[] = {
@@ -146,62 +151,62 @@ static const PQconninfoOption PQconninfo
* still try to set it.
*/
{authtype, PGAUTHTYPE, DefaultAuthtype, NULL,
- Database-Authtype, D, 20},
+ Database-Authtype, D\0, 20},
{service, PGSERVICE, NULL, NULL,
- Database-Service, , 20},
+ Database-Service, \0, 20},
{user, PGUSER, NULL, NULL,
- Database-User, , 20},
+ Database-User, \0R, 20},
{password, PGPASSWORD, NULL, NULL,
- Database-Password, *, 20},
+ Database-Password, *\0R, 20},
{connect_timeout, PGCONNECT_TIMEOUT, NULL, NULL,
- Connect-timeout, , 10}, /* strlen(INT32_MAX) == 10 */
+ Connect-timeout, \0R, 10}, /* strlen(INT32_MAX) == 10 */
{dbname, PGDATABASE, NULL, NULL,
- Database-Name, , 20},
+ Database-Name, \0, 20},
{host, PGHOST, NULL, NULL,
- Database-Host, , 40},
+ Database-Host, \0R, 40},
{hostaddr, PGHOSTADDR, NULL, NULL,
- Database-Host-IP-Address, , 45},
+ Database-Host-IP-Address, \0R, 45},
{port, PGPORT, DEF_PGPORT_STR, NULL,
- Database-Port, , 6},
+ Database-Port, \0R, 6},
{client_encoding, PGCLIENTENCODING, NULL, NULL,
- Client-Encoding, , 10},
+ Client-Encoding, \0, 10},
/*
* tty is no longer used either, but keep it present for backwards
* compatibility.
*/
{tty, PGTTY, DefaultTty, NULL,
- Backend-Debug-TTY, D, 40},
+ Backend-Debug-TTY, D\0, 40},
{options, PGOPTIONS, DefaultOption, NULL,
- Backend-Debug-Options, D, 40},
+ Backend-Debug-Options, D\0R, 40},
{application_name, PGAPPNAME, NULL, NULL,
- Application-Name, , 64},
+ Application-Name, \0, 64},
{fallback_application_name, NULL, NULL, NULL,
- Fallback-Application-Name, , 64},
+ Fallback-Application-Name, \0, 64},
{keepalives, NULL, NULL, NULL,
- TCP-Keepalives, , 1}, /* should be just '0' or '1' */
+ TCP-Keepalives, \0R, 1}, /* should be just '0' or '1' */
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
Boszormenyi Zoltan z...@cybertec.at writes: Did you think about something like the attached code? Or rather this one, which fixes a bug so fillPGconn() and PQconninfo() are symmetric and work for requiressl. That's incredibly ugly. I'm not sure where we should keep the R information, but shoehorning it into the existing PQconninfoOption struct like that seems totally unacceptable. Either we're willing to break backwards compatibility on the Disp-Char strings, or we need to put that info somewhere else. 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-04 16:43 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan z...@cybertec.at writes:
Did you think about something like the attached code?
Or rather this one, which fixes a bug so fillPGconn() and PQconninfo() are
symmetric and work for requiressl.
That's incredibly ugly. I'm not sure where we should keep the R
information, but shoehorning it into the existing PQconninfoOption
struct like that seems totally unacceptable. Either we're willing to
break backwards compatibility on the Disp-Char strings, or we need
to put that info somewhere else.
I hope only handling the new flag part is ugly. It can be hidden
in the PQconninfoMapping[] array and PQconninfo(conn, true)
pre-filters the list as in the attached version.
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff -durpN postgresql/doc/src/sgml/libpq.sgml postgresql.1/doc/src/sgml/libpq.sgml
--- postgresql/doc/src/sgml/libpq.sgml 2012-08-03 09:39:30.114266570 +0200
+++ postgresql.1/doc/src/sgml/libpq.sgml 2012-10-04 18:15:11.686199926 +0200
@@ -496,6 +496,37 @@ typedef struct
/listitem
/varlistentry
+varlistentry id=libpq-pqconninfo
+ termfunctionPQconninfo/functionindextermprimaryPQconninfo///term
+ listitem
+ para
+ Returns the connection options used by a live connection.
+synopsis
+PQconninfoOption *PQconninfo(PGconn *conn, bool for_replication);
+/synopsis
+ /para
+
+ para
+ Returns a connection options array. This can be used to determine
+ all possible functionPQconnectdb/function options and their
+ current values that were used to connect to the server. The return
+ value points to an array of structnamePQconninfoOption/structname
+ structures, which ends with an entry having a null structfieldkeyword/
+ pointer. Every notes above for functionPQconndefaults/function also apply.
+ /para
+
+ para
+ The literalfor_replication/ argument can be used to exclude some
+ options from the list which are used by the walreceiver module.
+ applicationpg_basebackup/application uses this pre-filtered list
+ to construct literalprimary_conninfo/ in the automatically generated
+ recovery.conf file.
+ /para
+
+ /listitem
+/varlistentry
+
+
varlistentry id=libpq-pqconninfoparse
termfunctionPQconninfoParse/functionindextermprimaryPQconninfoParse///term
listitem
diff -durpN postgresql/src/interfaces/libpq/exports.txt postgresql.1/src/interfaces/libpq/exports.txt
--- postgresql/src/interfaces/libpq/exports.txt 2012-08-03 09:39:30.118266598 +0200
+++ postgresql.1/src/interfaces/libpq/exports.txt 2012-10-04 17:10:11.483654334 +0200
@@ -161,3 +161,4 @@ PQping158
PQpingParams 159
PQlibVersion 160
PQsetSingleRowMode161
+PQconninfo162
diff -durpN postgresql/src/interfaces/libpq/fe-connect.c postgresql.1/src/interfaces/libpq/fe-connect.c
--- postgresql/src/interfaces/libpq/fe-connect.c 2012-09-09 08:11:09.470401480 +0200
+++ postgresql.1/src/interfaces/libpq/fe-connect.c 2012-10-04 17:53:26.205928500 +0200
@@ -137,6 +137,9 @@ static int ldapServiceLookup(const char
* PQconninfoOptions[] *must* be NULL. In a working copy, non-null val
* fields point to malloc'd strings that should be freed when the working
* array is freed (see PQconninfoFree).
+ *
+ * If you add a new connection option to this list, remember to add it to
+ * PQconninfoMappings[] below.
* --
*/
static const PQconninfoOption PQconninfoOptions[] = {
@@ -264,6 +267,62 @@ static const PQconninfoOption PQconninfo
NULL, NULL, 0}
};
+/*
+ * We need a mapping between the PQconninfoOptions[] array
+ * and PGconn members. We have to keep it separate from
+ * PQconninfoOptions[] to not leak info about PGconn members
+ * to clients.
+ */
+typedef struct PQconninfoMapping {
+ char *keyword;
+ size_t member_offset;
+ bool for_replication;
+ char *conninfoValue; /* Special value mapping */
+ char *connValue;
+} PQconninfoMapping;
+#define PGCONNMEMBERADDR(conn, mapping) ((char **)((char *)conn + mapping-member_offset))
+
+static const PQconninfoMapping PQconninfoMappings[] =
+{
+ /* authtype is not used anymore, there is no mapping to PGconn */
+ /* there is no mapping of service to PGconn */
+ { user, offsetof(struct pg_conn, pguser), true, NULL, NULL },
+ { password, offsetof(struct pg_conn, pgpass), true, NULL, NULL },
+ { connect_timeout, offsetof(struct pg_conn, connect_timeout), true, NULL, NULL },
+ { dbname, offsetof(struct pg_conn, dbName), false, NULL, NULL },
+ { host, offsetof(struct pg_conn, pghost), true, NULL, NULL },
+ { hostaddr, offsetof(struct pg_conn, pghostaddr), true, NULL, NULL },
+ { port, offsetof(struct pg_conn, pgport), true, NULL, NULL },
+
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
Hi,
first, thanks for the review. Comments are below.
2012-09-20 12:30 keltezéssel, Amit Kapila írta:
On Sun, 01 Jul 2012 13:02:17 +0200 Boszormenyi Zoltan wrote:
attached is a patch that does $SUBJECT.
It's a usability enhancement, to take a backup, write
a minimalistic recovery.conf and start the streaming
standby in one go.
Comments?
*[Review of Patch]*
*Basic stuff:*
--
- Patch applies OK
This is not true anymore with a newer GIT version.
Some chunk for pg_basebackup.c was rejected.
- Compiles cleanly with no warnings
*What it does:*
-
The pg_basebackup tool does the backup of Cluster from server to the specified
location.
This new functionality will also writes the recovery.conf in the database directory and
start the standby server based on options passed to pg_basebackup.
*Usability*
**
For usability aspect, I am not very sure how many users would like to start the standby
server using basebackup.
Also, Magnus raised the point that it wouldn't really work on MS Windows
where you *have to* start the service via OS facilities. This part of the patch
was killed.
According to me it can be useful for users who have automated scripts to start server
after backup can use this feature.
Well, scripting is not portable across UNIXes and Windows,
you have to spell out starting the server differently.
*Feature Testing:*
-
1. Test pg_basebackup with option -R to check that the recovery.conf file is written to
data directory.
--recovery.conf file is created in data directory.
2. Test pg_basebackup with option -R to check that the recovery.conf file is not able to
create because of disk full.
--Error is given as recovery.conf file is not able to create.
3. Test pg_basebackup with option -S to check the standby server start on the
same/different machine.
--Starting standby server is success in if pg_basebackup is taken in
different machine.
4. Test pg_basebackup with both options -S and -R to check the standby server start on
same/different machine.
--Starting standby server is success in if pg_basebackup is taken in
different machine.
5. Test pg_basebackup with option -S including -h, -U, -p, -w and -W to check the standy
server start
and verify the recovery.conf which is created in data directory.
--Except password, rest of the primary connection info parameters are working
fine.
The password part is now fixed.
6. Test pg_basebackup with conflict options (-x or -X and -R or -S).
--Error is given when the conflict options are provided to pg_basebackup.
7. Test pg_basebackup with option -S where pg_ctl/postmaster binaries are not present in
the path.
--Error is given as not able to execute.
8. Test pg_basebackup with option -S by connecting to a standby server.
--standby server is started successfully when pg_basebackup is made from a standby
server also.
*Code Review:*
1. In function WriteRecoveryConf(), un-initialized filename is used.
due to which it can print junk for below line in code
printf(add password to primary_conninfo in %s if needed\n, filename);
Fixed.
2. In function WriteRecoveryConf(), in below code if fopen fails (due to disk full or
any other file related error) it will print the error and exits.
So now it can be confusing to user, in respect to can he consider backup as
successfull and proceed. IMO, either error meesage or documentation
can suggest the for such error user can proceed with backup to write his own
recovery.conf and start the standby.
+cf = fopen(filename, w);
+if (cf == NULL)
+{
+fprintf(stderr, _(cannot create %s), filename);
+exit(1);
+}
But BaseBackup() function did indicate already that it has
finished successfully with
if (verbose)
fprintf(stderr, %s: base backup completed\n, progname);
Would it be an expected (as in: not confusing) behaviour to return 0
from pg_basebackup if the backup itself has finished, but failed to write
the recovery.conf or start the standby if those were requested?
I have modified my WriteRecoveryConf() to do exit(2) instead of exit(1)
to indicate a different error. exit(1) seems to be for reporting configuration
or connection errors. (I may be mistaken though.)
3. In function main,
instead of the following code it can be changed in two different ways,
if (startstandby)
writerecoveryconf = true;
change1:
case 'R':
writerecoveryconf = true;
break;
case 'S':
startstandby = true;
writerecoveryconf = true;
break;
change2:
case 'S':
startstandby = true;
case 'R':
writerecoveryconf = true;
break;
Re: [HACKERS] [PATCH] Make pg_basebackup configure and start standby [Review]
On Wed, 2012-10-03 at 18:15 +0200, Boszormenyi Zoltan wrote: The second generation of this work is now attached and contains a new feature as was discussed and suggested by Magnus Hagander, Fujii Masao and Peter Eisentraut. So libpq has grown a new function: +/* return the connection options used by a live connection */ +extern PQconninfoOption *PQconninfo(PGconn *conn); This copies all the connection parameters back from the live PGconn itself so everything that's needed to connect is already validated. I don't like that this code maintains a second list of all possible libpq connection parameters. The parameters to add to the connection string should be driven off the authoritative list in PQconninfoOptions. -- 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] [PATCH] Make pg_basebackup configure and start standby [Review]
2012-10-04 05:24 keltezéssel, Peter Eisentraut írta: On Wed, 2012-10-03 at 18:15 +0200, Boszormenyi Zoltan wrote: The second generation of this work is now attached and contains a new feature as was discussed and suggested by Magnus Hagander, Fujii Masao and Peter Eisentraut. So libpq has grown a new function: +/* return the connection options used by a live connection */ +extern PQconninfoOption *PQconninfo(PGconn *conn); This copies all the connection parameters back from the live PGconn itself so everything that's needed to connect is already validated. I don't like that this code maintains a second list of all possible libpq connection parameters. Where does it do that? In PQconninfo() itself? Why is it a problem? Or to put it bluntly: the same problem is in fillPGconn() too, as it also has the same set of parameters listed. So there is already code that you don't like. :-) How about a static mapping between option names and offsetof(struct pg_conn, member) values? This way both fillPGconn() and PQconninfo() can avoid maintaining the list of parameter names. The parameters to add to the connection string should be driven off the authoritative list in PQconninfoOptions. So, should I add a second flag to PQconninfoOption to indicate that certain options should not be used for primary_conninfo? Thanks and best regards, Zoltán Böszörményi -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
