Re: [HACKERS] Use %u to print user mapping's umid and userid

[Tom Lane]

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

[Etsuro Fujita]

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

[Tom Lane]

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

[Robert Haas]

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

[Alvaro Herrera]

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

[Robert Haas]

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

[Etsuro Fujita]

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

[Tom Lane]

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

[Etsuro Fujita]

On 2016/05/11 18:03, Ashutosh Bapat wrote:

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.


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

[Ashutosh Bapat]

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

[Etsuro Fujita]

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

[Ashutosh Bapat]

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

[Etsuro Fujita]

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 

Re: [HACKERS] Use %u to print user mapping's umid and userid

[Etsuro Fujita]

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
> 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 

Re: [HACKERS] Use %u to print user mapping's umid and userid

[Robert Haas]

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
>> > 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

[Etsuro Fujita]

On 2016/03/14 17:56, Ashutosh Bapat wrote:

On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita
> 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

[Kouhei Kaigai]

> -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 <kai...@ak.jp.nec.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] Use %u to print user mapping's umid and userid

[Ashutosh Bapat]

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

[Etsuro Fujita]

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

[Tom Lane]

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

[HACKERS] Use %u to print user mapping's umid and userid

[Etsuro Fujita]

Here is a patch to use %u not %d to print umid and userid.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***
*** 159,165  GetConnection(UserMapping *user, bool will_prep_stmt)
  		entry->have_error = false;
  		entry->conn = connect_pg_server(server, user);
  
! 		elog(DEBUG3, "new postgres_fdw connection %p for server \"%s\" (user mapping oid %d, userid %d)",
  			 entry->conn, server->servername, user->umid, user->userid);
  	}
  
--- 159,165 
  		entry->have_error = false;
  		entry->conn = connect_pg_server(server, user);
  
! 		elog(DEBUG3, "new postgres_fdw connection %p for server \"%s\" (user mapping oid %u, userid %u)",
  			 entry->conn, server->servername, user->umid, user->userid);
  	}
  

-- 
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

[Tom Lane]

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

[Ashutosh Bapat]

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]

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

[Michael Paquier]

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