Re: [HACKERS] pg_dump roles support

[Tom Lane]

=?ISO-8859-1?Q?Benedek_L=E1szl=F3?= l...@benedekl.tvnetwork.hu writes:
 Here is an updated patch, which deals with 's in the rolename.

Committed with revisions as per subsequent discussion: pg_restore has
its own switch and there's no change in archive contents.

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] pg_dump roles support [Review]

[Tom Lane]

[ starting to examine this patch now... ]

=?UTF-8?B?QmVuZWRlayBMw6FzemzDsw==?= l...@benedekl.tvnetwork.hu writes:
 I also need some feedback about the role support in pg_restore (not
 implemented yet).  Currently pg_restore sets the role during the
 restore process according to the TOC entry in the archive. It may also
 support the --role option (just like pg_dump).  If specified it can be
 used to cancel the effect of the TOC entry and force the emitting of
 the SET ROLE ... command. With emtpy argument it can be used to omit
 the SET ROLE even if it is specified in the archieve. What do you
 think?

I think that the entire concept of putting the rolename into the archive
is broken, and we should not do that part at all.  But we *especially*
should not do it if there is no way to override it.

I see no good reason to assume that the appropriate role to use during
restore is the same as that during dump.  We don't reflect the -U
setting into the dump file, and --role is really just an auxiliary
extension to -U.  What would make sense is to have a --role switch in
pg_restore, but have that function entirely independently of what
happened at dump time, just as is true for -U.

So my thought is:

--role switch for pg_dump and pg_dumpall: sets the role used while
dumping, has no effect on the emitted archive.

--role switch for pg_restore: sets the role used while restoring,
if it's to be different from what -U says.

This ignores the case of plain-text output from pg_dump, but you
don't really need any support for that case, as you can do the
restore like so:

psql -U admin_user target_db

target_db= SET ROLE superuser;
target_db=# \i dumpfile.sql

Comments?

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] pg_dump roles support [Review]

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 --role switch for pg_dump and pg_dumpall: sets the role used while
 dumping, has no effect on the emitted archive.
 
 --role switch for pg_restore: sets the role used while restoring,
 if it's to be different from what -U says.

As one of the original requestors for this capability, just wanted to
add my 2c that this will work for me and makes sense to me.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump roles support [Review]

[Abhijit Menon-Sen]

Hi Benedek.

At 2008-11-06 15:08:14 +0100, l...@benedekl.tvnetwork.hu wrote:

 I created an updated patch according to your notices.

I had a look at your updated patch, and it looks fine. I fiddled with
the documentation a little, and fixed up one place where the code had
drifted and the patch didn't apply.

-- ams
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 2e30906..9395c0a 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -698,6 +698,23 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies a role name to be used to create the dump. This causes
+applicationpg_dump/ to issue a
+commandSET ROLE TO replaceable class=parameterrolename//
+command after connecting to the database. It is useful when the
+authenticated user (specified by option-U/) lacks privileges
+needed by applicationpg_dump/, but can switch to a role with
+the required rights. The SET ROLE command is repeated in the
+resulting archive because it is usually also needed for the
+restore to succeed.
+   /para
+  /listitem
+ /varlistentry
 /variablelist
/para
  /refsect1
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index ec40890..5c9e6fa 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -417,6 +417,23 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies a role name to be used to create the dump. This causes
+applicationpg_dumpall/ to issue a
+commandSET ROLE TO replaceable class=parameterrolename//
+command after connecting to the database. It is useful when the
+authenticated user (specified by option-U/) lacks privileges
+needed by applicationpg_dumpall/, but can switch to a role
+with the required rights. This option is passed on to
+applicationpg_dump/, and the SET ROLE command is repeated in
+the output archive as well.
+   /para
+  /listitem
+ /varlistentry
/variablelist
   /para
  /refsect1
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index c57bb22..cbe4d46 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -70,6 +70,8 @@ typedef struct _Archive
 	int			encoding;		/* libpq code for client_encoding */
 	bool		std_strings;	/* standard_conforming_strings */
 
+	const char	   *rolename;			/* role name */
+
 	/* error handling */
 	bool		exit_on_error;	/* whether to exit on SQL errors... */
 	int			n_errors;		/* number of errors (if no die) */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0bbba25..69a5d9a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -56,6 +56,7 @@ static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
 static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
 static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
 static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
+static void processRolenameEntry(ArchiveHandle *AH, TocEntry *te);
 static teReqs _tocEntryRequired(TocEntry *te, RestoreOptions *ropt, bool include_acls);
 static void _disableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt);
 static void _enableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt);
@@ -1979,6 +1980,8 @@ ReadToc(ArchiveHandle *AH)
 			processEncodingEntry(AH, te);
 		else if (strcmp(te-desc, STDSTRINGS) == 0)
 			processStdStringsEntry(AH, te);
+		else if (strcmp(te-desc, ROLENAME) == 0)
+			processRolenameEntry(AH, te);
 	}
 }
 
@@ -2026,14 +2029,38 @@ processStdStringsEntry(ArchiveHandle *AH, TocEntry *te)
 	 te-defn);
 }
 
