Thread: SPITupleTable members missing in docs

SPITupleTable members missing in docs

From
Daniel Gustafsson
Date:
Commit 3d13623d75d3206c8f009353415043a191ebab39 added the next and subid fields
to the SPITupleTable struct, but they never made it into the documentation.
While these are internal members, we already document several other internal
ones (with a sentence on not using them) so add these too to make the
documentation match reality.

Since this makes the number of internal members far outnumber the public ones,
also reword the statement about which fields can be used to try and improve
clarity.

cheers ./daniel


Attachment

Re: SPITupleTable members missing in docs

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
> Commit 3d13623d75d3206c8f009353415043a191ebab39 added the next and subid fields
> to the SPITupleTable struct, but they never made it into the documentation.
> While these are internal members, we already document several other internal
> ones (with a sentence on not using them) so add these too to make the
> documentation match reality.

> Since this makes the number of internal members far outnumber the public ones,
> also reword the statement about which fields can be used to try and improve
> clarity.

I wonder if we should just show the public fields in the docs?  Something
like

    typedef struct
    {
        ...
        TupleDesc   tupdesc;        /* row descriptor */
        HeapTuple  *vals;           /* rows */
        ...
    } SPITupleTable;

    (The struct contains additional fields that should not be touched
    by users of SPI.)

Not wedded to that, but it would reduce the risks of future mistakes
of this same sort.

            regards, tom lane



Re: SPITupleTable members missing in docs

From
Daniel Gustafsson
Date:
> On 14 Jun 2019, at 16:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Commit 3d13623d75d3206c8f009353415043a191ebab39 added the next and subid fields
>> to the SPITupleTable struct, but they never made it into the documentation.
>> While these are internal members, we already document several other internal
>> ones (with a sentence on not using them) so add these too to make the
>> documentation match reality.
>
>> Since this makes the number of internal members far outnumber the public ones,
>> also reword the statement about which fields can be used to try and improve
>> clarity.
>
> I wonder if we should just show the public fields in the docs?  Something
> like
>
>    typedef struct
>    {
>        ...
>        TupleDesc   tupdesc;        /* row descriptor */
>        HeapTuple  *vals;           /* rows */
>        ...
>    } SPITupleTable;
>
>    (The struct contains additional fields that should not be touched
>    by users of SPI.)
>
> Not wedded to that, but it would reduce the risks of future mistakes
> of this same sort.

Yeah, that’s clearly an alternative.  If we go down this route, then perhaps
the docs should be swept for other instances (if any) and handle them all to
keep things consistent? (and yes, I volunteer if we want to opt for that.)

cheers ./daniel


Re: SPITupleTable members missing in docs

From
Michael Paquier
Date:
On Fri, Jun 14, 2019 at 04:40:51PM +0200, Daniel Gustafsson wrote:
> Yeah, that’s clearly an alternative.  If we go down this route, then perhaps
> the docs should be swept for other instances (if any) and handle them all to
> keep things consistent? (and yes, I volunteer if we want to opt for that.)

FWIW, we still need a proper description of these fields in the docs,
so listing them in spi.sgml has the advantage to improve the grepping
coverage of which area needs to be patched.  This structure not
updated actually shows that this argument can be wrong as well ;p
--
Michael

Attachment

Re: SPITupleTable members missing in docs

From
Fabien COELHO
Date:
Hello Daniel,

> Since this makes the number of internal members far outnumber the public 
> ones, also reword the statement about which fields can be used to try 
> and improve clarity.

Patch applies cleanly, doc build ok.

To take into account Tom's comment, I'd suggest a middle ground by 
commenting a public and private part explicitely in the struct, something 
like:

   typedef struct {
     /* PUBLIC members to be used by callers ... */
     ...
     ...
     /* PRIVATE members, not intended for external usage ... */
     ...
   } ... ;

Another option would be to use some python-like naming convention on such 
members, eg with a leading underline character.

Even if it is redundant with the paragraph below, it would make things 
visually clear as well.

Note: I'm probaly not a member of the pgdoc list, so the delivery may fail 
there.

-- 
Fabien.



Re: SPITupleTable members missing in docs

From
Tom Lane
Date:
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> To take into account Tom's comment, I'd suggest a middle ground by 
> commenting a public and private part explicitely in the struct, something 
> like:

>    typedef struct {
>      /* PUBLIC members to be used by callers ... */
>      ...
>      ...
>      /* PRIVATE members, not intended for external usage ... */
>      ...
>    } ... ;

