doc: Clarify Savepoint Behavior

On Thu, Jun 9, 2022 at 8:36 AM David G. Johnston <david.g.johnston@gmail.com>
wrote:

Hi,

Reposting this on its own thread.

/messages/by-id/CAKFQuwby1aMsJDMeibaBaohgoaZhivAo4WcqHC1=9-GDZ3TSng@mail.gmail.com

Presently, the open item seems to be whether my novelty regarding the
reworked example is too much.

Commentary:

Per documentation comment the savepoint command lacks an example
where the savepoint name is reused. The suggested example didn't
conform to the others on the page, nor did the suggested location
in compatibility seem desirable, but the omission rang true. Add
another example to the examples section demonstrating this case.
Additionally, document under the description for savepoint_name
that we allow for the name to be repeated - and note what that
means in terms of release and rollback. It seems desirable to
place this comment in description rather than notes for savepoint.
For the other two commands the behavior in the presence of
duplicate savepoint names best fits as notes. In fact release
already had one. This commit copies the same verbiage over to
rollback.

David J.

On Thu, 9 Jun 2022 at 16:41, David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Thu, Jun 9, 2022 at 8:36 AM David G. Johnston <david.g.johnston@gmail.com> wrote:

Hi,

Reposting this on its own thread.

/messages/by-id/CAKFQuwby1aMsJDMeibaBaohgoaZhivAo4WcqHC1=9-GDZ3TSng@mail.gmail.com

Presently, the open item seems to be whether my novelty regarding the reworked example is too much.

Commentary:

Per documentation comment the savepoint command lacks an example
where the savepoint name is reused. The suggested example didn't
conform to the others on the page, nor did the suggested location
in compatibility seem desirable, but the omission rang true. Add
another example to the examples section demonstrating this case.
Additionally, document under the description for savepoint_name
that we allow for the name to be repeated - and note what that
means in terms of release and rollback. It seems desirable to
place this comment in description rather than notes for savepoint.
For the other two commands the behavior in the presence of
duplicate savepoint names best fits as notes. In fact release
already had one. This commit copies the same verbiage over to
rollback.

Good idea.

"The name to give to the new savepoint.  The name may already exist,
+      in which case a rollback or release to the same name will use the
+      one that was most recently defined."

instead I propose:

"The name to give to the new savepoint. If the name duplicates a
previously defined savepoint name then only the latest savepoint with that name
can be referenced in a later ROLLBACK TO SAVEPOINT."

+  <para>
+   If multiple savepoints have the same name, only the one that was most
+   recently defined is released.
+  </para>

instead I propose

"Searches backwards through previously defined savepoints until the
a savepoint name matches the request. If the savepoint name duplicated earlier
defined savepoints then those earlier savepoints can only be released if
multiple ROLLBACK TO SAVEPOINT commands are issued with the same
name, as shown in the following example."

Also, I would just call the savepoint "s" in the example, to declutter it.

--
Simon Riggs http://www.EnterpriseDB.com/

Thank you for the review.

On Thu, Jun 23, 2022 at 5:35 AM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

On Thu, 9 Jun 2022 at 16:41, David G. Johnston
<david.g.johnston@gmail.com> wrote:

"The name to give to the new savepoint.  The name may already exist,
+      in which case a rollback or release to the same name will use the
+      one that was most recently defined."

instead I propose:

"The name to give to the new savepoint. If the name duplicates a
previously defined savepoint name then only the latest savepoint with
that name
can be referenced in a later ROLLBACK TO SAVEPOINT."

So leave the "release" behavior implied from the rollback behavior?

On the whole I'm slightly in favor of your proposed wording (mostly due to
the better fitting of the ROLLBACK command, though at the omission of
RELEASE...) but are you seeing anything beyond personal style as to why you
feel one is better than the other? Is there some existing wording in the
docs that I should be conforming to here?

+  <para>
+   If multiple savepoints have the same name, only the one that was most
+   recently defined is released.
+  </para>

instead I propose

"Searches backwards through previously defined savepoints until the
a savepoint name matches the request. If the savepoint name duplicated
earlier
defined savepoints then those earlier savepoints can only be released if
multiple ROLLBACK TO SAVEPOINT commands are issued with the same
name, as shown in the following example."

Upon reflection, adding this after the comments about cursors seems like a
poor location, I will probably move it up one paragraph.

I dislike this proposal for its added wordiness that doesn't bring in new
material. The whole idea of "searching backwards until the name is found"
is already covered in the description:

"ROLLBACK TO SAVEPOINT implicitly destroys all savepoints that were
established after the named savepoint."

Using the phrase "can only be released if" here in the rollback to
savepoint command page just seems to be asking for confusion between this
and the release savepoint command.

Also, I would just call the savepoint "s" in the example, to declutter it.

If I do use a name that differs from the other two examples on that page
I'll probably go with "sp" for added detectability - but deviating from the
established convention doesn't seem warranted here.

In all, I'm still content with the patch as-is; though I or the committer
should consider moving up the one paragraph in rollback to savepoint.
Otherwise I'll probably post an updated patch sometime this coming week and
give another look at the savepoint name description and make that paragraph
move.

David J.

On Sun, Jun 26, 2022 at 09:14:56AM -0700, David G. Johnston wrote:

So leave the "release" behavior implied from the rollback behavior?

On the whole I'm slightly in favor of your proposed wording (mostly due to the
better fitting of the ROLLBACK command, though at the omission of RELEASE...)
but are you seeing anything beyond personal style as to why you feel one is
better than the other?  Is there some existing wording in the docs that I
should be conforming to here?

I have developed the attached patch based on the discussion here. I
tried to simplify the language and example to clarify the intent.

I was confused why the first part of the patch added a mention of
releasing savepoints to the ROLLBACK TO manual page --- I have removed
that and improved the text in RELEASE SAVEPOINT.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Indecision is a decision. Inaction is an action. Mark Batterson

On Sat, Jul 9, 2022 at 12:59:23PM -0400, Bruce Momjian wrote:

On Sun, Jun 26, 2022 at 09:14:56AM -0700, David G. Johnston wrote:

So leave the "release" behavior implied from the rollback behavior?

On the whole I'm slightly in favor of your proposed wording (mostly due to the
better fitting of the ROLLBACK command, though at the omission of RELEASE...)
but are you seeing anything beyond personal style as to why you feel one is
better than the other?  Is there some existing wording in the docs that I
should be conforming to here?

I have developed the attached patch based on the discussion here. I
tried to simplify the language and example to clarify the intent.

I was confused why the first part of the patch added a mention of
releasing savepoints to the ROLLBACK TO manual page --- I have removed
that and improved the text in RELEASE SAVEPOINT.

Patch applied to all supported versions.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Indecision is a decision. Inaction is an action. Mark Batterson

On Thu, Jul 14, 2022 at 12:44 PM Bruce Momjian <bruce@momjian.us> wrote:

On Sat, Jul 9, 2022 at 12:59:23PM -0400, Bruce Momjian wrote:

On Sun, Jun 26, 2022 at 09:14:56AM -0700, David G. Johnston wrote:

So leave the "release" behavior implied from the rollback behavior?

On the whole I'm slightly in favor of your proposed wording (mostly

due to the

better fitting of the ROLLBACK command, though at the omission of

RELEASE...)

but are you seeing anything beyond personal style as to why you feel

one is

better than the other? Is there some existing wording in the docs

that I

should be conforming to here?

I have developed the attached patch based on the discussion here. I
tried to simplify the language and example to clarify the intent.

I was confused why the first part of the patch added a mention of
releasing savepoints to the ROLLBACK TO manual page --- I have removed
that and improved the text in RELEASE SAVEPOINT.

Patch applied to all supported versions.

Bruce,

Thanks for committing this and the other patches. Should I go into the
commitfest and mark the entries for these as committed or does protocol
dictate I remind you and you do that?

David J.