+static void
+processRolenameEntry(ArchiveHandle *AH, TocEntry *te)
+{
+	/* te-defn should have the form SET role = 'foo'; */
+	char	   *defn = strdup(te-defn);
+	char	   *ptr1;
+	char	   *ptr2 = NULL;
+
+	ptr1 = strchr(defn, '\'');
+	if (ptr1)
+		ptr2 = strchr(++ptr1, '\'');
+	if (ptr2)
+	{
+		*ptr2 = '\0';
+		AH-public.rolename = strdup(ptr1);
+	}
+	else
+		die_horribly(AH, modulename, invalid ROLENAME item: %s\n,
+	 te-defn);
+
+	free(defn);
+}
+
 static teReqs
 _tocEntryRequired(TocEntry *te, RestoreOptions *ropt, bool include_acls)
 {
 	teReqs		res = REQ_ALL;
 
-	/* ENCODING and STDSTRINGS items are dumped specially, so always reject */
+	/* ENCODING, STDSTRINGS and ROLENAME items are dumped specially, so always reject */
 	if (strcmp(te-desc, ENCODING) == 0 ||
-		strcmp(te-desc, STDSTRINGS) == 0)
+	

Re: [HACKERS] pg_dump roles support

[Benedek László]

On 2008-11-08 09:25, Benedek László wrote:

Does this work if the role name contains a ' ?


Right, this one fails with ' in the role name. An update coming soon 
closing this issue.



Here is an updated patch, which deals with 's in the rolename.
Please review.

 doc/src/sgml/ref/pg_dump.sgml|   16 +
 doc/src/sgml/ref/pg_dumpall.sgml |   15 
 src/bin/pg_dump/pg_backup.h  |2 +
 src/bin/pg_dump/pg_backup_archiver.c |   35 ++-
 src/bin/pg_dump/pg_dump.c|   60 
+-

 src/bin/pg_dump/pg_dumpall.c |   23 +
 6 files changed, 148 insertions(+), 3 deletions(-)

Thank you, regards

Benedek Laszlo

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 2e30906..5e4c3e0 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -698,6 +698,22 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This cause
+applicationpg_dump/application to issue a
+commandSET role = replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dump/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive because most of the time this
+user identifier also needed for the restore to succeed.
+   /para
+  /listitem
+ /varlistentry
 /variablelist
/para
  /refsect1
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index ec40890..640723d 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -417,6 +417,21 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This option is passed
+to applicationpg_dump/ too and cause these applications to issue a
+commandSET role = replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dumpall/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive by applicationpg_dump/application.
+   /para
+  /listitem
+ /varlistentry
/variablelist
   /para
  /refsect1
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index c57bb22..c9e7e72 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -70,6 +70,8 @@ typedef struct _Archive
 	int			encoding;		/* libpq code for client_encoding */
 	bool		std_strings;	/* standard_conforming_strings */
 
+	char	   *rolename;		/* role name in escaped form */
+
 	/* error handling */
 	bool		exit_on_error;	/* whether to exit on SQL errors... */
 	int			n_errors;		/* number of errors (if no die) */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7bd44f2..53bbfdf 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -56,6 +56,7 @@ static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
 static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
 static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
 static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
+static void processRolenameEntry(ArchiveHandle *AH, TocEntry *te);
 static teReqs _tocEntryRequired(TocEntry *te, RestoreOptions *ropt, bool include_acls);
 static void _disableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt);
 static void _enableTriggersIfNecessary(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt);
@@ -1979,6 +1980,8 @@ ReadToc(ArchiveHandle *AH)
 			processEncodingEntry(AH, te);
 		else if (strcmp(te-desc, STDSTRINGS) == 0)
 			processStdStringsEntry(AH, te);
+		else if (strcmp(te-desc, ROLENAME) == 0)
+			processRolenameEntry(AH, te);
 	}
 }
 
@@ -2026,14 +2029,38 @@ processStdStringsEntry(ArchiveHandle *AH, TocEntry *te)
 	 te-defn);
 }
 
