small patch

On Fri, 2021-10-01 at 21:06 -0400, rir wrote:

Minor changes to move.sgml and fetch.sgml.

The text 'or empty' is inconsistent by restating what the
synopsis notation has expressed.

The comments on sharing a language feature, while
likely helpful during review, seem verbose compared to
the non-commenting in other similar files.

Thanks for the effort of preparing a patch.

However, I don't think that is an improvement:

- the comments pointing from MOVE to FETCH and vice versa are
helpful for people who edit the documentation like you did

- we should retain "empty or one of", otherwise the following syntax
would be undocumented:

FETCH FROM c;

Yours,
Laurenz Albe

On Mon, Oct 04, 2021 at 08:18:22AM +0200, Laurenz Albe wrote:

On Fri, 2021-10-01 at 21:06 -0400, rir wrote:

Minor changes to move.sgml and fetch.sgml.

The text 'or empty' is inconsistent by restating what the
synopsis notation has expressed.

The comments on sharing a language feature, while
likely helpful during review, seem verbose compared to
the non-commenting in other similar files.

Thanks for the effort of preparing a patch.

However, I don't think that is an improvement:

- the comments pointing from MOVE to FETCH and vice versa are
helpful for people who edit the documentation like you did

- we should retain "empty or one of", otherwise the following syntax
would be undocumented:

FETCH FROM c;

Yours,
Laurenz Albe

Laurenz,

Your view is completely reasonable, but it suggests that
many of the synopses are leaving syntax undocumented.
The 'empty or one of:' is only used in these two synopses.

If I had found the comments helpful, I would have spared them.
My sense was that the comments were unusual by their existence
for their purpose. That was not rigorous: so okay.

Since I'm only making the same points again, I'll stop.

Thanks again,
Rob

On Wed, 2021-10-06 at 23:39 -0400, rir wrote:

On Mon, Oct 04, 2021 at 08:18:22AM +0200, Laurenz Albe wrote:

On Fri, 2021-10-01 at 21:06 -0400, rir wrote:

Minor changes to move.sgml and fetch.sgml.

The text 'or empty' is inconsistent by restating what the
synopsis notation has expressed.

The comments on sharing a language feature, while
likely helpful during review, seem verbose compared to
the non-commenting in other similar files.

Thanks for the effort of preparing a patch.

However, I don't think that is an improvement:

- the comments pointing from MOVE to FETCH and vice versa are
  helpful for people who edit the documentation like you did
- we should retain "empty or one of", otherwise the following syntax
  would be undocumented:

      FETCH FROM c;

Your view is completely reasonable, but it suggests that
many of the synopses are leaving syntax undocumented.
The 'empty or one of:' is only used in these two synopses.

You have a point there.

Can you think of a way to modify the syntax diagram so that it
expresses that and still remains comprehensible?

Yours,
Laurenz Albe

On Thu, Oct 07, 2021 at 07:58:47AM +0200, Laurenz Albe wrote:

On Wed, 2021-10-06 at 23:39 -0400, rir wrote:

On Mon, Oct 04, 2021 at 08:18:22AM +0200, Laurenz Albe wrote:

On Fri, 2021-10-01 at 21:06 -0400, rir wrote:

Minor changes to move.sgml and fetch.sgml.

However, I don't think that is an improvement:

- the comments pointing from MOVE to FETCH and vice versa are
� helpful for people who edit the documentation like you did
- we should retain "empty or one of", otherwise the following syntax
� would be undocumented:

����� FETCH FROM c;

Your view is completely reasonable, but it suggests that
many of the synopses are leaving syntax undocumented.
The 'empty or one of:' is only used in these two synopses.

Can you think of a way to modify the syntax diagram so that it
expresses that and still remains comprehensible?

For myself,
'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
clearly indicates that 'direction' is optional.

Rob

On 2021-Oct-07, rir wrote:

For myself,
'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
clearly indicates that 'direction' is optional.

Hmm, aren't you misreading the scope of the outer square brackets? If
<direction> is optional independently of [FROM|IN], then the synopsis
should be

FETCH [ <direction> ] [ FROM | IN ] <cursor_name>

and I think that's correct, since all forms omitting any of these are
accepted:

