Message-ID: <f7a51be79a5146740954e0b9b70515529a1d944b.camel@cybertec.at>
Subject: Re: nextval parameter is not clear
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Kirk Wolak <wolakk@gmail.com>, "David G. Johnston"
	 <david.g.johnston@gmail.com>
Cc: Bruce Momjian <bruce@momjian.us>, ailjushkin@gmail.com,
	pgsql-docs@lists.postgresql.org
Date: Mon, 28 Nov 2022 04:34:56 +0100
In-Reply-To: 
 <CACLU5mR7JsrJN6qnz=Gh4Ta-0E1hsL++DLLMeWDMQMCVKbOQvw@mail.gmail.com>
References: 
	<166608103231.2042183.13992576112735261253@wrigleys.postgresql.org>
	 <Y30ekxQpiIaG9eSl@momjian.us>
	 <CACLU5mQBVn+cyeAAbs7O6c8h14MBurhuxpp7z+TXWo2+vQcO+w@mail.gmail.com>
	 <CAKFQuwau7CpY+TLufpU1RPPzyLzjS0mvrXnSLf+eGSUJS-_pRQ@mail.gmail.com>
	 <CACLU5mQ1LTz9BnG-bbfCmwzytyUFwej90XBnFtQJKQkubS9Khw@mail.gmail.com>
	 <CACLU5mTKsNgWfY9qcsn1fhMRXbfJ0M8bPEwgXbmanxVgdKPQ7A@mail.gmail.com>
	 <CACLU5mS4b3xM-tuMpT2oP-pqwwnHJbSeLDBzZSs9Lq_ObgMu7Q@mail.gmail.com>
	 <21e017222199f3ca397fa70549140d226236e4b6.camel@cybertec.at>
	 <CACLU5mShHGjroH=Cp_k6i-T+357_Fhxahc5NqpNPoJnN56GyFg@mail.gmail.com>
	 <CAKFQuwarCJs9bbgJ9_PtsCBJKmSM1UnLx80J5JUJm1eJyYzHXQ@mail.gmail.com>
	 <CACLU5mRn-xHKP9irkBQvYCQYKNDCcud66zJHHaHNHoOMA4kmVQ@mail.gmail.com>
	 <0c0eddb287571e72a2df1be8758fbbb0cb7cc378.camel@cybertec.at>
	 <CAKFQuwYuu_FrZZ=TEdUPdB4c-hbsCHSKD+XSUdDFrGnaDMQ_=A@mail.gmail.com>
	 <CACLU5mTmEieU-uTF6bNm9T5Y94gd1g_cgz_JDLz6Qw36RjcBHQ@mail.gmail.com>
	 <CACLU5mR7JsrJN6qnz=Gh4Ta-0E1hsL++DLLMeWDMQMCVKbOQvw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/f7a51be79a5146740954e0b9b70515529a1d944b.camel%40cybertec.at>
Precedence: bulk

On Sat, 2022-11-26 at 23:22 -0500, Kirk Wolak wrote:
> Your feedback would be appreciated.

> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -17625,6 +17625,11 @@ $.* ? (@ like_regex "^\\d+$")
>          command.
>        </para>
>         <para>
> +<programlisting>
> +SELECT nextval('myseq'::regclass);
> +</programlisting>
> +      </para>
> +       <para>

The indentation of the <para> tags should be the same everywhere.
True, the misalignment was pre-existing here, but it would be better to
fix it than to repeat the mistake.

I think the cast to "regclass" should be only in the more elaborate
example at the end

> @@ -17669,7 +17674,6 @@ SELECT setval('myseq', 42, false);    <lineannotation>Next <function>nextval</fu
>          sequence.
>         </para></entry>
>        </row>
> -
>        <row>
>         <entry role="func_table_entry"><para role="func_signature">
>          <indexterm>

I think the empty line is intentional and should stay.  We have it
everywhere else, and it helps to read the source.

> @@ -17707,19 +17714,75 @@ SELECT setval('myseq', 42, false);    <lineannotation>Next <function>nextval</fu
>          identical to <function>currval</function>, except that instead
>          of taking the sequence name as an argument it refers to whichever
>          sequence <function>nextval</function> was most recently applied to
> -        in the current session. It is an error to call
> -        <function>lastval</function> if <function>nextval</function>
> -        has not yet been called in the current session.
> +        in the current session. (An error is reported if <function>nextval</function> has
> +        never been called in this session.) 
> +<programlisting> 
> +SELECT lastval();
> +</programlisting>

I have no problem with changing the sentence to a parenthetical remark, although
I thing the original wording was fine.  However, there is a trailing blank at the
end of the line (and in the <programlisting> line as well).  Please avoid these.
"git diff" highlights such training blanks.

I would not add a code sample in this case.  The function has no arguments, so what
cat we really learn from that?  We don't have to have a code sample here just because
we had one with all the other functions.

>         </para>
>         <para>
>          This function requires <literal>USAGE</literal>
>          or <literal>SELECT</literal> privilege on the last used sequence.
>         </para></entry>
>        </row>
> +      <row>
> +       <entry role="func_table_entry">
> +      </entry>
> +      </row>
>       </tbody>
>      </tgroup>
>     </table>

This empty row should definitely go.

> +  <para>Example

I think you should use the <title> tag to render that more conspicuous.

> +<programlisting>
> +CREATE SCHEMA play;                            <lineannotation>-- Create a play schema</lineannotation>
> [...]

Now I think that is taking it too far.  Your code sample would be great for a tutorial,
but it is too elaborate for the technical documentation.  The example should focus on
the sequence functions, but more than half of the code describes other parts of
PostgreSQL:

- schema and search_path
- psql commands
- INSERT ... RETURNING
- pg_get_serial_sequence()

I am not saying that this is bad material, but if anything, it should go to
chapter 3 (Advanced Features) of the tutorial (and then it could be even more
elaborate).

I am alright with having a CREATE TABLE with a DEFAULT and an INSERT or two;
whatever it takes to show the functions in a realistic scenario.
For example, you could INSERT a row that overrides the DEFAULT, then call
"setval()" to readjust the sequence.

Yours,
Laurenz Albe