+static void
+processRolenameEntry(ArchiveHandle *AH, TocEntry *te)
+{
+	/* te-defn should have the form SET role = foo; */
+	char	   *defn = strdup(te-defn);
+	char	   *ptr1;
+	char	   *ptr2 = NULL;
+
+	ptr1 = strchr(defn, '');
+	if (ptr1)
+		ptr2 = strrchr(ptr1+1, '');
+	if (ptr2)
+	{
+		

Re: [HACKERS] pg_dump roles support [Review]

[Ibrar Ahmed]

On Thu, Nov 6, 2008 at 8:08 PM, Benedek László
[EMAIL PROTECTED] wrote:
 Hi,

 Thanks for your review.

 I created an updated patch according to your notices.

 1 - Patch does not apply cleanly on latest git repository, although
 there is no hunk failed but there are some hunk succeeded messages.

 Rebased to the current HEAD.

 2- Patch contains unnecessary spaces and tabs which makes the patch
 unnecessarily big. IMHO please read the patch before sending and make
 sure that patch only contains the changes you intended to send.

 Yes, there were trailing whitespaces in the original files which
 were removed by the previous patch. The attached version leaves them as is.

 3 - We should follow the coding standards of existing code

 I tried, of course, but this escaped my observation.

 4 - pg_restore gives error wile restoring custom format backup
 5 - Do you really want to write this code like this

 Fixed.


 I also need some feedback about the role support in pg_restore (not
 implemented yet).
 Currently pg_restore sets the role during the restore process according to
 the TOC
 entry in the archive. It may also support the --role option (just like
 pg_dump).
 If specified it can be used to cancel the effect of the TOC entry and force
 the
 emitting of the SET ROLE ... command. With emtpy argument it can be used to
 omit
 the SET ROLE even if it is specified in the archieve. What do you think?


Now this patch looks OK to me. As for as pg_restore is concern I think
we should not add this option into pg_restore.

What advantages do you want to get by using SET ROLE in pg_restore?




 Thank you again.

  doc/src/sgml/ref/pg_dump.sgml|   16 ++
  doc/src/sgml/ref/pg_dumpall.sgml |   15 +
  src/bin/pg_dump/pg_backup.h  |2 +
  src/bin/pg_dump/pg_backup_archiver.c |   36 +-
  src/bin/pg_dump/pg_dump.c|   53
 ++
  src/bin/pg_dump/pg_dumpall.c |   23 ++
  6 files changed, 143 insertions(+), 2 deletions(-)






-- 
   Ibrar Ahmed
   EnterpriseDB   http://www.enterprisedb.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] pg_dump roles support

[Benedek László]

Hi,

Thank you for your review.

On 2008-11-07 21:20, Alvaro Herrera wrote:

The patch contains the following things:


- pg_dump and pg_dumpall accepts the --role=rolename parameter, and
sends a SET ROLE command on their connections
 


Minor comment -- I think you need to quote the role name in the SET
command.  Otherwise roles with funny names will fail (try a role with a
space for example)

   

Of course you need to quote the role names with special characters in it.
I tested it this way (from bash):
$ src/bin/pg_dump/pg_dump -h localhost -p 4003 --role asd ' \ qwe test
Note the bash style escaping of the string [asd '  qwe].
It created a dump file with SET role = asd '  qwe; line in it. Seems 
fine for me.

The SGML patch seems to contain unnecessary whitespace changes; please
clean that up.
   

Maybe you missed an updated version of the patch? Available here:
http://archives.postgresql.org/pgsql-hackers/2008-11/msg00391.php


+   /* te-defn should have the form SET role = 'foo'; */
+   char   *defn = strdup(te-defn);
+   char   *ptr1;
+   char   *ptr2 = NULL;
+
+   ptr1 = strchr(defn, '\'');
+   if (ptr1)
+   ptr2 = strchr(++ptr1, '\'');
 


Does this work if the role name contains a ' ?
   

Right, this one fails with ' in the role name. An update coming soon closing 
this issue.


Regards,

Benedek Laszlo



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump roles support

[Alvaro Herrera]

Benedek László wrote:

Hi,

 The patch contains the following things:


 - pg_dump and pg_dumpall accepts the --role=rolename parameter, and  
 sends a SET ROLE command on their connections

Minor comment -- I think you need to quote the role name in the SET
command.  Otherwise roles with funny names will fail (try a role with a
space for example)

 - sgml documentation of this feature

The SGML patch seems to contain unnecessary whitespace changes; please
clean that up.

 + /* te-defn should have the form SET role = 'foo'; */
 + char   *defn = strdup(te-defn);
 + char   *ptr1;
 + char   *ptr2 = NULL;
 +
 + ptr1 = strchr(defn, '\'');
 + if (ptr1)
 + ptr2 = strchr(++ptr1, '\'');

Does this work if the role name contains a ' ?

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump roles support [Review]

[Benedek László]

Hi,

Thanks for your review.

I created an updated patch according to your notices.


1 - Patch does not apply cleanly on latest git repository, although
there is no hunk failed but there are some hunk succeeded messages.

Rebased to the current HEAD.


2- Patch contains unnecessary spaces and tabs which makes the patch
unnecessarily big. IMHO please read the patch before sending and make
sure that patch only contains the changes you intended to send.

Yes, there were trailing whitespaces in the original files which
were removed by the previous patch. The attached version leaves them as is.


3 - We should follow the coding standards of existing code

I tried, of course, but this escaped my observation.


4 - pg_restore gives error wile restoring custom format backup
5 - Do you really want to write this code like this

Fixed.

I also need some feedback about the role support in pg_restore (not implemented 
yet).
Currently pg_restore sets the role during the restore process according to the 
TOC
entry in the archive. It may also support the --role option (just like pg_dump).
If specified it can be used to cancel the effect of the TOC entry and force the
emitting of the SET ROLE ... command. With emtpy argument it can be used to omit
the SET ROLE even if it is specified in the archieve. What do you think?

Thank you again.

  doc/src/sgml/ref/pg_dump.sgml|   16 ++
  doc/src/sgml/ref/pg_dumpall.sgml |   15 +
  src/bin/pg_dump/pg_backup.h  |2 +
  src/bin/pg_dump/pg_backup_archiver.c |   36 +-
  src/bin/pg_dump/pg_dump.c|   53 ++
  src/bin/pg_dump/pg_dumpall.c |   23 ++
  6 files changed, 143 insertions(+), 2 deletions(-)


diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 2e30906..de139c3 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -698,6 +698,22 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This cause
+applicationpg_dump/application to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dump/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive because most of the time this
+user identifier also needed for the restore to succeed.
+   /para
+  /listitem
+ /varlistentry
 /variablelist
/para
  /refsect1
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index ec40890..e3016cd 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -417,6 +417,21 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This option is passed
+to applicationpg_dump/ too and cause these applications to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dumpall/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive by applicationpg_dump/application.
+   /para
+  /listitem
+ /varlistentry
/variablelist
   /para
  /refsect1
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index c57bb22..cbe4d46 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -70,6 +70,8 @@ typedef struct _Archive
 	int			encoding;		/* libpq code for client_encoding */
 	bool		std_strings;	/* standard_conforming_strings */
 
+	const char	   *rolename;			/* role name */
+
 	/* error handling */
 	bool		exit_on_error;	/* whether to exit on SQL errors... */
 	int			n_errors;		/* number of errors (if no die) */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7bd44f2..53a469d 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -56,6 +56,7 @@ static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
 static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
 static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
 static void 

[HACKERS] pg_dump roles support [Review]

[Ibrar Ahmed]

Just a superficial review.  I haven't really looked hard at this yet.

1 - Patch does not apply cleanly on latest git repository, although
there is no hunk failed but there are some hunk succeeded messages.

2- Patch contains unnecessary spaces and tabs which makes the patch
unnecessarily big. IMHO please read the patch before sending and make
sure that patch only contains the changes you intended to send.

3 - We should follow the coding standards of existing code

   destroyPQExpBuffer(roleQry);
   g_fout-rolename = pgrole;
   } else {
   g_fout-rolename = NULL;
   }

Should be written like this

   destroyPQExpBuffer(roleQry);
   g_fout-rolename = pgrole;
   }
   else
   {
   g_fout-rolename = NULL;
   }


4 - pg_restore gives error wile restoring custom format backup

pg_restore: [archiver] invalid ROLENAME item: SET role = 'ibrar';
(reason check point 5)

5 - Do you really want to write this code like this

+   if (ptr2)
+   {
+   *ptr2 = '\0';
+   AH-public.rolename = strdup(ptr1);
+   free(defn);5 -
+   }
+   else
+   free(defn);
+   die_horribly(AH, modulename, invalid ROLENAME item: %s\n,
+te-defn);

I think you missed curly brackets of else here.

Please send updated patch!

--
  Ibrar Ahmed
  EnterpriseDB   http://www.enterprisedb.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] pg_dump roles support

[Benedek László]

Hello All,

in my last mail 
http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg114835.html
I have sent you a patch of pg_dump and pg_dumpall --role option support. 
Nobody replied
or commented jet so now I send it again. The attached patch is the same 
as the last one,

except it is against the current upstream HEAD.

The patch contains the following things:


- pg_dump and pg_dumpall accepts the --role=rolename parameter, and 
sends a SET ROLE


command on their connections
- pg_dumpall passes this option to the called pg_dump process
- pg_dump emits the SET ROLE command into the archive
- sgml documentation of this feature


Summary:

 doc/src/sgml/ref/pg_dump.sgml|   16 ++
 doc/src/sgml/ref/pg_dumpall.sgml |   27 +
 src/bin/pg_dump/pg_backup.h  |2 +
 src/bin/pg_dump/pg_backup_archiver.c |   38 +--
 src/bin/pg_dump/pg_dump.c|   55 
+-

 src/bin/pg_dump/pg_dumpall.c |   26 +++-
 6 files changed, 153 insertions(+), 11 deletions(-)

Thank you, best regards:
Laszlo Benedek

diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 2e30906..de139c3 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -698,6 +698,22 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This cause
+applicationpg_dump/application to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dump/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive because most of the time this
+user identifier also needed for the restore to succeed.
+   /para
+  /listitem
+ /varlistentry
 /variablelist
/para
  /refsect1
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index ec40890..16f3e0b 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -129,7 +129,7 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
- 
+
  varlistentry
   termoption-f replaceable class=parameterfilename/replaceable/option/term
   termoption--file=replaceable class=parameterfilename/replaceable/option/term
@@ -183,7 +183,7 @@ PostgreSQL documentation
 Do not output commands to set
 ownership of objects to match the original database.
 By default, applicationpg_dumpall/application issues
-commandALTER OWNER/ or 
+commandALTER OWNER/ or
 commandSET SESSION AUTHORIZATION/command
 statements to set ownership of created schema elements.
 These statements
@@ -266,7 +266,7 @@ PostgreSQL documentation
   /listitem
  /varlistentry
 
- varlistentry
+	 varlistentry
   termoption-v//term
   termoption--verbose//term
   listitem
@@ -354,7 +354,7 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
- 
+
  varlistentry
   term-l replaceabledbname/replaceable/term
   term--database=replaceabledbname/replaceable/term
@@ -397,7 +397,7 @@ PostgreSQL documentation
   listitem
para
 Force applicationpg_dumpall/application to prompt for a
-password before connecting to a database.  
+password before connecting to a database.
/para
 
para
@@ -417,6 +417,21 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This option is passed
+to applicationpg_dump/ too and cause these applications to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dumpall/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive by applicationpg_dump/application.
+   /para
+  /listitem
+ /varlistentry
/variablelist
   /para
  /refsect1
@@ -503,6 +518,6 @@ PostgreSQL documentation
 Check xref linkend=app-pgdump for details on possible
 error conditions.
   /para
- /refsect1   
+ /refsect1
 
 /refentry
diff --git a/src/bin/pg_dump/pg_backup.h 

Re: [HACKERS] pg_dump roles support

[Benedek László]

Hello,

I modified my previous patch supporting the --role option in pg_dump and 
pg_dumpall.

The attached patch contains the following things:

- pg_dump and pg_dumpall accepts the --role=rolename parameter, and 
sends a SET ROLE

command on their connections
- pg_dumpall passes this option to the called pg_dump process
- pg_dump emits the SET ROLE command into the archive
- sgml documentation of this feature

Please review it

Regards, Laszlo Benedek
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 2e30906..de139c3 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -698,6 +698,22 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This cause
+applicationpg_dump/application to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dump/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive because most of the time this
+user identifier also needed for the restore to succeed.
+   /para
+  /listitem
+ /varlistentry
 /variablelist
/para
  /refsect1
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index ec40890..16f3e0b 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -129,7 +129,7 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
- 
+
  varlistentry
   termoption-f replaceable class=parameterfilename/replaceable/option/term
   termoption--file=replaceable class=parameterfilename/replaceable/option/term
@@ -183,7 +183,7 @@ PostgreSQL documentation
 Do not output commands to set
 ownership of objects to match the original database.
 By default, applicationpg_dumpall/application issues
-commandALTER OWNER/ or 
+commandALTER OWNER/ or
 commandSET SESSION AUTHORIZATION/command
 statements to set ownership of created schema elements.
 These statements
@@ -266,7 +266,7 @@ PostgreSQL documentation
   /listitem
  /varlistentry
 
- varlistentry
+	 varlistentry
   termoption-v//term
   termoption--verbose//term
   listitem
@@ -354,7 +354,7 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
- 
+
  varlistentry
   term-l replaceabledbname/replaceable/term
   term--database=replaceabledbname/replaceable/term
@@ -397,7 +397,7 @@ PostgreSQL documentation
   listitem
para
 Force applicationpg_dumpall/application to prompt for a
-password before connecting to a database.  
+password before connecting to a database.
/para
 
para
@@ -417,6 +417,21 @@ PostgreSQL documentation
/para
   /listitem
  /varlistentry
+
+ varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This option is passed
+to applicationpg_dump/ too and cause these applications to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection. It is useful in cases when
+the logged in user specified by the -U option has not enough privileges needed by
+applicationpg_dumpall/application but can switch to a role with the needed rights.
+The SET ROLE command is reserved in the archive by applicationpg_dump/application.
+   /para
+  /listitem
+ /varlistentry
/variablelist
   /para
  /refsect1
@@ -503,6 +518,6 @@ PostgreSQL documentation
 Check xref linkend=app-pgdump for details on possible
 error conditions.
   /para
- /refsect1   
+ /refsect1
 
 /refentry
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index c57bb22..cbe4d46 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -70,6 +70,8 @@ typedef struct _Archive
 	int			encoding;		/* libpq code for client_encoding */
 	bool		std_strings;	/* standard_conforming_strings */
 
+	const char	   *rolename;			/* role name */
+
 	/* error handling */
 	bool		exit_on_error;	/* whether to exit on SQL errors... */
 	int			n_errors;		/* number of errors (if no die) */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7bd44f2..6f6ed2f 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ 

Re: [HACKERS] pg_dump roles support

[Benedek László]

Tom Lane wrote:

Some other review nitpicking:

   

Thank you for your review. I really need all suggestions, since I never
posted any patch to the community before.

The next patch will emit the SET ROLE command in the generated dump,
as you and Stephen said. This will fit in my workflow too, since mostly I
need to restore using the same role as the dump.


Regards, Laszlo Benedek

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump roles support

[Tom Lane]

=?ISO-8859-1?Q?Benedek_L=E1szl=F3?= [EMAIL PROTECTED] writes:
 pg_dumpall now just passes the --role option to pg_dump. What do you 
 think, is it enough
 or it should issue the SET ROLE TO ... command in its own session too?

I think it would have to, in the general case.  Consider the possibility
that someone has restricted access to the system catalogs, for instance.

You have missed an important component of Stephen's original proposal,
which was the point that something similar is needed on the restore
side.  This is a little bit tricky since the context at restore time
is not necessarily the same as the context at dump time.  When using
an archive file it's not a problem: the behavior can be driven off a
--role switch to pg_restore, and this is independent of what pg_dump
did.  In a dump to plain text, though, I'm not sure what to do.  The
simplest design would have pg_dump's --role switch control both
what it does in its own connection to the source database, and what it
puts into the output script.  I'm not sure that's adequate though.
Is it worth having two different switches for the two cases?  If we
think it's a corner case to need different role IDs, we could just
leave it like that and tell anyone who needs different behaviors that
they have to go through an archive file and pg_restore.  Stephen,
you were the one who wanted this in the first place, what's your
use-cases look like?

Some other review nitpicking:

The documentation part of the patch is well short of acceptable IMHO,
since it gives no hint of what this switch might be good for, and
indeed encourages the user to confuse it with the -U switch by injecting
a mention of it into the middle of a discussion about -U.

It is not normally considered appropriate for individual patches to
edit the release notes; and it's DEFINITELY not appropriate to put
a mention of a feature addition into the wrong section of the release
notes.

 + {role, required_argument, NULL, 'r' + 0x80},

This is not a good choice of option code IMHO ... what if the value is
stored in a signed char on some machines?  If you can't find a free
letter you like, use a small integer code, as you can find being done
elsewhere.

BTW, the patch fails to compile on a strict ANSI C compiler, because
you are using a C++-ism of declaring a variable mid-block.

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] pg_dump roles support

