Use %u to print user mapping's umid and userid
Here is a patch to use %u not %d to print umid and userid.
Best regards,
Etsuro Fujita
Attachments:
GetConnection.patchapplication/x-patch; name=GetConnection.patchDownload
*** 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);
}
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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
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 <tgl@sss.pgh.pa.us> wrote:
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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
On Tue, Feb 9, 2016 at 1:22 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> 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 0xffffffff
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
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 <tgl@sss.pgh.pa.us> wrote:
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> 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. ThisAPI
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 mappingobject
, 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 0xffffffff
is not a valid OID.regards, tom lane
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
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
Attachments:
postgres-fdw-umid.patchapplication/x-patch; name=postgres-fdw-umid.patchDownload
*** 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 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);
On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
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
-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@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 useridHi,
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 <kaigai@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
On 2016/03/14 17:56, Ashutosh Bapat wrote:
On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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
On Thu, Apr 28, 2016 at 7:59 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
On 2016/03/14 17:56, Ashutosh Bapat wrote:
On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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
On 2016/05/02 22:06, Robert Haas wrote:
On Thu, Apr 28, 2016 at 7:59 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:On 2016/03/14 17:56, Ashutosh Bapat wrote:
On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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]/messages/by-id/56E66F61.3070201@lab.ntt.co.jp, 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
Attachments:
postgres-fdw-umid-v2.patchtext/x-diff; name=postgres-fdw-umid-v2.patchDownload
*** 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 mapping, so pick the lowest-numbered one as a representative.
! * Note that in that case the current user is the same as the one for
! * which the plan was created (see RevalidateCachedQuery), so this will
! * derive the same user to do the remote access as as in build_simple_rel.
*/
if (fsplan->scan.scanrelid > 0)
! rtindex = fsplan->scan.scanrelid;
else
! rtindex = bms_first_member(fsplan->fs_relids);
! 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)
--- 1305,1319 ----
* 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,x2) lcons(x1, list_make1(x2))
#define list_make3(x1,x2,x3) lcons(x1, list_make2(x2, x3))
#define list_make4(x1,x2,x3,x4) lcons(x1, list_make3(x2, x3, x4))
- #define list_make5(x1,x2,x3,x4,x5) lcons(x1, list_make4(x2, x3, x4, x5))
#define list_make1_int(x1) lcons_int(x1, NIL)
#define list_make2_int(x1,x2) lcons_int(x1, list_make1_int(x2))
#define list_make3_int(x1,x2,x3) lcons_int(x1, list_make2_int(x2, x3))
#define list_make4_int(x1,x2,x3,x4) lcons_int(x1, list_make3_int(x2, x3, x4))
- #define list_make5_int(x1,x2,x3,x4,x5) lcons_int(x1, list_make4_int(x2, x3, x4, x5))
#define list_make1_oid(x1) lcons_oid(x1, NIL)
#define list_make2_oid(x1,x2) lcons_oid(x1, list_make1_oid(x2))
#define list_make3_oid(x1,x2,x3) lcons_oid(x1, list_make2_oid(x2, x3))
#define list_make4_oid(x1,x2,x3,x4) lcons_oid(x1, list_make3_oid(x2, x3, x4))
- #define list_make5_oid(x1,x2,x3,x4,x5) lcons_oid(x1, list_make4_oid(x2, x3, x4, x5))
/*
* foreach -
--- 134,149 ----
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
Attachments:
postgres-fdw-umid-v3.patchtext/x-diff; name=postgres-fdw-umid-v3.patchDownload
*** 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,x2) lcons(x1, list_make1(x2))
#define list_make3(x1,x2,x3) lcons(x1, list_make2(x2, x3))
#define list_make4(x1,x2,x3,x4) lcons(x1, list_make3(x2, x3, x4))
- #define list_make5(x1,x2,x3,x4,x5) lcons(x1, list_make4(x2, x3, x4, x5))
#define list_make1_int(x1) lcons_int(x1, NIL)
#define list_make2_int(x1,x2) lcons_int(x1, list_make1_int(x2))
#define list_make3_int(x1,x2,x3) lcons_int(x1, list_make2_int(x2, x3))
#define list_make4_int(x1,x2,x3,x4) lcons_int(x1, list_make3_int(x2, x3, x4))
- #define list_make5_int(x1,x2,x3,x4,x5) lcons_int(x1, list_make4_int(x2, x3, x4, x5))
#define list_make1_oid(x1) lcons_oid(x1, NIL)
#define list_make2_oid(x1,x2) lcons_oid(x1, list_make1_oid(x2))
#define list_make3_oid(x1,x2,x3) lcons_oid(x1, list_make2_oid(x2, x3))
#define list_make4_oid(x1,x2,x3,x4) lcons_oid(x1, list_make3_oid(x2, x3, x4))
- #define list_make5_oid(x1,x2,x3,x4,x5) lcons_oid(x1, list_make4_oid(x2, x3, x4, x5))
/*
* foreach -
--- 134,149 ----
On Wed, May 11, 2016 at 1:10 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
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
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
On Wed, May 11, 2016 at 1:34 PM, Etsuro Fujita <fujita.etsuro@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.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On 2016/05/11 18:03, Ashutosh Bapat wrote:
On Wed, May 11, 2016 at 1:34 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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
On 2016/05/12 13:02, Tom Lane wrote:
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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
On Thu, May 12, 2016 at 12:18 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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
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 Herrera http://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
On Thu, May 12, 2016 at 2:29 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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
Robert Haas <robertmhaas@gmail.com> 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
On 2016/05/13 3:53, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> 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
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
On 2016/05/13 3:53, Tom Lane wrote:
Robert Haas <robertmhaas(at)gmail(dot)com> 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