SPITupleTable members missing in docs

Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

SPITupleTable members missing in docs

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


spitupletable.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Tom Lane-2
Daniel Gustafsson <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Daniel Gustafsson
> On 14 Jun 2019, at 16:15, Tom Lane <[hidden email]> wrote:
>
> Daniel Gustafsson <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Fabien COELHO-3
In reply to this post by Daniel Gustafsson

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.


Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Tom Lane-2
Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Daniel Gustafsson
In reply to this post by Tom Lane-2
> On 12 Jul 2019, at 17:04, Tom Lane <[hidden email]> wrote:
>
> Fabien COELHO <[hidden email]> 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


spitupletable-v2.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Fabien COELHO-3

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 <[hidden email]>.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Tom Lane-2
Fabien COELHO <[hidden email]> 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 <[hidden email]>.

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

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: SPITupleTable members missing in docs

Fabien COELHO-3
In reply to this post by Daniel Gustafsson

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