Hi Kuroda-san,
I did not apply these v12* patches, but I have diff'ed all of them
with the previous v11* patches and confirmed that all of my previous
review comments now seem to be addressed.
I don't have any more comments to make at this time. Thanks!
==
Kind Regards,
Peter Smith.
Fujitsu
Hi, Here are my remaining review comments for the latest v11* patches.
//
v11-0001
//
No changes. No comments.
//
v11-0002
//
==
doc/src/sgml/ref/alter_subscription.sgml
2.1.
ALTER SUBSCRIPTION ... SET (failover = on|off) and
- ALTER SUBSCRIPTION ...
Dear Peter,
> You wrote "Fixed" for that patch v9-0004 suggestion but I don't think
> anything was changed at all. Accidentally missed?
Sorry, I missed to do `git add` after the revision.
The change was also included in new patch [1].
[1]:
On Tue, May 14, 2024 at 10:03 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Peter,
>
...
> > 4.11.
> >
> > +# Alter the two_phase with the force_alter option. Apart from the the last
> > +# ALTER SUBSCRIPTION command, the command will abort the prepared
> > transaction
> > +# and succeed.
> >
> >
Hi Kuroda-san. Here are my review comments for latest v10* patches.
//
patch v10-0001
//
No changes. No comments.
//
patch v10-0002
//
==
Commit message
2.1.
Regarding the false->true case, the backend process alters the subtwophase to
Dear Peter,
Thanks for reviewing! New patch is available in [1].
> I'm having second thoughts about how these patches mention the option
> values "on|off". These are used in the ALTER SUBSCRIPTION document
> page for 'two_phase' and 'failover' parameters, and then those
> "on|off" get propagated
Hi Kuroda-san, Here are some review comments for all patches v9*
//
Patch v9-0001
//
There were no changes since v8-0001, so no comments.
//
Patch v9-0002
//
==
Commit Message
2.1.
Regarding the off->on case, the logical replication already has a
mechanism
Hi Kuroda-san,
I'm having second thoughts about how these patches mention the option
values "on|off". These are used in the ALTER SUBSCRIPTION document
page for 'two_phase' and 'failover' parameters, and then those
"on|off" get propagated to the code comments, error messages, and
tests...
Now I
Dear Peter,
Thanks for giving comments! New patch was posted in [1].
> 0.1 General - Patch name
>
> /SUBSCIRPTION/SUBSCRIPTION/
Fixed.
> ==
> 0.2 General - Apply
>
> FYI, there are whitespace warnings:
>
> git
> apply ../patches_misc/v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTI
>
Dear Peter,
Thanks for reviewing! Patch can be available in [1].
> ==
> src/sgml/ref/alter_subscription.sgml
>
> 1.
> +
> + The two_phase parameter can only be altered when
> the
> + subscription is disabled. When altering the parameter from
> on
> + to off, the backend
Hi, Here are some review comments for v8-0002.
==
Commit message
1.
Regarding the off->on case, the logical replication already has a mechanism for
it, so there is no need to do anything special for the on->off case; however,
we must connect to the publisher and expressly change the
Hi, Here are some comments for v8-0004
==
0.1 General - Patch name
/SUBSCIRPTION/SUBSCRIPTION/
==
0.2 General - Apply
FYI, there are whitespace warnings:
git apply
../patches_misc/v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTION-.-S.patch
Hi, Here are some review comments for v8-0003.
==
src/sgml/ref/alter_subscription.sgml
1.
+
+ The two_phase parameter can only be altered when the
+ subscription is disabled. When altering the parameter from
on
+ to off, the backend process checks prepared
+
Dear Peter,
>
> ==
> Commit message
>
> 1.
> A detailed commit message is needed to describe the purpose and
> details of this patch.
Added.
> ==
> doc/src/sgml/ref/alter_subscription.sgml
>
> 2. CREATE SUBSCRIPTION
>
> Shouldn't there be an entry for "force_alter" parameter in the
Dear Peter,
> Commit Message
>
> 1.
> The patch needs a commit message to describe the purpose and highlight
> any limitations and other details.
Added.
> ==
> doc/src/sgml/ref/alter_subscription.sgml
>
> 2.
> +
> +
> + The two_phase parameter can only be altered when
> the
> +
Dear Peter,
> ==
> Commit message
>
> 1.
> IIUC there is quite a lot of subtlety and details about why the slot
> option needs to be changed only when altering "true" to "false", but
> not when altering "false" to "true".
>
> It also should explain why PreventInTransactionBlock is only
Dear Peter,
Thanks for reviewing! The patch will be posted in the upcoming post.
> ==
> src/backend/access/transam/twophase.c
>
> 1. IsTwoPhaseTransactionGidForSubid
>
> +/*
> + * IsTwoPhaseTransactionGidForSubid
> + * Check whether the given GID is formed by TwoPhaseTransactionGid.
> + */
Hi, Here are some review comments for patch v7-0004
==
Commit message
1.
A detailed commit message is needed to describe the purpose and
details of this patch.
==
doc/src/sgml/ref/alter_subscription.sgml
2. CREATE SUBSCRIPTION
Shouldn't there be an entry for "force_alter" parameter in
Hi, Here are some review comments for the patch v7-0003.
==
Commit Message
1.
The patch needs a commit message to describe the purpose and highlight
any limitations and other details.
==
doc/src/sgml/ref/alter_subscription.sgml
2.
+
+
+ The two_phase parameter can only be
Hi, Here are some review comments for v7-0002
==
Commit message
1.
IIUC there is quite a lot of subtlety and details about why the slot
option needs to be changed only when altering "true" to "false", but
not when altering "false" to "true".
It also should explain why
Hi Kuroda-san,
Thanks for addressing most of my v6-0001 review comments.
Below are some minor follow-up comments for v7-0001.
==
src/backend/access/transam/twophase.c
1. IsTwoPhaseTransactionGidForSubid
+/*
+ * IsTwoPhaseTransactionGidForSubid
+ * Check whether the given GID is formed by
Dear Peter,
Thanks for reviewing! Here are updated patches.
I updated patches only for HEAD.
> ==
> Commit message
>
> 1.
> This patch allows user to alter two_phase option
>
> /allows user/allows the user/
>
> /to alter two_phase option/to alter the 'two_phase' option/
Fixed.
> ==
Hi, Here are some review comments for patch v6-0001
==
Commit message
1.
This patch allows user to alter two_phase option
/allows user/allows the user/
/to alter two_phase option/to alter the 'two_phase' option/
==
doc/src/sgml/ref/alter_subscription.sgml
2.
two_phase can be altered
Dear hackers,
Per recent commit (b29cbd3da), our patch needed to be rebased.
Here is an updated version.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
v6-0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIPT.patch
Description:
Dear Hayato,
On Monday, April 22, 2024 15:54 MSK, "Hayato Kuroda (Fujitsu)"
wrote:
> Dear Vitaly,
>
> > I looked at the patch and realized that I can't try it easily in the near
> > future
> > because the solution I'm working on is based on PG16 or earlier. This patch
> > is
> > not easily
> Dear Vitaly,
>
> > I looked at the patch and realized that I can't try it easily in the near
> > future
> > because the solution I'm working on is based on PG16 or earlier. This patch
> > is
> > not easily applicable to the older releases. I have to port my solution to
> > the
> > master,
Dear hackers,
> Looking at this a bit more, maybe rolling back all prepared transactions on
> the
> subscriber when toggling two_phase from true to false might not be desirable
> for the customer. Maybe we should have an option for customers to control
> whether transactions should be rolled
Dear Vitaly,
> I looked at the patch and realized that I can't try it easily in the near
> future
> because the solution I'm working on is based on PG16 or earlier. This patch is
> not easily applicable to the older releases. I have to port my solution to the
> master, which is not done yet.
We
On Thu, Apr 18, 2024 at 4:26 PM Ajin Cherian wrote:
>
> Attaching the patch for your review and comments. Big thanks to Kuroda-san
> for also working on the patch.
>
>
Looking at this a bit more, maybe rolling back all prepared transactions on
the subscriber when toggling two_phase from true to
On Tue, Apr 16, 2024 at 4:25 PM Amit Kapila wrote:
>
> >
>
> Can you please once consider the idea shared by me at [1] (One naive
> idea is that on the publisher .) to solve this problem?
>
> [1] -
>
On Tue, Apr 16, 2024 at 1:31 AM Давыдов Виталий
wrote:
> Dear All,
> Just interested, does anyone tried to reproduce the problem with slow
> catchup of twophase transactions (pgbench should be used with big number of
> clients)? I haven't seen any messages from anyone other that me that the
>
On Tue, Apr 16, 2024 at 7:48 AM Hayato Kuroda (Fujitsu)
wrote:
>
> > > FYI - We also considered the idea which walsender waits until all prepared
> > transactions
> > > are resolved before decoding and sending changes, but it did not work well
> > > - the restarted walsender sent only COMMIT
Dear Amit,
> > FYI - We also considered the idea which walsender waits until all prepared
> transactions
> > are resolved before decoding and sending changes, but it did not work well
> > - the restarted walsender sent only COMMIT PREPARED record for
> transactions which
> > have been prepared
Dear All,
On Wednesday, April 10, 2024 17:16 MSK, Давыдов Виталий
wrote:
Hi Amit, Ajin, All
Thank you for the patch and the responses. I apologize for my delayed answer
due to some curcumstances.
On Wednesday, April 10, 2024 14:18 MSK, Amit Kapila
wrote:
Vitaly, does the minimal solution
On Mon, Apr 15, 2024 at 1:28 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > Vitaly, does the minimal solution provided by the proposed patch
> > (Allow to alter two_phase option of a subscriber provided no
> > uncommitted
> > prepared transactions are pending on that subscription.) address your use
> >
Dear Amit,
> Vitaly, does the minimal solution provided by the proposed patch
> (Allow to alter two_phase option of a subscriber provided no
> uncommitted
> prepared transactions are pending on that subscription.) address your use
> case?
I think we do not have to handle cases which there are
Dear Amit,
> One naive idea is that on the publisher we can remember whether the
> prepare has been sent and if so then only send commit_prepared,
> otherwise send the entire transaction. On the subscriber-side, we
> somehow, need to ensure before applying the first change whether the
>
Hi Amit, Ajin, All
Thank you for the patch and the responses. I apologize for my delayed answer
due to some curcumstances.
On Wednesday, April 10, 2024 14:18 MSK, Amit Kapila
wrote:
Vitaly, does the minimal solution provided by the proposed patch (Allow to
alter two_phase option of a
On Fri, Apr 5, 2024 at 4:59 PM Ajin Cherian wrote:
>
> On Thu, Apr 4, 2024 at 4:38 PM Amit Kapila wrote:
>>
>>
>> I think this would probably be better than the current situation but
>> can we think of a solution to allow toggling the value of two_phase
>> even when prepared transactions are
On Thu, Apr 4, 2024 at 4:38 PM Amit Kapila wrote:
>
> I think this would probably be better than the current situation but
> can we think of a solution to allow toggling the value of two_phase
> even when prepared transactions are present? Can you please summarize
> the reason for the problems
On Thu, Apr 4, 2024 at 10:53 AM Ajin Cherian wrote:
>
> On Wed, Mar 6, 2024 at 1:29 AM Давыдов Виталий
> wrote:
>>
>> In usual work, the subscription has two_phase = on. I have to change this
>> option at catchup stage only, but this parameter can not be altered. There
>> was a patch proposal
On Wed, Mar 6, 2024 at 1:29 AM Давыдов Виталий
wrote:
> In usual work, the subscription has two_phase = on. I have to change this
> option at catchup stage only, but this parameter can not be altered. There
> was a patch proposal in past to implement altering of two_phase option, but
> it was
On Tue, Mar 5, 2024 at 7:59 PM Давыдов Виталий wrote:
>
> Thank you for the reply.
>
> On Tuesday, March 05, 2024 12:05 MSK, Heikki Linnakangas
> wrote:
>
>
> In a nutshell, this changes PREPARE TRANSACTION so that if
> synchronous_commit is 'off', the PREPARE TRANSACTION is not fsync'd to
>
Hi Heikki,
Thank you for the reply.
On Tuesday, March 05, 2024 12:05 MSK, Heikki Linnakangas
wrote:
In a nutshell, this changes PREPARE TRANSACTION so that if
synchronous_commit is 'off', the PREPARE TRANSACTION is not fsync'd to
disk. So if you crash after the PREPARE TRANSACTION has
On 29/02/2024 19:34, Давыдов Виталий wrote:
Dear All,
Consider, please, my patch for async commit for twophase transactions.
It can be applicable when catchup performance is not enought with
publication parameter twophase = on.
The key changes are:
* Use XLogSetAsyncXactLSN instead of
Dear All,
Consider, please, my patch for async commit for twophase transactions. It can
be applicable when catchup performance is not enought with publication
parameter twophase = on.
The key changes are:
* Use XLogSetAsyncXactLSN instead of XLogFlush as it is for usual
transactions. * In
Hi Amit,
On Tuesday, February 27, 2024 16:00 MSK, Amit Kapila
wrote:
As we do XLogFlush() at the time of prepare then why it is not available? OR
are you talking about this state after your idea/patch where you are trying to
make both Prepare and Commit_prepared records async?Right, I'm
On Tue, Feb 27, 2024 at 4:49 PM Давыдов Виталий
wrote:
>
> Thank you for your interest in the discussion!
>
> On Monday, February 26, 2024 16:24 MSK, Amit Kapila
> wrote:
>
>
> I think the reason is probably that when the WAL record for prepared is
> already flushed then what will be the idea
Hi Amit,
Thank you for your interest in the discussion!
On Monday, February 26, 2024 16:24 MSK, Amit Kapila
wrote:
I think the reason is probably that when the WAL record for prepared is already
flushed then what will be the idea of async commit here?I think, the idea of
async commit
On Fri, Feb 23, 2024 at 10:41 PM Давыдов Виталий
wrote:
>
> Amit Kapila wrote:
>
> I don't see we do anything specific for 2PC transactions to make them behave
> differently than regular transactions with respect to synchronous_commit
> setting. What makes you think so? Can you pin point the
Hi Amit,
Amit Kapila wrote:
I don't see we do anything specific for 2PC transactions to make them behave
differently than regular transactions with respect to synchronous_commit
setting. What makes you think so? Can you pin point the code you are referring
to?Yes, sure. The function
Hi Ajin,
Thank you for your feedback. Could you please try to increase the number of
clients (-c pgbench option) up to 20 or more? It seems, I forgot to specify it.
With best regards,
Vitaly Davydov On Fri, Feb 23, 2024 at 12:29 AM Давыдов Виталий
wrote:
Dear All,
I'd like to present and
On Fri, Feb 23, 2024 at 12:29 AM Давыдов Виталий
wrote:
> Dear All,
>
> I'd like to present and talk about a problem when 2PC transactions are
> applied quite slowly on a replica during logical replication. There is a
> master and a replica with established logical replication from the master
>
On Thu, Feb 22, 2024 at 6:59 PM Давыдов Виталий
wrote:
>
> I'd like to present and talk about a problem when 2PC transactions are
> applied quite slowly on a replica during logical replication. There is a
> master and a replica with established logical replication from the master to
> the
Dear All,
I'd like to present and talk about a problem when 2PC transactions are applied
quite slowly on a replica during logical replication. There is a master and a
replica with established logical replication from the master to the replica
with twophase = true. With some load level on the
55 matches
Mail list logo
