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