[Stephen Frost]

* Tom Lane ([EMAIL PROTECTED]) wrote:
 =?ISO-8859-1?Q?Benedek_L=E1szl=F3?= [EMAIL PROTECTED] writes:
  pg_dumpall now just passes the --role option to pg_dump. What do you 
  think, is it enough
  or it should issue the SET ROLE TO ... command in its own session too?
 
 I think it would have to, in the general case.  Consider the possibility
 that someone has restricted access to the system catalogs, for instance.

I would agree with this.  pg_dumpall should do the 'set role' in its
session as well.

 You have missed an important component of Stephen's original proposal,
 which was the point that something similar is needed on the restore
 side.  This is a little bit tricky since the context at restore time
 is not necessarily the same as the context at dump time.  When using
 an archive file it's not a problem: the behavior can be driven off a
 --role switch to pg_restore, and this is independent of what pg_dump
 did.  In a dump to plain text, though, I'm not sure what to do.  The
 simplest design would have pg_dump's --role switch control both
 what it does in its own connection to the source database, and what it
 puts into the output script.  I'm not sure that's adequate though.

This makes sense to me and I feel it's adequate.  If necessary, people
can post-process their .sql files using sed or something similar.
That's made reasonably easy by having a 'set role' in the .sql file.  I
actively dislike the idea that pg_restore would modify the input stream
from a text file, even if it was passed a --role switch.

 Is it worth having two different switches for the two cases?  If we
 think it's a corner case to need different role IDs, we could just
 leave it like that and tell anyone who needs different behaviors that
 they have to go through an archive file and pg_restore.  Stephen,
 you were the one who wanted this in the first place, what's your
 use-cases look like?

