Re: [HACKERS] Use %u to print user mapping's umid and userid
Etsuro Fujita writes: > On 2016/05/13 3:53, Tom Lane wrote: >> Robert Haas writes: >>> Regardless of what approach we take, I disagree that this needs to be >>> fixed in 9.6. >> Agreed. This is only a cosmetic issue, and it's only going to be visible >> to a very small group of people, so we can leave it alone until 9.7. > Agreed. This thread is shown as "Needs review" in the 2016-09 commitfest, but so far as I can find, no new patch has been posted since Robert proposed that we ought to get rid of the current List format for the extra info in favor of using ExtensibleNode. I'm going to mark the CF entry as Returned With Feedback. 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] Use %u to print user mapping's umid and userid
On 2016/05/13 3:53, Tom Lane wrote: Robert Haas writes: Regardless of what approach we take, I disagree that this needs to be fixed in 9.6. Agreed. This is only a cosmetic issue, and it's only going to be visible to a very small group of people, so we can leave it alone until 9.7. Agreed. (FWIW, now that we've put in the list_make5 macros, I'd vote against taking them out, independently of what happens in postgres_fdw. Somebody else will need them someday, or indeed might already be using them in some non-core extension.) Agreed. Best regards, Etsuro Fujita -- 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] Use %u to print user mapping's umid and userid
Robert Haas writes: > My suggestion is that we switch from using a List to marshal the data > to using an ExtensibleNode. An advantage of that is that we'd have > some in-core test coverage for the ExtensibleNode stuff. In theory it > ought to be simpler and less messy, too, but I guess we'll find out. Seems like a good idea, or at least one worth trying. > Regardless of what approach we take, I disagree that this needs to be > fixed in 9.6. Agreed. This is only a cosmetic issue, and it's only going to be visible to a very small group of people, so we can leave it alone until 9.7. (FWIW, now that we've put in the list_make5 macros, I'd vote against taking them out, independently of what happens in postgres_fdw. Somebody else will need them someday, or indeed might already be using them in some non-core extension.) 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] Use %u to print user mapping's umid and userid
On Thu, May 12, 2016 at 2:29 PM, Alvaro Herrera wrote: > Robert Haas wrote: >> My suggestion is that we switch from using a List to marshal the data >> to using an ExtensibleNode. An advantage of that is that we'd have >> some in-core test coverage for the ExtensibleNode stuff. In theory it >> ought to be simpler and less messy, too, but I guess we'll find out. > > So the data in the list has a certain specific meaning according to its > position within the list? And the enum being modified by this patch, > corresponds to knowledge of what each element in the list is? Right. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Use %u to print user mapping's umid and userid
Robert Haas wrote: > My suggestion is that we switch from using a List to marshal the data > to using an ExtensibleNode. An advantage of that is that we'd have > some in-core test coverage for the ExtensibleNode stuff. In theory it > ought to be simpler and less messy, too, but I guess we'll find out. So the data in the list has a certain specific meaning according to its position within the list? And the enum being modified by this patch, corresponds to knowledge of what each element in the list is? This seems a bit odd. I agree that something more similar to a struct would be more appropriate. Maybe there are other ways, but ExtensibleNode seems like a reasonable tool to use here. +1 to having in-core use case for ExtensibleNode too. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] Use %u to print user mapping's umid and userid
On Thu, May 12, 2016 at 12:18 AM, Etsuro Fujita wrote: > I think if scanning a foreign join, the user mapping is still valid at > execution, and that is ensured by RevalidateChachedQuery, IIUC. Yes, we added special machinery for that, along the lines of what is also done for RLS. But I have to say I don't like this patch very much. The problem here is that we want to store an unsigned integer in a node tree, but makeInteger() takes a long, and there's no other similar node that accepts an OID instead. Your solution to that problem is not to store the data at all, which seems like letting the tail wag the dog. Maybe the performance difference in this case is minor and maybe it's not, but that isn't really the point. The point is that storing an OID is a pretty reasonable thing to want to do, and we should find a way to do it instead of working around the problem. My suggestion is that we switch from using a List to marshal the data to using an ExtensibleNode. An advantage of that is that we'd have some in-core test coverage for the ExtensibleNode stuff. In theory it ought to be simpler and less messy, too, but I guess we'll find out. Regardless of what approach we take, I disagree that this needs to be fixed in 9.6. The only people who are going to be hurt by this are people who are using postgres_fdw and reading node-tree dumps and have an OID counter that advances past 2 billion. And those people are going to be pretty rare, and they'll just have to live with it. It's more important to keep the code stable than to fix a minor issue like this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Use %u to print user mapping's umid and userid
On 2016/05/12 13:02, Tom Lane wrote: Etsuro Fujita writes: On 2016/05/11 18:03, Ashutosh Bapat wrote: A call to GetForeignTable would incur a catalog lookup which means a catalog table/index scan if corresponding entry is not in the cache. This is followed by GetUserMapping() which is another catalog access. That's bound to be expensive than an makeOid(), oidVal() call. Right, but such lookups have been incurred at the planning time (ie, build_simple_rel), and corresponding entries would be in the cache. So, the overhead in that recalculation at the execution time would be not that large in practice. No? It's a mistake to assume that execution immediately follows planning. Yeah, that would not be the case in PREPARE/EXECUTE, right? Having said that, I wonder whether you should be thinking less about performance and more about correctness. Is a user mapping lookup done at plan time still valid at execution, and if so what ensures that? I think if scanning a foreign join, the user mapping is still valid at execution, and that is ensured by RevalidateChachedQuery, IIUC. Best regards, Etsuro Fujita -- 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] Use %u to print user mapping's umid and userid
Etsuro Fujita writes: > On 2016/05/11 18:03, Ashutosh Bapat wrote: >> A call to GetForeignTable would incur a catalog lookup which means a >> catalog table/index scan if corresponding entry is not in the cache. >> This is followed by GetUserMapping() which is another catalog access. >> That's bound to be expensive than an makeOid(), oidVal() call. > Right, but such lookups have been incurred at the planning time (ie, > build_simple_rel), and corresponding entries would be in the cache. So, > the overhead in that recalculation at the execution time would be not > that large in practice. No? It's a mistake to assume that execution immediately follows planning. Having said that, I wonder whether you should be thinking less about performance and more about correctness. Is a user mapping lookup done at plan time still valid at execution, and if so what ensures 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
Re: [HACKERS] Use %u to print user mapping's umid and userid
On 2016/05/11 18:03, Ashutosh Bapat wrote: On Wed, May 11, 2016 at 1:34 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: On 2016/05/11 16:49, Ashutosh Bapat wrote: The patch is calculating user mapping when it's readily available through RelOptInfo::fdw_private. That incurs a catalog lookup unnecessarily. Instead, can we add new function makeOid, oidVal on the lines of makeInteger and intVal to store and retrieve an OID resp. and also corresponding print function? It might be helpful in future. That might be an idea, but is the overhead in that re-calculation so large? A call to GetForeignTable would incur a catalog lookup which means a catalog table/index scan if corresponding entry is not in the cache. This is followed by GetUserMapping() which is another catalog access. That's bound to be expensive than an makeOid(), oidVal() call. Right, but such lookups have been incurred at the planning time (ie, build_simple_rel), and corresponding entries would be in the cache. So, the overhead in that recalculation at the execution time would be not that large in practice. No? Best regards, Etsuro Fujita -- 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] Use %u to print user mapping's umid and userid
On Wed, May 11, 2016 at 1:34 PM, Etsuro Fujita wrote: > On 2016/05/11 16:49, Ashutosh Bapat wrote: > >> The patch is calculating user mapping when it's readily available >> through RelOptInfo::fdw_private. That incurs a catalog lookup >> unnecessarily. Instead, can we add new function makeOid, oidVal on the >> lines of makeInteger and intVal to store and retrieve an OID resp. and >> also corresponding print function? It might be helpful in future. >> > > That might be an idea, but is the overhead in that re-calculation so large? > > A call to GetForeignTable would incur a catalog lookup which means a catalog table/index scan if corresponding entry is not in the cache. This is followed by GetUserMapping() which is another catalog access. That's bound to be expensive than an makeOid(), oidVal() call. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] Use %u to print user mapping's umid and userid
On 2016/05/11 16:49, Ashutosh Bapat wrote: The patch is calculating user mapping when it's readily available through RelOptInfo::fdw_private. That incurs a catalog lookup unnecessarily. Instead, can we add new function makeOid, oidVal on the lines of makeInteger and intVal to store and retrieve an OID resp. and also corresponding print function? It might be helpful in future. That might be an idea, but is the overhead in that re-calculation so large? Best regards, Etsuro Fujita -- 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] Use %u to print user mapping's umid and userid
On Wed, May 11, 2016 at 1:10 PM, Etsuro Fujita wrote: > On 2016/05/10 16:56, Etsuro Fujita wrote: > >> Here is a patch to fix this. >> > > I found that the previous patch handles the ForeignScan's fs_relids > Bitmapset destructively. Also, I noticed that I removed some existing > comments inadvertently. So, I'm attaching the updated patch to fix those > things. I'll add this to the next CF. I think this should be addressed in > advance of the release of 9.6, though. > > The patch is calculating user mapping when it's readily available through RelOptInfo::fdw_private. That incurs a catalog lookup unnecessarily. Instead, can we add new function makeOid, oidVal on the lines of makeInteger and intVal to store and retrieve an OID resp. and also corresponding print function? It might be helpful in future. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] Use %u to print user mapping's umid and userid
On 2016/05/10 16:56, Etsuro Fujita wrote: Here is a patch to fix this. I found that the previous patch handles the ForeignScan's fs_relids Bitmapset destructively. Also, I noticed that I removed some existing comments inadvertently. So, I'm attaching the updated patch to fix those things. I'll add this to the next CF. I think this should be addressed in advance of the release of 9.6, though. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/postgres_fdw.c --- b/contrib/postgres_fdw/postgres_fdw.c *** *** 67,74 enum FdwScanPrivateIndex FdwScanPrivateRetrievedAttrs, /* Integer representing the desired fetch_size */ FdwScanPrivateFetchSize, - /* Oid of user mapping to be used while connecting to the foreign server */ - FdwScanPrivateUserMappingOid, /* * String describing join i.e. names of relations being joined and types --- 67,72 *** *** 1198,1208 postgresGetForeignPlan(PlannerInfo *root, * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ ! fdw_private = list_make5(makeString(sql.data), remote_conds, retrieved_attrs, ! makeInteger(fpinfo->fetch_size), ! makeInteger(foreignrel->umid)); if (foreignrel->reloptkind == RELOPT_JOINREL) fdw_private = lappend(fdw_private, makeString(fpinfo->relation_name->data)); --- 1196,1205 * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ ! fdw_private = list_make4(makeString(sql.data), remote_conds, retrieved_attrs, ! makeInteger(fpinfo->fetch_size)); if (foreignrel->reloptkind == RELOPT_JOINREL) fdw_private = lappend(fdw_private, makeString(fpinfo->relation_name->data)); *** *** 1234,1240 postgresBeginForeignScan(ForeignScanState *node, int eflags) --- 1231,1241 ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan; EState *estate = node->ss.ps.state; PgFdwScanState *fsstate; + RangeTblEntry *rte; + Oid userid; + ForeignTable *table; UserMapping *user; + int rtindex; int numParams; /* *** *** 1256,1285 postgresBeginForeignScan(ForeignScanState *node, int eflags) * planning to ensure that the join is safe to pushdown. In case the * information goes stale between planning and execution, plan will be * invalidated and replanned. */ if (fsplan->scan.scanrelid > 0) { - ForeignTable *table; - /* ! * Identify which user to do the remote access as. This should match ! * what ExecCheckRTEPerms() does. */ ! RangeTblEntry *rte = rt_fetch(fsplan->scan.scanrelid, estate->es_range_table); ! Oid userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); ! ! fsstate->rel = node->ss.ss_currentRelation; ! table = GetForeignTable(RelationGetRelid(fsstate->rel)); ! ! user = GetUserMapping(userid, table->serverid); } ! else ! { ! Oid umid = intVal(list_nth(fsplan->fdw_private, FdwScanPrivateUserMappingOid)); ! user = GetUserMappingById(umid); ! Assert(fsplan->fs_server == user->serverid); ! } /* * Get connection to the foreign server. Connection manager will --- 1257,1283 * planning to ensure that the join is safe to pushdown. In case the * information goes stale between planning and execution, plan will be * invalidated and replanned. + * + * This should match what ExecCheckRTEPerms() does. */ if (fsplan->scan.scanrelid > 0) + rtindex = fsplan->scan.scanrelid; + else { /* ! * It is ensured that foreign tables appearing in a foreign join ! * belong to the same server and use the same user mapping, so pick ! * the lowest-numbered one as a representative. */ ! rtindex = -1; ! rtindex = bms_next_member(fsplan->fs_relids, rtindex); ! Assert(rtindex > 0); } ! rte = rt_fetch(rtindex, estate->es_range_table); ! userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); ! table = GetForeignTable(rte->relid); ! user = GetUserMapping(userid, table->serverid); /* * Get connection to the foreign server. Connection manager will *** *** 1316,1324 postgresBeginForeignScan(ForeignScanState *node, int eflags) --- 1314,1328 * into local representation and error reporting during that process. */ if (fsplan->scan.scanrelid > 0) + { + fsstate->rel = node->ss.ss_currentRelation; fsstate->tupdesc = RelationGetDescr(fsstate->rel); + } else + { + fsstate->rel = NULL; fsstate->tupdesc = node->ss.ss_ScanTupleSlot->tts_tupleDescriptor; + } fsstate->attinmeta = TupleDescGetAttInMetadata(fsstate->tupdesc); *** a/src/include/nodes/pg_list.h --- b/src/include/nodes/pg_list.h *** *** 134,152 list_length(const List *l) #define list_make2(x1
Re: [HACKERS] Use %u to print user mapping's umid and userid
On 2016/05/02 22:06, Robert Haas wrote: On Thu, Apr 28, 2016 at 7:59 AM, Etsuro Fujita wrote: On 2016/03/14 17:56, Ashutosh Bapat wrote: On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: /* * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ fdw_private = list_make4(makeString(sql.data), retrieved_attrs, makeInteger(fpinfo->fetch_size), makeInteger(foreignrel->umid)); I don't think it's correct to use makeInteger for the foreignrel's umid. As long as we are using makeInteger() and inVal() pair to set and extract the values, it should be fine. Yeah, but my concern about this is eg, print plan if debugging (ie, debug_print_plan=on); the umid OID will be printed with the %ld specifier, so in some platform, the OID might be printed wrongly. Maybe I'm missing something, though. That seems like a legitimate, if minor, complaint. Here is a patch to fix this. That is basically the same as in [1], but I rebased the patch against HEAD and removed list_make5 and its friends, which were added just for the postgres_fdw DML pushdown. Sorry for the delay. I was on vacation. Best regards, Etsuro Fujita [1] http://www.postgresql.org/message-id/56e66f61.3070...@lab.ntt.co.jp *** a/contrib/postgres_fdw/postgres_fdw.c --- b/contrib/postgres_fdw/postgres_fdw.c *** *** 67,74 enum FdwScanPrivateIndex FdwScanPrivateRetrievedAttrs, /* Integer representing the desired fetch_size */ FdwScanPrivateFetchSize, - /* Oid of user mapping to be used while connecting to the foreign server */ - FdwScanPrivateUserMappingOid, /* * String describing join i.e. names of relations being joined and types --- 67,72 *** *** 1198,1208 postgresGetForeignPlan(PlannerInfo *root, * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ ! fdw_private = list_make5(makeString(sql.data), remote_conds, retrieved_attrs, ! makeInteger(fpinfo->fetch_size), ! makeInteger(foreignrel->umid)); if (foreignrel->reloptkind == RELOPT_JOINREL) fdw_private = lappend(fdw_private, makeString(fpinfo->relation_name->data)); --- 1196,1205 * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ ! fdw_private = list_make4(makeString(sql.data), remote_conds, retrieved_attrs, ! makeInteger(fpinfo->fetch_size)); if (foreignrel->reloptkind == RELOPT_JOINREL) fdw_private = lappend(fdw_private, makeString(fpinfo->relation_name->data)); *** *** 1234,1240 postgresBeginForeignScan(ForeignScanState *node, int eflags) --- 1231,1241 ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan; EState *estate = node->ss.ps.state; PgFdwScanState *fsstate; + RangeTblEntry *rte; + Oid userid; + ForeignTable *table; UserMapping *user; + int rtindex; int numParams; /* *** *** 1250,1285 postgresBeginForeignScan(ForeignScanState *node, int eflags) node->fdw_state = (void *) fsstate; /* ! * Obtain the foreign server where to connect and user mapping to use for ! * connection. For base relations we obtain this information from ! * catalogs. For join relations, this information is frozen at the time of ! * planning to ensure that the join is safe to pushdown. In case the ! * information goes stale between planning and execution, plan will be ! * invalidated and replanned. */ if (fsplan->scan.scanrelid > 0) ! { ! ForeignTable *table; ! ! /* ! * Identify which user to do the remote access as. This should match ! * what ExecCheckRTEPerms() does. ! */ ! RangeTblEntry *rte = rt_fetch(fsplan->scan.scanrelid, estate->es_range_table); ! Oid userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); ! ! fsstate->rel = node->ss.ss_currentRelation; ! table = GetForeignTable(RelationGetRelid(fsstate->rel)); ! ! user = GetUserMapping(userid, table->serverid); ! } else ! { ! Oid umid = intVal(list_nth(fsplan->fdw_private, FdwScanPrivateUserMappingOid)); ! user = GetUserMappingById(umid); ! Assert(fsplan->fs_server == user->serverid); ! } /* * Get connection to the foreign server. Connection manager will --- 1251,1274 node->fdw_state = (void *) fsstate; /* ! * Get the user mapping. This should match what ExecCheckRTEPerms() does. ! * ! * If scanning a foreign join, the planner ensured that joined relations ! * are foreign tables belonging to the same server and using the same ! * user m
Re: [HACKERS] Use %u to print user mapping's umid and userid
On Thu, Apr 28, 2016 at 7:59 AM, Etsuro Fujita wrote: > On 2016/03/14 17:56, Ashutosh Bapat wrote: >> On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita >> mailto:fujita.ets...@lab.ntt.co.jp>> wrote: > >> /* >> * Build the fdw_private list that will be available to the >> executor. >> * Items in the list must match order in enum >> FdwScanPrivateIndex. >> */ >> fdw_private = list_make4(makeString(sql.data), >> retrieved_attrs, >> makeInteger(fpinfo->fetch_size), >> makeInteger(foreignrel->umid)); >> >> I don't think it's correct to use makeInteger for the foreignrel's >> umid. > > >> As long as we are using makeInteger() and inVal() pair to set and >> extract the values, it should be fine. > > Yeah, but my concern about this is eg, print plan if debugging (ie, > debug_print_plan=on); the umid OID will be printed with the %ld specifier, > so in some platform, the OID might be printed wrongly. Maybe I'm missing > something, though. That seems like a legitimate, if minor, complaint. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Use %u to print user mapping's umid and userid
On 2016/03/14 17:56, Ashutosh Bapat wrote: On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: /* * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ fdw_private = list_make4(makeString(sql.data), retrieved_attrs, makeInteger(fpinfo->fetch_size), makeInteger(foreignrel->umid)); I don't think it's correct to use makeInteger for the foreignrel's umid. As long as we are using makeInteger() and inVal() pair to set and extract the values, it should be fine. Yeah, but my concern about this is eg, print plan if debugging (ie, debug_print_plan=on); the umid OID will be printed with the %ld specifier, so in some platform, the OID might be printed wrongly. Maybe I'm missing something, though. Sorry for the long delay. Best regards, Etsuro Fujita -- 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] Use %u to print user mapping's umid and userid
> -Original Message- > From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Etsuro Fujita > Sent: Monday, March 14, 2016 4:59 PM > To: Ashutosh Bapat; Tom Lane > Cc: pgsql-hackers > Subject: Re: [HACKERS] Use %u to print user mapping's umid and userid > > Hi, > > On 2016/02/09 14:09, Ashutosh Bapat wrote: > > Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid), > > which is returned as is in UserMapping object. I confused InvalidOid > > with -1. > > I think the following umid handling in postgresGetForeignPlan has the > same issue: > > /* > * Build the fdw_private list that will be available to the executor. > * Items in the list must match order in enum FdwScanPrivateIndex. > */ > fdw_private = list_make4(makeString(sql.data), > retrieved_attrs, > makeInteger(fpinfo->fetch_size), > makeInteger(foreignrel->umid)); > > I don't think it's correct to use makeInteger for the foreignrel's umid. > BTW, use of ExtensibleNode allows to forget problems come from data format translation. -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei -- 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] Use %u to print user mapping's umid and userid
On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita wrote: > Hi, > > On 2016/02/09 14:09, Ashutosh Bapat wrote: > >> Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid), >> which is returned as is in UserMapping object. I confused InvalidOid >> with -1. >> > > I think the following umid handling in postgresGetForeignPlan has the same > issue: > > /* > * Build the fdw_private list that will be available to the executor. > * Items in the list must match order in enum FdwScanPrivateIndex. > */ > fdw_private = list_make4(makeString(sql.data), > retrieved_attrs, > makeInteger(fpinfo->fetch_size), > makeInteger(foreignrel->umid)); > > I don't think it's correct to use makeInteger for the foreignrel's umid. > As long as we are using makeInteger() and inVal() pair to set and extract the values, it should be fine. > > You store the umid in the fdw_private list here and extract it from the > list in postgresBeginForeignScan, to get the user mapping. But we really > need that? We have a validated plan when getting called from > postgresBeginForeignScan, so if foreign join, we can simply pick any of the > plan's fs_relids and use it to identify which user to do the remote access > as, in the same way as for foreign tables. > We have done that calculation ones while creating the plan, why do we want to do that again? For a base relation, the user mapping needs to be found out at the time of execution, since it could change between plan creation and execution. But for a join plan invalidation takes care of this change. > Attached is a patch for that. > > Best regards, > Etsuro Fujita > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] Use %u to print user mapping's umid and userid
Hi, On 2016/02/09 14:09, Ashutosh Bapat wrote: Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid), which is returned as is in UserMapping object. I confused InvalidOid with -1. I think the following umid handling in postgresGetForeignPlan has the same issue: /* * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ fdw_private = list_make4(makeString(sql.data), retrieved_attrs, makeInteger(fpinfo->fetch_size), makeInteger(foreignrel->umid)); I don't think it's correct to use makeInteger for the foreignrel's umid. You store the umid in the fdw_private list here and extract it from the list in postgresBeginForeignScan, to get the user mapping. But we really need that? We have a validated plan when getting called from postgresBeginForeignScan, so if foreign join, we can simply pick any of the plan's fs_relids and use it to identify which user to do the remote access as, in the same way as for foreign tables. Attached is a patch for that. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/postgres_fdw.c --- b/contrib/postgres_fdw/postgres_fdw.c *** *** 65,72 enum FdwScanPrivateIndex FdwScanPrivateRetrievedAttrs, /* Integer representing the desired fetch_size */ FdwScanPrivateFetchSize, - /* Oid of user mapping to be used while connecting to the foreign server */ - FdwScanPrivateUserMappingOid, /* * String describing join i.e. names of relations being joined and types --- 65,70 *** *** 1133,1142 postgresGetForeignPlan(PlannerInfo *root, * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ ! fdw_private = list_make4(makeString(sql.data), retrieved_attrs, ! makeInteger(fpinfo->fetch_size), ! makeInteger(foreignrel->umid)); if (foreignrel->reloptkind == RELOPT_JOINREL) fdw_private = lappend(fdw_private, makeString(fpinfo->relation_name->data)); --- 1131,1139 * Build the fdw_private list that will be available to the executor. * Items in the list must match order in enum FdwScanPrivateIndex. */ ! fdw_private = list_make3(makeString(sql.data), retrieved_attrs, ! makeInteger(fpinfo->fetch_size)); if (foreignrel->reloptkind == RELOPT_JOINREL) fdw_private = lappend(fdw_private, makeString(fpinfo->relation_name->data)); *** *** 1168,1174 postgresBeginForeignScan(ForeignScanState *node, int eflags) --- 1165,1175 ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan; EState *estate = node->ss.ps.state; PgFdwScanState *fsstate; + RangeTblEntry *rte; + Oid userid; + ForeignTable *table; UserMapping *user; + int rtindex; int numParams; int i; ListCell *lc; *** *** 1193,1221 postgresBeginForeignScan(ForeignScanState *node, int eflags) * information goes stale between planning and execution, plan will be * invalidated and replanned. */ - if (fsplan->scan.scanrelid > 0) - { - ForeignTable *table; ! /* ! * Identify which user to do the remote access as. This should match ! * what ExecCheckRTEPerms() does. ! */ ! RangeTblEntry *rte = rt_fetch(fsplan->scan.scanrelid, estate->es_range_table); ! Oid userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); ! ! fsstate->rel = node->ss.ss_currentRelation; ! table = GetForeignTable(RelationGetRelid(fsstate->rel)); ! ! user = GetUserMapping(userid, table->serverid); ! } else { ! Oid umid = intVal(list_nth(fsplan->fdw_private, FdwScanPrivateUserMappingOid)); ! ! user = GetUserMappingById(umid); ! Assert(fsplan->fs_server == user->serverid); } /* * Get connection to the foreign server. Connection manager will --- 1194,1217 * information goes stale between planning and execution, plan will be * invalidated and replanned. */ ! /* ! * Identify which user to do the remote access as. This should match what ! * ExecCheckRTEPerms() does. ! */ ! if (fsplan->scan.scanrelid > 0) ! rtindex = fsplan->scan.scanrelid; else { ! /* Pick the lowest-numbered one as a representative */ ! rtindex = bms_first_member(fsplan->fs_relids); } + rte = rt_fetch(rtindex, estate->es_range_table); + userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + + /* Get info about foreign table */ + table = GetForeignTable(rte->relid); + user = GetUserMapping(userid, table->serverid); /* * Get connection to the foreign server. Connection manager will *** *** 1252,1260 postgresBeginForeignScan(ForeignScanState *node, int eflags) --- 1248,1262 * into local representation and error reporting during that
Re: [HACKERS] Use %u to print user mapping's umid and userid
Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid), which is returned as is in UserMapping object. I confused InvalidOid with -1. Sorry for the confusion. On Tue, Feb 9, 2016 at 10:21 AM, Tom Lane wrote: > Ashutosh Bapat writes: > > Sorry to come to this late. > > The userid being printed is from UserMapping. The new API > > GetUserMappingById() allows an FDW to get user mapping by its OID. This > API > > is intended to be used by FDWs to fetch the user mapping inferred by the > > core for given join between foreign relations. In such user mapping > object > > , userid may be -1 for a public user mapping. > > If that is actually how it works, it's broken and I'm going to insist > on a redesign. There is nothing anywhere that says that 0x > is not a valid OID. > > regards, tom lane > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] Use %u to print user mapping's umid and userid
Ashutosh Bapat writes: > Sorry to come to this late. > The userid being printed is from UserMapping. The new API > GetUserMappingById() allows an FDW to get user mapping by its OID. This API > is intended to be used by FDWs to fetch the user mapping inferred by the > core for given join between foreign relations. In such user mapping object > , userid may be -1 for a public user mapping. If that is actually how it works, it's broken and I'm going to insist on a redesign. There is nothing anywhere that says that 0x is not a valid OID. 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] Use %u to print user mapping's umid and userid
On Tue, Feb 9, 2016 at 1:22 PM, Ashutosh Bapat wrote: > The userid being printed is from UserMapping. The new API > GetUserMappingById() allows an FDW to get user mapping by its OID. This API > is intended to be used by FDWs to fetch the user mapping inferred by the > core for given join between foreign relations. In such user mapping object , > userid may be -1 for a public user mapping. I am a bit surprised by this sentence, UserMapping->userid is an Oid, and those are unsigned. Could you clarify? -- Michael -- 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] Use %u to print user mapping's umid and userid
Sorry to come to this late. The userid being printed is from UserMapping. The new API GetUserMappingById() allows an FDW to get user mapping by its OID. This API is intended to be used by FDWs to fetch the user mapping inferred by the core for given join between foreign relations. In such user mapping object , userid may be -1 for a public user mapping. I think using %u for -1 will print it as largest integer. Would that create confusion for users? On Mon, Feb 8, 2016 at 9:37 PM, Tom Lane wrote: > Etsuro Fujita writes: > > Here is a patch to use %u not %d to print umid and userid. > > Pushed, thanks. > > 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 > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] Use %u to print user mapping's umid and userid
Etsuro Fujita writes: > Here is a patch to use %u not %d to print umid and userid. Pushed, thanks. 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