Thread: Re: [BUGS] BUG #5957: createdb with description and md5 auth forces to provide password twice

Grzegorz Szpetkowski wrote:
>
> The following bug has been logged online:
>
> Bug reference:      5957
> Logged by:          Grzegorz Szpetkowski
> Email address:      gszpetkowski@gmail.com
> PostgreSQL version: 9.0.3
> Operating system:   Ubuntu 10.10
> Description:        createdb with description and md5 auth forces to provide
> password twice
> Details:
>
> How to reproduce the problem:
>
> 1.Create new role with (encrypted password):
> createuser -SdRP user
> 2.In PostgreSQL 9.0.3 I found pg_hba.conf with local all all ident, so
> change to local all all md5
> 3.Restart/Reload used cluster
> 4.Execute createdb -U user mydb "My DB Description"
>
> Output:
>
> Password:
> Password:
>
> creatdb command prompts password twice and I think it's improper behaviour
> (and documentation is silent about that).

Interesting.  This is happening because we are connecting to one
database to create the new database, and then connecting to the new
database to add the comment.

Prior to PG 8.2, this was necessary to put the comment on the database,
but now that we have the shared comment/description table
pg_shdescription, this is not necessary.

Do we need createdb to be able to create databases for pre-8.2 clusters?
If not, the attached patch fixes the double-prompting.

Also, why is this code used to create the new database?

    conn = connectDatabase(strcmp(dbname, "postgres") == 0 ? "template1" : "postgres",
                           host, port, username, prompt_password, progname);

Do we assume more users can connect to the 'postgres' database, but we
want 'postgres' to connect to 'template1' in case it wants to drop the
'postgres' database?  Whatever the purpose, this code certainly needs a
comment.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
new file mode 100644
index 9b72eac..b1c417b
*** a/src/bin/scripts/createdb.c
--- b/src/bin/scripts/createdb.c
*************** main(int argc, char *argv[])
*** 208,219 ****
      }

      PQclear(result);
-     PQfinish(conn);

      if (comment)
      {
-         conn = connectDatabase(dbname, host, port, username, prompt_password, progname);
-
          printfPQExpBuffer(&sql, "COMMENT ON DATABASE %s IS ", fmtId(dbname));
          appendStringLiteralConn(&sql, comment, conn);
          appendPQExpBuffer(&sql, ";\n");
--- 208,216 ----
*************** main(int argc, char *argv[])
*** 231,239 ****
          }

          PQclear(result);
-         PQfinish(conn);
      }

      exit(0);
  }

--- 228,237 ----
          }

          PQclear(result);
      }

+     PQfinish(conn);
+
      exit(0);
  }

Bruce Momjian <bruce@momjian.us> writes:
> Prior to PG 8.2, this was necessary to put the comment on the database,
> but now that we have the shared comment/description table
> pg_shdescription, this is not necessary.

> Do we need createdb to be able to create databases for pre-8.2 clusters?
> If not, the attached patch fixes the double-prompting.

Well, if you're only going to change this in HEAD, that might be an
acceptable limitation, but if you intend to back-patch I think not.
Older versions of createdb are probably significantly more likely to
be used with even-older servers.

Seems like it wouldn't be that hard to test the server version and only
reconnect if it's pre-8.2.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Prior to PG 8.2, this was necessary to put the comment on the database,
> > but now that we have the shared comment/description table
> > pg_shdescription, this is not necessary.
> 
> > Do we need createdb to be able to create databases for pre-8.2 clusters?
> > If not, the attached patch fixes the double-prompting.
> 
> Well, if you're only going to change this in HEAD, that might be an
> acceptable limitation, but if you intend to back-patch I think not.
> Older versions of createdb are probably significantly more likely to
> be used with even-older servers.

This code has been that way since pre-8.2 so I see no need to backpatch;
this is the first such complaint I have seen.

> Seems like it wouldn't be that hard to test the server version and only
> reconnect if it's pre-8.2.

I am not excited about adding more code for this so I am thinking
head-only.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > Prior to PG 8.2, this was necessary to put the comment on the database,
> > > but now that we have the shared comment/description table
> > > pg_shdescription, this is not necessary.
> >
> > > Do we need createdb to be able to create databases for pre-8.2 clusters?
> > > If not, the attached patch fixes the double-prompting.
> >
> > Well, if you're only going to change this in HEAD, that might be an
> > acceptable limitation, but if you intend to back-patch I think not.
> > Older versions of createdb are probably significantly more likely to
> > be used with even-older servers.
>
> This code has been that way since pre-8.2 so I see no need to backpatch;
> this is the first such complaint I have seen.
>
> > Seems like it wouldn't be that hard to test the server version and only
> > reconnect if it's pre-8.2.
>
> I am not excited about adding more code for this so I am thinking
> head-only.

Attached patch applied to head only.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
new file mode 100644
index 9b72eac..544f2f6
*** a/src/bin/scripts/createdb.c
--- b/src/bin/scripts/createdb.c
*************** main(int argc, char *argv[])
*** 192,197 ****
--- 192,202 ----

      appendPQExpBuffer(&sql, ";\n");

+     /*
+      * Connect to the 'postgres' database by default, except have
+      * the 'postgres' user use 'template1' so he can create the
+      * 'postgres' database.
+      */
      conn = connectDatabase(strcmp(dbname, "postgres") == 0 ? "template1" : "postgres",
                             host, port, username, prompt_password, progname);

*************** main(int argc, char *argv[])
*** 208,219 ****
      }

      PQclear(result);
-     PQfinish(conn);

      if (comment)
      {
-         conn = connectDatabase(dbname, host, port, username, prompt_password, progname);
-
          printfPQExpBuffer(&sql, "COMMENT ON DATABASE %s IS ", fmtId(dbname));
          appendStringLiteralConn(&sql, comment, conn);
          appendPQExpBuffer(&sql, ";\n");
--- 213,221 ----
*************** main(int argc, char *argv[])
*** 231,239 ****
          }

          PQclear(result);
-         PQfinish(conn);
      }

      exit(0);
  }

--- 233,242 ----
          }

          PQclear(result);
      }

+     PQfinish(conn);
+
      exit(0);
  }

diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
new file mode 100644
index 1cf18fd..48f73ae
*** a/src/bin/scripts/dropdb.c
--- b/src/bin/scripts/dropdb.c
*************** main(int argc, char *argv[])
*** 113,118 ****
--- 113,123 ----
      appendPQExpBuffer(&sql, "DROP DATABASE %s;\n",
                        fmtId(dbname));

+     /*
+      * Connect to the 'postgres' database by default, except have
+      * the 'postgres' user use 'template1' so he can drop the
+      * 'postgres' database.
+      */
      conn = connectDatabase(strcmp(dbname, "postgres") == 0 ? "template1" : "postgres",
                             host, port, username, prompt_password, progname);