My primary use cases are performing a pg_dump when logging in as one
user but needing the permissions of another role, followed by loading
the data into another system when logging in as one user and needing to
set role first to another.  In at least 90% of those cases, that role is
postgres, and in the other 10% most, if not all, are the same role on
both sides.  There are a few cases where we might change the restore-as
role away from the dumped-as role, but we're happy to use pg_restore to
handle that, or take care of changing the role in the .sql file (which
is what we tend to use, honestly) using sed or similar.

Alot of this is driven from the fact that we don't allow admins to
remotely connect directly as postgres (akin to disabling remote root
logins in sshd_config via PermitRootLogin, and for the same reasons).
They must authenticate and connect as their own user first and then use
'set role postgres;' to gain superuser rights.  Not being able to have
pg_dump do that set role has been quite frustrating as we use it
extensively for transferring data between systems.

 Some other review nitpicking:

I agree with the other comments.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump roles support

[Benedek László]

Hello,

daveg wrote:

I created a patch to set the role to a specified name just after the db
connection.
 

I was going to do this, but you have beat me to it. You will want to update
the sgml documentation, and pg_dumpall as well.

-dg

   

Ok, here is the next one.
pg_dumpall now just passes the --role option to pg_dump. What do you 
think, is it enough

or it should issue the SET ROLE TO ... command in its own session too?