alvherre=# begin;
BEGIN
alvherre=*# declare c scroll cursor with hold for select 1 ;
DECLARE CURSOR
alvherre=*# commit;
COMMIT
alvherre=# fetch forward from c;
?column?
----------
1
(1 fila)

alvherre=# fetch from c;
?column?
----------
(0 filas)

alvherre=# fetch BACKWARD c;
?column?
----------
1
(1 fila)

alvherre=# fetch c;
?column?
----------
(0 filas)

--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/

rir <rirans@comcast.net> writes:

On Thu, Oct 07, 2021 at 07:58:47AM +0200, Laurenz Albe wrote:

Can you think of a way to modify the syntax diagram so that it
expresses that and still remains comprehensible?

For myself,
'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
clearly indicates that 'direction' is optional.

Right, but if we don't say that <direction> can be
empty, then this diagram disallows

FETCH FROM cursor_name

which in fact is legal. I think however that we could make it read

FETCH [ <direction> ] [ FROM | IN ] <cursor_name>'

and have a correct description without requiring <direction>
to be allowed to be empty.

BTW, as it stands, the diagram is ambiguous
because there are two ways to parse

FETCH cursor_name

... is <direction> present but empty, or omitted altogether?

regards, tom lane

On Thu, 2021-10-07 at 16:06 -0400, rir wrote:

- we should retain "empty or one of", otherwise the following syntax
would be undocumented:

For myself,
'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
clearly indicates that 'direction' is optional.

Yes, but since [ FROM | IN ] is inside the bracket, that
diagram would not account for "direction" being omitted
while retaining FROM.

So I suggest that you change the syntax diagram to

FETCH [ direction ] [ FROM | IN ] cursor_name

Then I agree that the "empty or" can be removed.
I think that would be a good idea, and it would add
clarity.

I remain of the opinion that the comments should be
retained, but we can leave that for somebody else to
decide.

Do you want to prepare a new patch and register it in
the commitfest?

Yours,
Laurenz Albe

On Thu, Oct 07, 2021 at 04:24:16PM -0400, Tom Lane wrote:

rir <rirans@comcast.net> writes:

On Thu, Oct 07, 2021 at 07:58:47AM +0200, Laurenz Albe wrote:

Can you think of a way to modify the syntax diagram so that it
expresses that and still remains comprehensible?

For myself,
'FETCH [ <direction> [ FROM | IN ] ] <cursor_name>'
clearly indicates that 'direction' is optional.

FETCH FROM cursor_name

which in fact is legal. I think however that we could make it read

FETCH [ <direction> ] [ FROM | IN ] <cursor_name>'

and have a correct description without requiring <direction>
to be allowed to be empty.

BTW, as it stands, the diagram is ambiguous
because there are two ways to parse

FETCH cursor_name

... is <direction> present but empty, or omitted altogether?

I am confident you know what you mean, but I don't. At the _parsing_
stage how is any distinction between emptiness and omission?

I can correct the patch to reflect the rightness of
FETCH [ <direction> ] [ FROM | IN ] <cursor_name>
in the sgml file and at least one misquote I have found.

Not being here that often, I will wait to see this conversation resolve.

Rob

rir <rirans@comcast.net> writes:

On Thu, Oct 07, 2021 at 04:24:16PM -0400, Tom Lane wrote:

BTW, as it stands, the diagram is ambiguous
because there are two ways to parse
FETCH cursor_name
... is <direction> present but empty, or omitted altogether?

I am confident you know what you mean, but I don't. At the _parsing_
stage how is any distinction between emptiness and omission?

