Use %u to print user mapping's umid and 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. 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

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

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

[1]: /messages/by-id/56E66F61.3070201@lab.ntt.co.jp

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

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