Laszlo Benedek

diff -ur postgresql-8.3.1.orig/doc/src/sgml/backup.sgml postgresql-8.3.1/doc/src/sgml/backup.sgml
--- postgresql-8.3.1.orig/doc/src/sgml/backup.sgml	2008-03-07 02:46:50.0 +0100
+++ postgresql-8.3.1/doc/src/sgml/backup.sgml	2008-08-27 15:29:26.0 +0200
@@ -68,7 +68,9 @@
applicationpg_dump/ will by default connect with the database
user name that is equal to the current operating system user name. To override
this, either specify the option-U/option option or set the
-   environment variable envarPGUSER/envar. Remember that
+   environment variable envarPGUSER/envar. It is possible to change
+   the current user identifier of the dump session by using the
+   option--role/option option. Remember that
applicationpg_dump/ connections are subject to the normal
client authentication mechanisms (which are described in xref
linkend=client-authentication).
diff -ur postgresql-8.3.1.orig/doc/src/sgml/ref/pg_dump.sgml postgresql-8.3.1/doc/src/sgml/ref/pg_dump.sgml
--- postgresql-8.3.1.orig/doc/src/sgml/ref/pg_dump.sgml	2007-12-11 20:57:32.0 +0100
+++ postgresql-8.3.1/doc/src/sgml/ref/pg_dump.sgml	2008-08-27 15:58:05.0 +0200
@@ -522,6 +522,18 @@
  /varlistentry
 
  varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This will cause
+applicationpg_dump/application to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection.
+   /para
+  /listitem
+ /varlistentry
+ 
+ varlistentry
   termoption-v//term
   termoption--verbose//term
   listitem
diff -ur postgresql-8.3.1.orig/doc/src/sgml/ref/pg_dumpall.sgml postgresql-8.3.1/doc/src/sgml/ref/pg_dumpall.sgml
--- postgresql-8.3.1.orig/doc/src/sgml/ref/pg_dumpall.sgml	2007-12-11 20:57:32.0 +0100
+++ postgresql-8.3.1/doc/src/sgml/ref/pg_dumpall.sgml	2008-08-27 15:49:18.0 +0200
@@ -248,6 +248,18 @@
  /varlistentry
 
  varlistentry
+  termoption--role=replaceable class=parameterrolename/replaceable/option/term
+  listitem
+   para
+Specifies the user identifier used by the dump session. This option will be passed
+to applicationpg_dump/ and will cause applicationpg_dump/application to issue a
+commandSET ROLE TO replaceable class=parameterrolename/replaceable/command
+command just after a successful database connection.
+   /para
+  /listitem
+ /varlistentry
+
+ varlistentry
   termoption-v//term
   termoption--verbose//term
   listitem
diff -ur postgresql-8.3.1.orig/doc/src/sgml/release.sgml postgresql-8.3.1/doc/src/sgml/release.sgml
--- postgresql-8.3.1.orig/doc/src/sgml/release.sgml	2008-03-14 00:47:59.0 +0100
+++ postgresql-8.3.1/doc/src/sgml/release.sgml	2008-08-27 16:06:12.0 +0200
@@ -2395,6 +2395,13 @@
 
  listitem
   para
+   Add literal--role/ option to applicationpg_dump/application and
+   applicationpg_dumpall/application (Benedek Laszlo)
+  /para
+ /listitem
+
+ listitem
+  para
Add literal--tablespaces-only/ and literal--roles-only/
options to applicationpg_dumpall/application (Dave Page)
   /para
