2012-11-22 12:44 keltezéssel, Magnus Hagander írta:
On Thu, Nov 22, 2012 at 10:16 AM, Boszormenyi Zoltan <z...@cybertec.at> wrote:
2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta:
2012-11-21 22:15 keltezéssel, Magnus Hagander írta:
On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <z...@cybertec.at>
wrote:
Hi,
2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
I'm breaking this out into it's own thread, for my own sanity if
nothing else :) And it's an isolated feature after all.
I still agree with the previous review at
http://archives.postgresql.org/message-id/1349321071.23971.0.ca...@vanquo.pezone.net
about keeping the data in more than one place.
OK, it seems I completely missed that comment.
(Or forgot about it if I happened to answer it.)
Based on that, I've created a different version of this patch,
attached. This way we keep all the data in one struct.
I like this single structure but not the way you handle the
options' classes. In your patch, each option belongs to only
one class. These classes are:
PG_CONNINFO_REPLICATION = "replication" only
PG_CONNINFO_PASSWORD = "password" only
PG_CONNINFO_NORMAL = everything else
How does it help pg_basebackup to filter out e.g. dbname and
replication?
PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
actually, it shouldn't have the replication=1 part, right? So it
should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?
These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the
documentation.
Hmm. I wasn't actually thinking about the dbname part here, I admit that.
And not only the dbname, libpqwalreceiver adds these three:
[zozo@localhost libpqwalreceiver]$ grep dbname *
libpqwalreceiver.c: "%s dbname=replication replication=true
fallback_application_name=walreceiver",
I also excluded "application_name" from PG_CONNINFO_REPLICATION
by this reasoning:
- for async replication or single standby, it doesn't matter,
the connection will show up as "walreceiver"
- for sync replication, the administrator has to add the node name
manually via application_name.
In my view, the classes should be inclusive:
PG_CONNINFO_NORMAL = Everything that's usable for a regular client
connection. This mean everything, maybe including "password" but
excluding "replication".
PG_CONNINFO_PASSWORD = "password" only.
PG_CONNINFO_REPLICATION = Everything usable for a replication client
not added by walreceiver. Maybe including/excluding "password".
Maybe there should be two flags for replication usage:
PG_CONNINFO_WALRECEIVER = everything except those not added
by walreceiver (and maybe "password" too)
PG_CONNINFO_REPLICATION = "replication" only
And every option can belong to more than one class, just as in my patch.
Hmm. I kind of liked having each option in just one class, but I see
the problem. Looking at the ones you suggest, all the non-password
ones *have* to be without password, otherwise there i no way to get
the conninfo without password - which is the whole reason for that
parameter to exist. So the PASSWORD one has to be additional - which
means that not making the other ones additional makes them
inconsistent. But maybe we don't really have a choice there.
Yes, the PASSWORD part can be on its own, this is what I meant above
but wanted a different opinion about having it completely separate
is better or not.
But the NORMAL and REPLICATION (or WALRECEIVER) classes
need to overlap.
At this point, the patch is untested beyond compiling and a trivial
psql check, because I ran out of time :) But I figured I'd throw it
out there for comments on which version people prefer. (And yes, it's
quite possible I've made a big think-mistake in it somewhere, but
again, better to get some eyes on it early)
My version also contains a fixed version of the docs that should be
moved back into Zoltans version if that's the one we end up
preferring.
I also liked your version of the documentation better,
I am not too good at writing docs.
np.
Also, a question was buried in the other review which is - are we OK
to remove the requiressl parameter. Both these patches do so, because
the code becomes much simpler if we can do that. It has been
deprecated since 7.2. Is it OK to remove it, or do we need to put back
in the more complex code to deal with both?
Just going to highlight that we're looking for at least one third
party to comment on this :)
Yes, me too. A +1 for removing wouldn't count from me. ;-)
Attached is both Zoltans latest patch (v16) and my slightly different
approach.
Comments on which approach is best?
Test results from somebody who has the time to look at it? :)
Do you happen to have a set of tests you've been running on your
patches? Can you try them again this one?
My set of tests are:
1. initdb the master
2. pg_basebackup -R the first standby from the master
3. pg_basebackup -R the second standby from the master
4. pg_basebackup -R the third standby from the first standby
and diff -durpN the different data directories while there is no load on
either.
I will test it today after fixing the classes in your patch. ;-)
Attached is my v17 patch based on your version with the classes fixed.
I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN
which contains "dbname" and "fallback_application_name, and
PG_CONNINFO_REPLICATION_USER which contains everything else
except "replication".
Since pg_basebackup sets fallback_application_name and not
application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER.
I'm still not sure I like this. The "since pg_basebackup sets" part
really points to the problem - we're designing a public API for just
internal options here, aren't we? Is there *any* other application
that would ever use this?
I was thinking about this pg_receivexlog application but PQconninfo
is not needed there.
It would be a nice libpq extension to be able to login using a
PQconninfoOption array instead of the keyword/value arrays.
But the replication/normal distinction is not needed there either.
And if we do it like this, changing how
pg_basebackup works will make a change to our public libpq APIs in
worst case.
fallback_application_name certainly has *nothing* to do with
replication, in principle.
I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate), and
then plug in the exclusion of specific parameters in pg_basebackup, in
the CreateRecoveryConf() function. pg_basebackup will of course always
know what pg_basebackup is doing with these things.
OK, I will post a new pg_basebackup patch with this in mind
in its own thread.
Anyway, here are two small patches, the first is against your
previous patch to fix a typo and and the ignoreMissing argument
to conninfo_storeval().
The second one is the product of what caught my attention while
I was looking at pg_receivexlog. The current coding may write
beyond the end of the allocated arrays and the password may
overwrite a previously set keyword/value pair.
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.1/src/interfaces/libpq/fe-connect.c postgresql.1.1/src/interfaces/libpq/fe-connect.c
--- postgresql.1/src/interfaces/libpq/fe-connect.c 2012-11-22 13:42:42.820738413 +0100
+++ postgresql.1.1/src/interfaces/libpq/fe-connect.c 2012-11-22 13:49:00.828604651 +0100
@@ -4005,7 +4005,7 @@ conninfo_init(PQExpBuffer errorMessage,
const internalPQconninfoOption *cur_opt;
/*
- * Get enough memory for all optoins in PQconninfoOptions, even if some
+ * Get enough memory for all options in PQconninfoOptions, even if some
* end up being filtered out.
*/
options = (PQconninfoOption *) malloc(sizeof(PQconninfoOption) * sizeof(PQconninfoOptions)/sizeof(PQconninfoOptions[0]));
@@ -5107,7 +5107,7 @@ PQconninfo(PGconn *conn, unsigned int fl
if (*connmember)
conninfo_storeval(connOptions, option->keyword, *connmember,
- &errorBuf, false, false);
+ &errorBuf, true, false);
}
}
--- postgresql.1.1/src/bin/pg_basebackup/streamutil.c.old 2012-11-22 13:57:09.954235023 +0100
+++ postgresql.1.1/src/bin/pg_basebackup/streamutil.c 2012-11-22 14:02:19.712243537 +0100
@@ -77,8 +77,8 @@
GetConnection(void)
{
PGconn *tmpconn;
- int argcount = 4; /* dbname, replication, fallback_app_name,
- * password */
+ int argcount = 7; /* dbname, replication, fallback_app_name,
+ * host, user, port, password */
int i;
const char **keywords;
const char **values;
@@ -133,15 +133,15 @@
* meaning this is the call for a second session to the same
* database, so just forcibly reuse that password.
*/
- keywords[argcount - 1] = "password";
- values[argcount - 1] = dbpassword;
+ keywords[i] = "password";
+ values[i] = dbpassword;
dbgetpassword = -1; /* Don't try again if this fails */
}
else if (dbgetpassword == 1)
{
password = simple_prompt(_("Password: "), 100, false);
- keywords[argcount - 1] = "password";
- values[argcount - 1] = password;
+ keywords[i] = "password";
+ values[i] = password;
}
tmpconn = PQconnectdbParams(keywords, values, true);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
