Re: [HACKERS] GetExistingLocalJoinPath() vs. the docs

2016-03-04 Thread Robert Haas 
On Wed, Mar 2, 2016 at 1:12 AM, Ashutosh Bapat
 wrote:
>> I think that you need to take a little broader look at this section.
>> At the top, it says "To use any of these functions, you need to
>> include the header file foreign/foreign.h in your source file", but
>> this function is defined in foreign/fdwapi.h.  It's not clear to me
>> whether we should consider moving the prototype, or just document that
>> this function is someplace else.  The other functions prototyped in
>> fdwapi.h aren't documented at all, except for
>> IsImportableForeignTable, which is mentioned in passing.
>>
>> Further down, the section says "Some object types have name-based
>> lookup functions in addition to the OID-based ones:" and you propose
>> to put the documentation for this function after that.  But this
>> comment doesn't actually describe this particular function.
>>
>>
>> Actually, this function just doesn't seem to fit into this section at
>> all.  It's really quite different from the others listed there.  How
>> about something like the attached instead?
>
> Right. Mentioning the function in the description of relevant function looks
> better and avoids some duplication.

Cool, committed that way.

-- 
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] GetExistingLocalJoinPath() vs. the docs

2016-03-01 Thread Ashutosh Bapat 
I think that you need to take a little broader look at this section.
> At the top, it says "To use any of these functions, you need to
> include the header file foreign/foreign.h in your source file", but
> this function is defined in foreign/fdwapi.h.  It's not clear to me
> whether we should consider moving the prototype, or just document that
> this function is someplace else.  The other functions prototyped in
> fdwapi.h aren't documented at all, except for
> IsImportableForeignTable, which is mentioned in passing.
>
> Further down, the section says "Some object types have name-based
> lookup functions in addition to the OID-based ones:" and you propose
> to put the documentation for this function after that.  But this
> comment doesn't actually describe this particular function.
>

> Actually, this function just doesn't seem to fit into this section at
> all.  It's really quite different from the others listed there.  How
> about something like the attached instead?


Right. Mentioning the function in the description of relevant function
looks better and avoids some duplication.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] GetExistingLocalJoinPath() vs. the docs

2016-03-01 Thread Robert Haas 
On Tue, Feb 16, 2016 at 7:53 AM, Ashutosh Bapat
 wrote:
> PFA patch fixing those things.

I think that you need to take a little broader look at this section.
At the top, it says "To use any of these functions, you need to
include the header file foreign/foreign.h in your source file", but
this function is defined in foreign/fdwapi.h.  It's not clear to me
whether we should consider moving the prototype, or just document that
this function is someplace else.  The other functions prototyped in
fdwapi.h aren't documented at all, except for
IsImportableForeignTable, which is mentioned in passing.

Further down, the section says "Some object types have name-based
lookup functions in addition to the OID-based ones:" and you propose
to put the documentation for this function after that.  But this
comment doesn't actually describe this particular function.

Actually, this function just doesn't seem to fit into this section at
all.  It's really quite different from the others listed there.  How
about something like the attached instead?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


gejlp-rmh-doc.patch
Description: application/download

-- 
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] GetExistingLocalJoinPath() vs. the docs

2016-02-16 Thread Ashutosh Bapat 
On Mon, Feb 15, 2016 at 9:11 PM, Stephen Frost  wrote:

> Greetings,
>
> While getting back into the thread Re: Optimization for updating foreign
> tables in Postgres FDW, I noticed some issues with the docs and
> GetExistingLocalJoinPath():
>
> GetExistingLocalJoinPath() exists in src/backend/foreign/foreign.c, but
> the docs include discussion of it under 54.2 - Foreign Data Wrapper
> Callback Routines.  Shouldn't this be under 54.3. Foreign Data Wrapper
> Helper Functions?


Yes


> Also, the prototype is incorrect in the docs (it
> doesn't return a void)


Thanks for pointing that out.


> and the paragraph about what it's for could do
> with some wordsmithing.
>

Any specific suggestions?


>
> A link from 54.2 to 54.3 which mentions it would be fine, of course.
>

Ok

PFA patch fixing those things.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


geljp_doc.patch
Description: application/download

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] GetExistingLocalJoinPath() vs. the docs

2016-02-15 Thread Stephen Frost 
Greetings,

While getting back into the thread Re: Optimization for updating foreign
tables in Postgres FDW, I noticed some issues with the docs and
GetExistingLocalJoinPath():

GetExistingLocalJoinPath() exists in src/backend/foreign/foreign.c, but
the docs include discussion of it under 54.2 - Foreign Data Wrapper
Callback Routines.  Shouldn't this be under 54.3. Foreign Data Wrapper
Helper Functions?  Also, the prototype is incorrect in the docs (it
doesn't return a void) and the paragraph about what it's for could do
with some wordsmithing.

A link from 54.2 to 54.3 which mentions it would be fine, of course.

Thanks!

Stephen


signature.asc
Description: Digital signature