diff -ur postgresql-8.3.1.orig/src/bin/pg_dump/pg_dump.c postgresql-8.3.1/src/bin/pg_dump/pg_dump.c
--- postgresql-8.3.1.orig/src/bin/pg_dump/pg_dump.c	2008-01-30 19:35:55.0 +0100
+++ postgresql-8.3.1/src/bin/pg_dump/pg_dump.c	2008-08-27 15:10:41.0 +0200
@@ -208,6 +208,7 @@
 	const char *pgport = NULL;
 	const char *username = NULL;
 	const char *dumpencoding = NULL;
+	const char *pgrole = NULL;
 	const char *std_strings;
 	bool		oids = false;
 	TableInfo  *tblinfo;
@@ -258,6 +259,7 @@
 		{no-acl, no_argument, NULL, 'x'},
 		{compress, required_argument, NULL, 'Z'},
 		{encoding, required_argument, NULL, 'E'},
+		{role, required_argument, NULL, 'r' + 0x80},
 		{help, no_argument, NULL, '?'},
 		{version, no_argument, NULL, 'V'},
 
@@ -437,6 +439,10 @@
 /* This covers the long options equivalent to -X xxx. */
 break;
 
+			case 'r' + 0x80:	/* role */
+pgrole = optarg;
+break;
+
 			default:
 fprintf(stderr, _(Try \%s --help\ for more information.\n), progname);
 exit(1);
@@ -539,6 +545,18 @@
 			exit(1);
 		}
 	}
+	
+	/* Set the role if requested */
+	if (pgrole)
+	{
+		PQExpBuffer roleQry = 

Re: [HACKERS] pg_dump roles support

[Benedek László]

Hello,

Stephen Frost wrote:
 As I discuss above, it'd be really nice have a --role or similar option
 to ask pg_dump to set role to a particular user before dumping the
 database.

I created a patch to set the role to a specified name just after the db 
connection.
Please review it for possible upstream inclusion.

Regards,
Laszlo Benedek


--- postgresql-8.3.1.orig/src/bin/pg_dump/pg_dump.c	2008-01-30 19:35:55.0 +0100
+++ postgresql-8.3.1/src/bin/pg_dump/pg_dump.c	2008-08-26 12:26:56.0 +0200
@@ -208,6 +208,7 @@
 	const char *pgport = NULL;
 	const char *username = NULL;
 	const char *dumpencoding = NULL;
+	const char *pgrole = NULL;
 	const char *std_strings;
 	bool		oids = false;
 	TableInfo  *tblinfo;
@@ -258,6 +259,7 @@
 		{no-acl, no_argument, NULL, 'x'},
 		{compress, required_argument, NULL, 'Z'},
 		{encoding, required_argument, NULL, 'E'},
+		{role, required_argument, NULL, 'r'},
 		{help, no_argument, NULL, '?'},
 		{version, no_argument, NULL, 'V'},
 
@@ -302,7 +304,7 @@
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, abcCdDE:f:F:h:in:N:oOp:RsS:t:T:U:vWxX:Z:,
+	while ((c = getopt_long(argc, argv, abcCdDE:f:F:h:in:N:oOp:r:RsS:t:T:U:vWxX:Z:,
 			long_options, optindex)) != -1)
 	{
 		switch (c)
@@ -374,6 +376,10 @@
 pgport = optarg;
 break;
 
+			case 'r':			/* role */
+pgrole = optarg;
+break;
+
 			case 'R':
 /* no-op, still accepted for backwards compatibility */
 break;
@@ -539,6 +545,18 @@
 			exit(1);
 		}
 	}
