Обсуждение: 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

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

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

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

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

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

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

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

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], 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.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 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

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

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

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

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

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

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

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

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

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