One problem is that the members we've retroactively decided are "public"
are in the middle of the struct :-(.

But it occurs to me that there's no good reason we couldn't re-order the
members, as long as we only do so on HEAD and not in released versions.
That would make it a bit less inconsistent and easier to add labels
such as you suggest.

> Note: I'm probaly not a member of the pgdoc list, so the delivery may fail 
> there.

FYI, I believe the current policy is that as long as you're subscribed
to at least one PG list you can post to any of them.

            regards, tom lane



Re: SPITupleTable members missing in docs

From
Fabien COELHO
Date:
Hello,

>> To take into account Tom's comment, I'd suggest a middle ground by 
>> commenting a public and private part explicitely in the struct,
>>    typedef struct {
>>      /* PUBLIC members to be used by callers ... */
>>      /* PRIVATE members, not intended for external usage ... */
>>    } ... ;
>
> One problem is that the members we've retroactively decided are "public"
> are in the middle of the struct :-(.

Argh, I did not notice this tiny but relevant detail.

> But it occurs to me that there's no good reason we couldn't re-order the
> members, as long as we only do so on HEAD and not in released versions.
> That would make it a bit less inconsistent and easier to add labels
> such as you suggest.

Indeed. SPI-dependent extensions are likely recompiled between major 
version, so a reordering should not cause significant problems.

This mean that a simple doc patch is turned into a code patch.

-- 
Fabien.



Re: SPITupleTable members missing in docs

From
Daniel Gustafsson
Date:
> On 12 Jul 2019, at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> To take into account Tom's comment, I'd suggest a middle ground by
>> commenting a public and private part explicitely in the struct, something
>> like:

Thanks for the review!

>>   typedef struct {
>>     /* PUBLIC members to be used by callers ... */
>>     ...
>>     ...
>>     /* PRIVATE members, not intended for external usage ... */
>>     ...
>>   } ... ;
>
> One problem is that the members we've retroactively decided are "public"
> are in the middle of the struct :-(.
>
> But it occurs to me that there's no good reason we couldn't re-order the
> members, as long as we only do so on HEAD and not in released versions.
> That would make it a bit less inconsistent and easier to add labels
> such as you suggest.

I quite like this suggestion, so I’ve changed the patch to do this.  Removed
the doc: in the commit message to indicate that this is no longer just touching
documentation.

cheers ./daniel


Attachment

Re: SPITupleTable members missing in docs

From
Fabien COELHO
Date:
Hello Daniel,

> I quite like this suggestion, so I’ve changed the patch to do this. 
> Removed the doc: in the commit message to indicate that this is no 
> longer just touching documentation.

And it should be posted to <pgsql-hackers@lists.postgresql.org>.

-- 
Fabien.

Re: SPITupleTable members missing in docs

From
Tom Lane
Date:
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Hello Daniel,
>> I quite like this suggestion, so I’ve changed the patch to do this. 
>> Removed the doc: in the commit message to indicate that this is no 
>> longer just touching documentation.

> And it should be posted to <pgsql-hackers@lists.postgresql.org>.

Seems excessive, since there's no actual functional change here.

            regards, tom lane



Re: SPITupleTable members missing in docs

From
Fabien COELHO
Date:
> I quite like this suggestion, so I’ve changed the patch to do this.  Removed
> the doc: in the commit message to indicate that this is no longer just touching
> documentation.

About v2: applies cleanly, compiles, make check and doc gen ok.

However, the documentation does not look right, field comments are not 
aligned. Do not use tabs in the sgml file, use spaces only, otherwise the 
display layout is left to chance.

-- 
Fabien.

Re: SPITupleTable members missing in docs

From
Daniel Gustafsson
Date:
> On 15 Jul 2019, at 23:18, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

>> I quite like this suggestion, so I’ve changed the patch to do this.  Removed
>> the doc: in the commit message to indicate that this is no longer just touching
>> documentation.
>
> About v2: applies cleanly, compiles, make check and doc gen ok.
>
> However, the documentation does not look right, field comments are not aligned. Do not use tabs in the sgml file, use
spacesonly, otherwise the display layout is left to chance. 

Of course, that was me fat-fingering.  Fixed in the attached v3.  Thanks for
reviewing!

cheers ./daniel


Attachment

Re: SPITupleTable members missing in docs

From
Fabien COELHO
Date:
>>> I quite like this suggestion, so I’ve changed the patch to do this.  Removed
>>> the doc: in the commit message to indicate that this is no longer just touching
>>> documentation.
>>
>> About v2: applies cleanly, compiles, make check and doc gen ok.
>>
>> However, the documentation does not look right, field comments are not 
>> aligned. Do not use tabs in the sgml file, use spaces only, otherwise 
>> the display layout is left to chance.
>
> Of course, that was me fat-fingering.  Fixed in the attached v3. 
> Thanks for reviewing!

V3 fixes the html display alignment issue. Patch marked as ready.

-- 
Fabien.

Re: SPITupleTable members missing in docs

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 15 Jul 2019, at 23:18, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> However, the documentation does not look right, field comments are not aligned. Do not use tabs in the sgml file,
usespaces only, otherwise the display layout is left to chance. 

> Of course, that was me fat-fingering.  Fixed in the attached v3.  Thanks for
> reviewing!

Pushed with some marginal additional fiddling with the comments.

            regards, tom lane