+	
+	/* Set the role if requested */
+	if (pgrole)
+	{
+		PQExpBuffer roleQry = createPQExpBuffer();
+		appendPQExpBuffer(roleQry, SET ROLE TO %s;\n, fmtId(pgrole));
+		PGresult *res = PQexec(g_conn, roleQry-data);
+	check_sql_result(res, g_conn, roleQry-data, PGRES_COMMAND_OK);
+
+		PQclear(res);
+		destroyPQExpBuffer(roleQry);
+	}
 
 	/*
 	 * Get the active encoding and the standard_conforming_strings setting, so
@@ -771,6 +789,8 @@
 	printf(_(  --use-set-session-authorization\n
 			   use SESSION AUTHORIZATION commands instead of\n
 	  ALTER OWNER commands to set ownership\n));
+	printf(_(  -r, --role  set role before dump\n));
+
 
 	printf(_(\nConnection options:\n));
 	printf(_(  -h, --host=HOSTNAME  database server host or socket directory\n));

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_dump roles support

[Tom Lane]

Stephen Frost [EMAIL PROTECTED] writes:
 Discussing psql options made me recall an annoying problem that we've
 run into.  There's no way (unless it was added to 8.3 and I missed it,
 but I don't think so) to tell pg_dump 'switch to this role before
 doing anything else'.  That's very frustrating when you use no-inherit
 roles for admins.  eg:

 I've looked into using PGOPTIONS to set the role, and it doesn't seem to
 be possible because when we're processing the backend command-line
 options we're not yet in a transaction state, so variable.c:assign_role
 will always come back with NULL and you get:

 vardamir:/home/sfrost PGOPTIONS=-c role=postgres psql -d networx -h 
 vardamir
 psql: FATAL:  invalid value for parameter role: postgres

FWIW, I found by experimentation that ALTER USER ... SET ROLE does work
to cause a SET ROLE at login, though that might be a bit useless for
your purposes --- you'd more or less need a dedicated userid for
pg_dump.

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] pg_dump roles support

[Stephen Frost]

Greetings,

* Stephen Frost ([EMAIL PROTECTED]) wrote:
   Discussing psql options made me recall an annoying problem that we've
   run into.  There's no way (unless it was added to 8.3 and I missed it,
   but I don't think so) to tell pg_dump 'switch to this role before
   doing anything else'.  That's very frustrating when you use no-inherit
   roles for admins.  eg:

I've looked into using PGOPTIONS to set the role, and it doesn't seem to
be possible because when we're processing the backend command-line
options we're not yet in a transaction state, so variable.c:assign_role
will always come back with NULL and you get:

vardamir:/home/sfrost PGOPTIONS=-c role=postgres psql -d networx -h vardamir
psql: FATAL:  invalid value for parameter role: postgres

In current CVS the relevant lines in variable.c are around 868.  That's
my best guess as to what's happening anyway, I havn't had a chance to
actually hook up a debugger and trace it.

As I discuss above, it'd be really nice have a --role or similar option
to ask pg_dump to set role to a particular user before dumping the
database.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] pg_dump roles support

[Stephen Frost]

Greetings,

  Discussing psql options made me recall an annoying problem that we've
  run into.  There's no way (unless it was added to 8.3 and I missed it,
  but I don't think so) to tell pg_dump 'switch to this role before
  doing anything else'.  That's very frustrating when you use no-inherit
  roles for admins.  eg:

  create role admin with noinherit;
  grant postgres to admin;
  grant admin to joesysadmin;

  pg_dump -U joesysadmin mydb;

  Fails because joesysadmin hasn't got rights to everything directly.
  It'd be nice if pg_dump could take a '-r postgres' to 'set role' to
  a role which has the necessary permissions before locking all the
  tables and whatnot.  The same 'set role' would also be included at the
  top of the resulting dump file.  We could have a seperate flag for
  that but I don't think it's necessary.

  Comments?

  I doubt there'd be very much code involved but I'd be willing to write
  a patch if people agree with the general idea/approach.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump roles support

[Tom Lane]

Stephen Frost [EMAIL PROTECTED] writes:
   create role admin with noinherit;
   grant postgres to admin;
   grant admin to joesysadmin;

   pg_dump -U joesysadmin mydb;

   Fails because joesysadmin hasn't got rights to everything directly.

Seems like the correct answer to that is use a saner role
configuration.

regards, tom lane

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] pg_dump roles support

[Stephen Frost]

* Tom Lane ([EMAIL PROTECTED]) wrote:
 Stephen Frost [EMAIL PROTECTED] writes:
create role admin with noinherit;
grant postgres to admin;
grant admin to joesysadmin;
 
pg_dump -U joesysadmin mydb;
 
Fails because joesysadmin hasn't got rights to everything directly.
 
 Seems like the correct answer to that is use a saner role
 configuration.

Funny, it's exactly the type of setup described here:
http://www.postgresql.org/docs/8.2/interactive/role-membership.html

Far as I can tell anyway.  What would you suggest?  The point here is
that joesysadmin shouldn't get full postgres privs on login since most
of the time he won't need them.  When he does need them, he can do a
'set role postgres', do what he needs to do and then 'reset role' when
he's done.  Minimizing the amount of time with superuser privs is a good
thing in general, I would think.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pg_dump roles support

[Tom Lane]

Stephen Frost [EMAIL PROTECTED] writes:
 * Tom Lane ([EMAIL PROTECTED]) wrote:
 Seems like the correct answer to that is use a saner role
 configuration.

 Far as I can tell anyway.  What would you suggest?  The point here is
 that joesysadmin shouldn't get full postgres privs on login since most
 of the time he won't need them.

It's sane to set up a manually-used admin account that way, I agree.
What doesn't follow is that an account configured for manual use should
be used for non-interactive stuff like pg_dump.

regards, tom lane

---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
   choose an index scan if your joining column's datatypes do not
   match

Re: [HACKERS] pg_dump roles support

[Stephen Frost]

* Tom Lane ([EMAIL PROTECTED]) wrote:
 Stephen Frost [EMAIL PROTECTED] writes:
  Far as I can tell anyway.  What would you suggest?  The point here is
  that joesysadmin shouldn't get full postgres privs on login since most
  of the time he won't need them.
 
 It's sane to set up a manually-used admin account that way, I agree.
 What doesn't follow is that an account configured for manual use should
 be used for non-interactive stuff like pg_dump.

I strongly disagree that pg_dump isn't to be used manually, which I
think is what you were getting at.  We use it a great deal to dump
individual schemas and copy them between systems.  We *don't* use it
anymore for full database dumps (something which was done in the past)
because we use PITR instead.  In fact, I encourage my users to use it to
copy schema structures around when they need a seperate area for
development or testing.

What's frustrating is when an admin needs to copy a schema that he
doesn't directly have rights to (another user's schema, or a schema used
by a site or something) he has to login to the database server
(something I'd like to minimize anyway- almost everything can easily be
done from psql), su to root, su to postgres, do the pg_dump on the box,
and then work out copying it off the box.

I suppose I could write my own pg_dump that actually used psql
underneath or add it's functionality to our perl toolkit (it's damn
handy to be able to change permissions on every table in a schema with
one command, and it takes role as an argument, heh), but rewriting the
dependency handling and whatnot isn't something I'm really keen to do.

Thanks,

Stephen


signature.asc
Description: Digital signature