Thread: SPITupleTable members missing in docs
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
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
> 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
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
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.
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
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.
> 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
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.
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
> 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.
> 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
>>> 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.
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