SI messages sent when excuting ROLLBACK PREPARED command
Hi
When reading the code FinishPreparedTransaction, I found that SI messages are sent
when executing ROLLBACK PREPARED command.
But according to AtEOXact_Inval function, we send the SI messages only when committing the transaction .
So, I think we needn't send SI messags when rollbacking the two-phase transaction.
Or Does it has something special because of two-phase transaction?
Regards
"liuhuailing@fujitsu.com" <liuhuailing@fujitsu.com> writes:
So, I think we needn't send SI messags when rollbacking the two-phase transaction.
Or Does it has something special because of two-phase transaction?
Hmmm, yeah, I think you're right. It probably doesn't make a big
difference in the real world --- anyone who's dependent on the
performance of 2PC rollbaxks is Doing It Wrong. But we'd have
already done LocalExecuteInvalidationMessage when getting out of
the prepared transaction, so no other SI invals should be needed.
regards, tom lane
Hi, tom
Thanks for your reply.
Hmmm, yeah, I think you're right. It probably doesn't make a big difference in the real world --- anyone who's dependent on the performance of 2PC rollbaxks is Doing It Wrong.
But we'd have already done LocalExecuteInvalidationMessage when getting out of the prepared transaction, so no other SI invals should be needed.
Yes, it does not make any error.
But for the beginner, when understanding the code, it may make confused.
And for the developer, when developing based on this code, it may make unnecessary handling added.
So, I think it is better to optimize the code.
Here is the patch.
Regards, liuhl
-----Original Message-----
From: Tom Lane <tgl@sss.pgh.pa.us>
Sent: Thursday, July 15, 2021 1:36 AM
To: Liu, Huailing/刘 怀玲 <liuhuailing@fujitsu.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: SI messages sent when excuting ROLLBACK PREPARED command
"liuhuailing@fujitsu.com" <liuhuailing@fujitsu.com> writes:
So, I think we needn't send SI messags when rollbacking the two-phase transaction.
Or Does it has something special because of two-phase transaction?
Hmmm, yeah, I think you're right. It probably doesn't make a big difference in the real world --- anyone who's dependent on the performance of 2PC rollbaxks is Doing It Wrong. But we'd have already done LocalExecuteInvalidationMessage when getting out of the prepared transaction, so no other SI invals should be needed.
regards, tom lane
Attachments:
twophase.patchapplication/octet-stream; name=twophase.patchDownload
diff --git a/before/src/backend/access/transam/twophase.c b/after/src/backend/access/transam/twophase.c
index 6d3efb4..b2b5192 100644
--- a/before/src/backend/access/transam/twophase.c
+++ b/after/src/backend/access/transam/twophase.c
@@ -1522,11 +1522,14 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
* Relcache init file invalidation requires processing both before and
* after we send the SI messages. See AtEOXact_Inval()
*/
- if (hdr->initfileinval)
- RelationCacheInitFilePreInvalidate();
- SendSharedInvalidMessages(invalmsgs, hdr->ninvalmsgs);
- if (hdr->initfileinval)
- RelationCacheInitFilePostInvalidate();
+ if (isCommit)
+ {
+ if (hdr->initfileinval)
+ RelationCacheInitFilePreInvalidate();
+ SendSharedInvalidMessages(invalmsgs, hdr->ninvalmsgs);
+ if (hdr->initfileinval)
+ RelationCacheInitFilePostInvalidate();
+ }
/*
* Acquire the two-phase lock. We want to work on the two-phase callbacks
Hi, tom
Hmmm, yeah, I think you're right. It probably doesn't make a big difference in
the real world --- anyone who's dependent on the performance of 2PC rollbaxks
is Doing It Wrong.But we'd have already done LocalExecuteInvalidationMessage when getting
out of the prepared transaction, so no other SI invals should be needed.
Yes, it does not make any error.But for the beginner, when understanding the code, it may make confused.
And for the developer, when developing based on this code, it may make
unnecessary handling added.So, I think it is better to optimize the code.
Here is the patch.
There was a problem with the before patch when testing.
So resubmit it.
Regards, Liu Huailing
Attachments:
0001-Disallow-sending-SI-messages-when-excuting-ROLLBACK.patchapplication/octet-stream; name=0001-Disallow-sending-SI-messages-when-excuting-ROLLBACK.patchDownload
From 71a086f283303f8f2e40d0ab3e7dc0e27f98cbd7 Mon Sep 17 00:00:00 2001
From: liuhl <liuhuailing@cn.fujitsu.com>
Date: Tue, 3 Aug 2021 17:04:55 +0800
Subject: [PATCH] Disallow sending SI messages when excuting ROLLBACK PREPARED
command
---
src/backend/access/transam/twophase.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6d3efb4..4c6e2be 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1522,11 +1522,15 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
* Relcache init file invalidation requires processing both before and
* after we send the SI messages. See AtEOXact_Inval()
*/
- if (hdr->initfileinval)
- RelationCacheInitFilePreInvalidate();
- SendSharedInvalidMessages(invalmsgs, hdr->ninvalmsgs);
- if (hdr->initfileinval)
- RelationCacheInitFilePostInvalidate();
+ if (isCommit)
+ {
+ if (hdr->initfileinval)
+ RelationCacheInitFilePreInvalidate();
+ SendSharedInvalidMessages(invalmsgs, hdr->ninvalmsgs);
+ if (hdr->initfileinval)
+ RelationCacheInitFilePostInvalidate();
+ }
+
/*
* Acquire the two-phase lock. We want to work on the two-phase callbacks
--
2.7.2.windows.1
On Tue, Aug 03, 2021 at 09:29:48AM +0000, liuhuailing@fujitsu.com wrote:
There was a problem with the before patch when testing.
So resubmit it.
FWIW, I see no problems with patch version 1 or 2, as long as you
apply patch version 1 with a command like patch -p2. One thing of
patch 2 is that git diff --check complains because of a whitespace.
Anyway, I also think that you are right here and that there is no need
to run this code path with ROLLBACK PREPARED. It is worth noting that
the point of Tom about local invalidation messages in PREPARE comes
from PostPrepare_Inval().
I would just tweak the comment block at the top of what's being
changed, as per the attached. Please let me know if there are any
objections.
--
Michael
Attachments:
twophase-3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6d3efb49a4..2156de187c 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1520,13 +1520,17 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
* Handle cache invalidation messages.
*
* Relcache init file invalidation requires processing both before and
- * after we send the SI messages. See AtEOXact_Inval()
+ * after we send the SI messages, only when committing. See
+ * AtEOXact_Inval().
*/
- if (hdr->initfileinval)
- RelationCacheInitFilePreInvalidate();
- SendSharedInvalidMessages(invalmsgs, hdr->ninvalmsgs);
- if (hdr->initfileinval)
- RelationCacheInitFilePostInvalidate();
+ if (isCommit)
+ {
+ if (hdr->initfileinval)
+ RelationCacheInitFilePreInvalidate();
+ SendSharedInvalidMessages(invalmsgs, hdr->ninvalmsgs);
+ if (hdr->initfileinval)
+ RelationCacheInitFilePostInvalidate();
+ }
/*
* Acquire the two-phase lock. We want to work on the two-phase callbacks
On Wed, Aug 11, 2021 at 03:14:11PM +0900, Michael Paquier wrote:
I would just tweak the comment block at the top of what's being
changed, as per the attached. Please let me know if there are any
objections.
And applied as of 710796f.
--
Michael
On Wed, Aug 11, 2021 at 03:14:11PM +0900, Michael Paquier wrote:
I would just tweak the comment block at the top of what's being
changed, as per the attached. Please let me know if there are any
objections.And applied as of 710796f.
Thanks for your comment and commit.
I've changed the patch's commit fest status to 'committed'.
https://commitfest.postgresql.org/34/3257/
Regards,
Liu