Bison certainly makes a distinction, because it would have two different
production paths it could follow if the actual grammar looked like this
(which it doesn't). It doesn't care whether the emitted parse tree
would be the same. Indeed, it would be possible for the two cases to
create different parse trees.

regards, tom lane

On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:

On Thu, 2021-10-07 at 16:06 -0400, rir wrote:

So I suggest that you change the syntax diagram to

FETCH [ direction ] [ FROM | IN ] cursor_name

Then I agree that the "empty or" can be removed.

I remain of the opinion that the comments should be
retained, but we can leave that for somebody else to
decide.

I accept your three points above.

The MOVE synopsis shows the same parsing as I presented,
should it be changed in the same way (move a square bracket left to
be after <direction>)? My guess is yes, but I've never used an
SQL cursor.

When this convo settles, I send a new patch. Probably
here in the group. If I have a few more, or a complex one,
I'll check out the other submission method.

Rob

On Sun, 2021-10-10 at 16:15 -0400, rir wrote:

On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:

On Thu, 2021-10-07 at 16:06 -0400, rir wrote:

So I suggest that you change the syntax diagram to

FETCH [ direction ] [ FROM | IN ] cursor_name

Then I agree that the "empty or" can be removed.

I remain of the opinion that the comments should be
retained, but we can leave that for somebody else to
decide.

I accept your three points above.

The MOVE synopsis shows the same parsing as I presented,
should it be changed in the same way (move a square bracket left to
be after <direction>)?  My guess is yes, but I've never used an
SQL cursor.

When this convo settles, I send a new patch.  Probably
here in the group.  If I have a few more, or a complex one,
I'll check out the other submission method.

Yes, I think that such a patch would meet with favor.

Make sure to register it on the commitfest. To do that, it is better
to send the patch to the -hackers list. The commitfest application
won't find conversations on the -docs list.

Yours,
Laurenz Albe

On Sun, Oct 10, 2021 at 04:15:01PM -0400, rir wrote:

On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:

On Thu, 2021-10-07 at 16:06 -0400, rir wrote:

FETCH [ direction ] [ FROM | IN ] cursor_name

Should the pgplsql docs be similiarly changed per the above
in 43.7.3.1 and 2?

Rob

On Mon, 2021-10-11 at 23:14 -0400, rir wrote:

On Sun, Oct 10, 2021 at 04:15:01PM -0400, rir wrote:

On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:

On Thu, 2021-10-07 at 16:06 -0400, rir wrote:

FETCH [ direction ] [ FROM | IN ] cursor_name

Should the pgplsql docs be similiarly changed per the above
in 43.7.3.1 and 2?

Absolutely!

It may even make sense to extend the existing comments to
alert people who modify the syntax diagram in the future
that they should also consider reviewing the PL/pgSQL syntax.

Yours,
Laurenz Albe

On Tue, Oct 12, 2021 at 08:43:34AM +0200, Laurenz Albe wrote:

On Mon, 2021-10-11 at 23:14 -0400, rir wrote:

On Sun, Oct 10, 2021 at 04:15:01PM -0400, rir wrote:

On Fri, Oct 08, 2021 at 02:47:43PM +0200, Laurenz Albe wrote:

On Thu, 2021-10-07 at 16:06 -0400, rir wrote:

FETCH [ direction ] [ FROM | IN ] cursor_name

Should the pgplsql docs be similiarly changed per the above
in 43.7.3.1 and 2?

Absolutely!

It may even make sense to extend the existing comments to
alert people who modify the syntax diagram in the future
that they should also consider reviewing the PL/pgSQL syntax.

I will make the syntax change.

Since my understanding is that, absent contrary documentation,
plpgSQL follows SQL, I resist the idea of yet another comment.
Better would be establishing a SPoT, tagging it, and tagging
various forms of derived info as quotes, code samples, etc. or,
at worst, logically dependent. This more form of commenting
could allow linting processes to support its use and check
its accuracy.

Absent that ...

Rob

On 2021-Oct-08, Laurenz Albe wrote:

I remain of the opinion that the comments should be
retained, but we can leave that for somebody else to
decide.

So I just realized that I added this comment in 8c250f3741f.

The point of this comment is that the list of options to which
"direction" expands is duplicate and needs to be kept in sync. However,
clearly we have other places in the docs where similar lists are
duplicated and no such comments are kept; see "column_constraint" in
alter_table.sgml and create_table.sgml for an obvious one. I can't
quite make up my mind about the comment being actually helpful if
somebody adds a new type of contraints to remind them that they also
need to add it to the other place. What do you think?

I think I side with Laurenz that the comment should be kept, even if
it's just out of inertia.

Maybe a better solution (more convoluted? overengineered?) would be to
define an SGML entity in the first of those pages that uses the list in
such a way so that it expands to that list, and then use that entity in
the other page. I don't know how that is done, but surely it must be
possible.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"E pur si muove" (Galileo Galilei)