object description for FDW user mappings
When commit cae565e503 introduced FDW user mappings, it used this in
getObjectDescription for them:
appendStringInfo(&buffer, _("user mapping for %s"), usename);
This was later mostly copied (by yours truly) as object identity by
commit f8348ea32e wherein I used this:
appendStringInfo(&buffer, "%s", usename);
As it turns out, this is wrong, because the pg_user_mapping catalog has
a two-column "primary key" which is user OID and server OID. Therefore
it seems to me that the correct object description and identity must
include both username and server name. I propose we change the above to
this:
appendStringInfo(&buffer, _("user mapping for %s in server %s"), usename,
srv->servername);
I propose to apply the (appropriately rebased) attached patch to all
back branches. Note in particular the wording changes in some error
messages in the foreign_data test.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-fix-user-mapping-object-description-identity.patchtext/x-diff; charset=us-asciiDownload
>From 82e8814c9f4b89d31d51b2fa370add1c04ae0f12 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 5 Mar 2015 12:30:53 -0300
Subject: [PATCH] fix user mapping object description/identity
---
src/backend/catalog/objectaddress.c | 30 ++++++++++++++++++++----------
src/test/regress/expected/foreign_data.out | 24 ++++++++++++------------
2 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 70043fc..0740b4f 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2510,14 +2510,17 @@ getObjectDescription(const ObjectAddress *object)
HeapTuple tup;
Oid useid;
char *usename;
+ Form_pg_user_mapping umform;
+ ForeignServer *srv;
tup = SearchSysCache1(USERMAPPINGOID,
ObjectIdGetDatum(object->objectId));
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for user mapping %u",
object->objectId);
-
- useid = ((Form_pg_user_mapping) GETSTRUCT(tup))->umuser;
+ umform = (Form_pg_user_mapping) GETSTRUCT(tup);
+ useid = umform->umuser;
+ srv = GetForeignServer(umform->umserver);
ReleaseSysCache(tup);
@@ -2526,7 +2529,8 @@ getObjectDescription(const ObjectAddress *object)
else
usename = "public";
- appendStringInfo(&buffer, _("user mapping for %s"), usename);
+ appendStringInfo(&buffer, _("user mapping for %s in server %s"), usename,
+ srv->servername);
break;
}
@@ -3906,19 +3910,18 @@ getObjectIdentityParts(const ObjectAddress *object,
{
HeapTuple tup;
Oid useid;
+ Form_pg_user_mapping umform;
+ ForeignServer *srv;
const char *usename;
- /* no objname support */
- if (objname)
- *objname = NIL;
-
tup = SearchSysCache1(USERMAPPINGOID,
ObjectIdGetDatum(object->objectId));
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for user mapping %u",
object->objectId);
-
- useid = ((Form_pg_user_mapping) GETSTRUCT(tup))->umuser;
+ umform = (Form_pg_user_mapping) GETSTRUCT(tup);
+ useid = umform->umuser;
+ srv = GetForeignServer(umform->umserver);
ReleaseSysCache(tup);
@@ -3927,7 +3930,14 @@ getObjectIdentityParts(const ObjectAddress *object,
else
usename = "public";
- appendStringInfoString(&buffer, usename);
+ if (objname)
+ {
+ *objname = list_make1(pstrdup(usename));
+ *objargs = list_make1(pstrdup(srv->servername));
+ }
+
+ appendStringInfo(&buffer, "%s in server %s", usename,
+ srv->servername);
break;
}
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 632b7e5..4d0de1f 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -247,7 +247,7 @@ CREATE USER MAPPING FOR current_user SERVER s1;
DROP FOREIGN DATA WRAPPER foo; -- ERROR
ERROR: cannot drop foreign-data wrapper foo because other objects depend on it
DETAIL: server s1 depends on foreign-data wrapper foo
-user mapping for foreign_data_user depends on server s1
+user mapping for foreign_data_user in server s1 depends on server s1
HINT: Use DROP ... CASCADE to drop the dependent objects too.
SET ROLE regress_test_role;
DROP FOREIGN DATA WRAPPER foo CASCADE; -- ERROR
@@ -256,7 +256,7 @@ RESET ROLE;
DROP FOREIGN DATA WRAPPER foo CASCADE;
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to server s1
-drop cascades to user mapping for foreign_data_user
+drop cascades to user mapping for foreign_data_user in server s1
\dew+
List of foreign-data wrappers
Name | Owner | Handler | Validator | Access privileges | FDW Options | Description
@@ -526,10 +526,10 @@ CREATE USER MAPPING FOR current_user SERVER s3;
DROP SERVER s3; -- ERROR
ERROR: cannot drop server s3 because other objects depend on it
-DETAIL: user mapping for foreign_data_user depends on server s3
+DETAIL: user mapping for foreign_data_user in server s3 depends on server s3
HINT: Use DROP ... CASCADE to drop the dependent objects too.
DROP SERVER s3 CASCADE;
-NOTICE: drop cascades to user mapping for foreign_data_user
+NOTICE: drop cascades to user mapping for foreign_data_user in server s3
\des
List of foreign servers
Name | Owner | Foreign-data wrapper
@@ -1183,8 +1183,8 @@ GRANT USAGE ON FOREIGN SERVER s9 TO regress_test_role;
CREATE USER MAPPING FOR current_user SERVER s9;
DROP SERVER s9 CASCADE;
NOTICE: drop cascades to 2 other objects
-DETAIL: drop cascades to user mapping for public
-drop cascades to user mapping for unprivileged_role
+DETAIL: drop cascades to user mapping for public in server s9
+drop cascades to user mapping for unprivileged_role in server s9
RESET ROLE;
CREATE SERVER s9 FOREIGN DATA WRAPPER foo;
GRANT USAGE ON FOREIGN SERVER s9 TO unprivileged_role;
@@ -1256,14 +1256,14 @@ DROP ROLE regress_test_role; -- ERROR
ERROR: role "regress_test_role" cannot be dropped because some objects depend on it
DETAIL: privileges for server s4
privileges for foreign-data wrapper foo
-owner of user mapping for regress_test_role
-owner of user mapping for regress_test_role
+owner of user mapping for regress_test_role in server s6
+owner of user mapping for regress_test_role in server s5
owner of server s5
owner of server t2
DROP SERVER s5 CASCADE;
-NOTICE: drop cascades to user mapping for regress_test_role
+NOTICE: drop cascades to user mapping for regress_test_role in server s5
DROP SERVER t1 CASCADE;
-NOTICE: drop cascades to user mapping for public
+NOTICE: drop cascades to user mapping for public in server t1
DROP SERVER t2;
DROP USER MAPPING FOR regress_test_role SERVER s6;
-- This test causes some order dependent cascade detail output,
@@ -1274,8 +1274,8 @@ NOTICE: drop cascades to 5 other objects
\set VERBOSITY default
DROP SERVER s8 CASCADE;
NOTICE: drop cascades to 2 other objects
-DETAIL: drop cascades to user mapping for foreign_data_user
-drop cascades to user mapping for public
+DETAIL: drop cascades to user mapping for foreign_data_user in server s8
+drop cascades to user mapping for public in server s8
DROP ROLE regress_test_indirect;
DROP ROLE regress_test_role;
DROP ROLE unprivileged_role; -- ERROR
--
2.1.4
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
When commit cae565e503 introduced FDW user mappings, it used this in
getObjectDescription for them:
appendStringInfo(&buffer, _("user mapping for %s"), usename);
This was later mostly copied (by yours truly) as object identity by
commit f8348ea32e wherein I used this:
appendStringInfo(&buffer, "%s", usename);
As it turns out, this is wrong, because the pg_user_mapping catalog has
a two-column "primary key" which is user OID and server OID. Therefore
it seems to me that the correct object description and identity must
include both username and server name. I propose we change the above to
this:
appendStringInfo(&buffer, _("user mapping for %s in server %s"), usename,
srv->servername);
+1 for the concept, but to be nitpicky, "in" doesn't seem like the right
word here. "on server" would read better to me; or perhaps "at server".
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
On 06-03-2015 AM 01:32, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
appendStringInfo(&buffer, _("user mapping for %s in server %s"), usename,
srv->servername);+1 for the concept, but to be nitpicky, "in" doesn't seem like the right
word here. "on server" would read better to me; or perhaps "at server".
One more option may be "for server" (reading the doc for CREATE USER MAPPING)
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06-03-2015 AM 09:18, Amit Langote wrote:
On 06-03-2015 AM 01:32, Tom Lane wrote:
+1 for the concept, but to be nitpicky, "in" doesn't seem like the right
word here. "on server" would read better to me; or perhaps "at server".One more option may be "for server" (reading the doc for CREATE USER MAPPING)
Oh, I see it's been done that way already.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
On 06-03-2015 AM 01:32, Tom Lane wrote:
+1 for the concept, but to be nitpicky, "in" doesn't seem like the right
word here. "on server" would read better to me; or perhaps "at server".
One more option may be "for server" (reading the doc for CREATE USER MAPPING)
Hm, but then you'd have "user mapping for foo for server bar", which
doesn't read so nicely either.
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
On 06-03-2015 AM 09:30, Tom Lane wrote:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
One more option may be "for server" (reading the doc for CREATE USER MAPPING)
Hm, but then you'd have "user mapping for foo for server bar", which
doesn't read so nicely either.
Yeah, I had totally missed the "for foo" part; I was thinking of it as "of foo".
By the way, in this case, is "foo" the name/id of a local user or does it
really refer to some "foo on the remote server"?
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
By the way, in this case, is "foo" the name/id of a local user or does it
really refer to some "foo on the remote server"?
It's the name of a local user. I see your point that somebody might
misread this as suggesting that it's a remote username, but not sure
that there's anything great we can do here to disambiguate that.
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
Tom Lane-2 wrote
Amit Langote <
Langote_Amit_f8@.co
> writes:
By the way, in this case, is "foo" the name/id of a local user or does it
really refer to some "foo on the remote server"?It's the name of a local user. I see your point that somebody might
misread this as suggesting that it's a remote username, but not sure
that there's anything great we can do here to disambiguate that.
Yeah, clarifying that point seems to add verbosity for little gain.
On the message aspect is it against style to use composite notation in a
case like this?
"user mapping (%s, %s)"
It is basically a single, if compound, noun so adding in/at/to doesn't feel
right...
David J.
--
View this message in context: http://postgresql.nabble.com/object-description-for-FDW-user-mappings-tp5840655p5840729.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers