Thread: Automatic notification of top transaction IDs
(Re-sending this email, because the Commitfest app mistakenly [3] considered previous email [4] to be part of the old thread, whereas it should not be considered that way) I came across this thread [1] to disallow canceling a transaction not yet confirmed by a synchronous replica. I think my proposed patch might help that case as well, hence adding all involved in that thread to BCC, for one-time notification. As mentioned in that thread, when sending a cancellation signal, the client cannot be sure if the cancel signal was honored, and if the transaction was cancelled successfully. In the attached patch, the backend emits a NotificationResponse containing the current full transaction id. It does so only if the relevant GUC is enabled, and when the top-transaction is being assigned the ID. This information can be useful to the client, when: i) it wants to cancel a transaction _after_ issuing a COMMIT, and ii) it wants to check the status of its transaction that it sent COMMIT for, but never received a response (perhaps because the server crashed). Additionally, this information can be useful for middleware, like Transaction Processing Monitors, which can now transparently (without any change in application code) monitor the status of transactions (by watching for the transaction status indicator in the ReadyForQuery protocol message). They can use the transaction ID from the NotificationResponse to open a watcher, and on seeing either an 'E' or 'I' payload in subsequent ReadyForQuery messages, close the watcher. On server crash, or other adverse events, they can then use the transaction IDs still being watched to check status of those transactions, and take appropriate actions, e.g. retry any aborted transactions. We cannot use the elog() mechanism for this notification because it is sensitive to the value of client_min_messages. Hence I used the NOTIFY infrastructure for this message. I understand that this usage violates some expectations as to how NOTIFY messages are supposed to behave (see [2] below), but I think these are acceptable violations; open to hearing if/why this might not be acceptable, and any possible alternatives. I'm not very familiar with the parallel workers infrastructure, so the patch is missing any consideration for those. Reviews welcome. [1]: subject was: Re: Disallow cancellation of waiting for synchronous replication thread: https://www.postgresql.org/message-id/flat/C1F7905E-5DB2-497D-ABCC-E14D4DEE506C%40yandex-team.ru [2]: At present, NotificationResponse can only be sent outside a transaction, and thus it will not occur in the middle of a command-response series, though it might occur just before ReadyForQuery. It is unwise to design frontend logic that assumes that, however. Good practice is to be able to accept NotificationResponse at any point in the protocol. [3]: See Emails section in https://commitfest.postgresql.org/33/3198/ The email [4] is considered a continuation of a previous thread, _and_ the 'Latest attachment' entry points to a different email, even though my email [4] contained a patch. [4]: https://www.postgresql.org/message-id/CABwTF4VS+HVm11XRE_Yv0vGmG=5kpYdx759RyJEp9F+fiLTU=Q@mail.gmail.com Best regards, -- Gurjeet Singh http://gurjeet.singh.im/
The proposed patch is attached. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/ On Wed, Jun 30, 2021 at 5:56 PM Gurjeet Singh <gurjeet@singh.im> wrote: > > (Re-sending this email, because the Commitfest app mistakenly [3] > considered previous email [4] to be part of the old thread, whereas it > should not be considered that way) > > I came across this thread [1] to disallow canceling a transaction not > yet confirmed by a synchronous replica. I think my proposed patch > might help that case as well, hence adding all involved in that thread > to BCC, for one-time notification. > > As mentioned in that thread, when sending a cancellation signal, the > client cannot be sure if the cancel signal was honored, and if the > transaction was cancelled successfully. In the attached patch, the > backend emits a NotificationResponse containing the current full > transaction id. It does so only if the relevant GUC is enabled, and > when the top-transaction is being assigned the ID. > > This information can be useful to the client, when: > i) it wants to cancel a transaction _after_ issuing a COMMIT, and > ii) it wants to check the status of its transaction that it sent > COMMIT for, but never received a response (perhaps because the server > crashed). > > Additionally, this information can be useful for middleware, like > Transaction Processing Monitors, which can now transparently (without > any change in application code) monitor the status of transactions (by > watching for the transaction status indicator in the ReadyForQuery > protocol message). They can use the transaction ID from the > NotificationResponse to open a watcher, and on seeing either an 'E' or > 'I' payload in subsequent ReadyForQuery messages, close the watcher. > On server crash, or other adverse events, they can then use the > transaction IDs still being watched to check status of those > transactions, and take appropriate actions, e.g. retry any aborted > transactions. > > We cannot use the elog() mechanism for this notification because it is > sensitive to the value of client_min_messages. Hence I used the NOTIFY > infrastructure for this message. I understand that this usage violates > some expectations as to how NOTIFY messages are supposed to behave > (see [2] below), but I think these are acceptable violations; open to > hearing if/why this might not be acceptable, and any possible > alternatives. > > I'm not very familiar with the parallel workers infrastructure, so the > patch is missing any consideration for those. > > Reviews welcome. > > [1]: subject was: Re: Disallow cancellation of waiting for synchronous > replication > thread: https://www.postgresql.org/message-id/flat/C1F7905E-5DB2-497D-ABCC-E14D4DEE506C%40yandex-team.ru > > [2]: > At present, NotificationResponse can only be sent outside a > transaction, and thus it will not occur in the middle of a > command-response series, though it might occur just before ReadyForQuery. > It is unwise to design frontend logic that assumes that, however. > Good practice is to be able to accept NotificationResponse at any > point in the protocol. > > [3]: > > See Emails section in https://commitfest.postgresql.org/33/3198/ > > The email [4] is considered a continuation of a previous thread, _and_ > the 'Latest attachment' entry points to a different email, even though > my email [4] contained a patch. > > [4]: https://www.postgresql.org/message-id/CABwTF4VS+HVm11XRE_Yv0vGmG=5kpYdx759RyJEp9F+fiLTU=Q@mail.gmail.com > > Best regards, > -- > Gurjeet Singh http://gurjeet.singh.im/
Attachment
On Thu, Jul 1, 2021 at 6:41 AM Gurjeet Singh <gurjeet@singh.im> wrote:
>
> The proposed patch is attached.
There is one compilation warning:
xid.c:165:1: warning: no previous prototype for
‘FullTransactionIdToStr’ [-Wmissing-prototypes]
165 | FullTransactionIdToStr(FullTransactionId fxid)
| ^~~~~~~~~~~~~~~~~~~~~~
There are few compilation issues in documentation:
/usr/bin/xmllint --path . --noout --valid postgres.sgml
protocol.sgml:1327: parser error : Opening and ending tag mismatch:
literal line 1322 and para
</para>
^
protocol.sgml:1339: parser error : Opening and ending tag mismatch:
literal line 1322 and sect2
</sect2>
^
protocol.sgml:1581: parser error : Opening and ending tag mismatch:
para line 1322 and sect1
</sect1>
^
protocol.sgml:7893: parser error : Opening and ending tag mismatch:
sect2 line 1322 and chapter
</chapter>
^
protocol.sgml:7894: parser error : chunk is not well balanced
^
postgres.sgml:253: parser error : Failure to process entity protocol
&protocol;
^
postgres.sgml:253: parser error : Entity 'protocol' not defined
&protocol;
^
Regards,
Vignesh
Greetings,
I simply tested it and it works well. But I got a compilation warning, should we move the definition of function FullTransactionIdToStr to the "transam.h"?
There is no royal road to learning.
HighGo Software Co.
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Hello This patch applies fine to master branch and the regression tests are passing. Regarding the parallel worker case, the AssignTransactionId() function is already handling that and it will error out ifIsParallelWorker() is true. In a normal case, this function is only called by the main backend, and the parallel workerswill synchronize the transaction ID when they are spawned and they will not call this function anyway. thank you Cary Huang ---------------- HighGo Software Canada www.highgo.ca
> On 22 Jul 2021, at 07:28, vignesh C <vignesh21@gmail.com> wrote: > > On Thu, Jul 1, 2021 at 6:41 AM Gurjeet Singh <gurjeet@singh.im> wrote: >> >> The proposed patch is attached. > > There is one compilation warning: > xid.c:165:1: warning: no previous prototype for > ‘FullTransactionIdToStr’ [-Wmissing-prototypes] > 165 | FullTransactionIdToStr(FullTransactionId fxid) > | ^~~~~~~~~~~~~~~~~~~~~~ > > There are few compilation issues in documentation: > /usr/bin/xmllint --path . --noout --valid postgres.sgml > protocol.sgml:1327: parser error : Opening and ending tag mismatch: > literal line 1322 and para > </para> > ^ > protocol.sgml:1339: parser error : Opening and ending tag mismatch: > literal line 1322 and sect2 > </sect2> > ^ > protocol.sgml:1581: parser error : Opening and ending tag mismatch: > para line 1322 and sect1 > </sect1> > ^ > protocol.sgml:7893: parser error : Opening and ending tag mismatch: > sect2 line 1322 and chapter > </chapter> > ^ > protocol.sgml:7894: parser error : chunk is not well balanced > > ^ > postgres.sgml:253: parser error : Failure to process entity protocol > &protocol; > ^ > postgres.sgml:253: parser error : Entity 'protocol' not defined > &protocol; > ^ The above compiler warning and documentation compilation errors haven't been addressed still, so I'm marking this patch Returned with Feedback. Please feel free to open a new entry for an updated patch. -- Daniel Gustafsson https://vmware.com/
On Wed, Jun 30, 2021 at 8:56 PM Gurjeet Singh <gurjeet@singh.im> wrote: > As mentioned in that thread, when sending a cancellation signal, the > client cannot be sure if the cancel signal was honored, and if the > transaction was cancelled successfully. In the attached patch, the > backend emits a NotificationResponse containing the current full > transaction id. It does so only if the relevant GUC is enabled, and > when the top-transaction is being assigned the ID. There's nothing to keep a client that wants this information from just using SELECT txid_current() to get it, so this doesn't really seem worth it to me. It's true that it could be convenient for someone not to need to issue an SQL query to get the information and instead just get it automatically, but I don't think that minor convenience is enough to justify a new feature of this type. Also, your 8-line documentation changes contains two spelling mistakes, and you've used // comments which are not project style in two places. It's a good idea to check over your patches for such simple mistakes before submitting them. -- Robert Haas EDB: http://www.enterprisedb.com