Horton

Thread

Commits

  1. Add option to enable two_phase commits via pg_create_logical_replication_slot.

  2. Avoid repeated decoding of prepared transactions after a restart.

  3. Allow decoding at prepare time in ReorderBuffer.

  1. repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-08T08:31:12Z

    Amit, Ajin, hackers,

testing logical decoding for two-phase transactions, I stumbled over 
what I first thought is a bug.  But comments seems to indicate this is 
intended behavior.  Could you please clarify or elaborate on the design 
decision?  Or indicate this indeed is a bug?

What puzzled me is that if a decoder is restarted in between the PREPARE 
and the COMMIT PREPARED, it repeats the entire transaction, despite it 
being already sent and potentially prepared on the receiving side.

In terms of `pg_logical_slot_get_changes` (and roughly from the 
prepare.sql test), this looks as follows:

                         data
----------------------------------------------------
  BEGIN
  table public.test_prepared1: INSERT: id[integer]:1
  PREPARE TRANSACTION 'test_prepared#1'
(3 rows)


This is the first delivery of the transaction.  After a restart, it will 
get all of the changes again, though:


                         data
----------------------------------------------------
  BEGIN
  table public.test_prepared1: INSERT: id[integer]:1
  PREPARE TRANSACTION 'test_prepared#1'
  COMMIT PREPARED 'test_prepared#1'
(4 rows)


I did not expect this, as any receiver that wants to have decoded 2PC is 
likely supporting some kind of two-phase commits itself.  And would 
therefore prepare the transaction upon its first reception.  Potentially 
receiving it a second time would require complicated filtering on every 
prepared transaction.

Furthermore, this clearly and unnecessarily holds back the restart LSN. 
Meaning even just a single prepared transaction can block advancing the 
restart LSN.  In most cases, these are short lived.  But on the other 
hand, there may be an arbitrary amount of other transactions in between 
a PREPARE and the corresponding COMMIT PREPARED in the WAL.  Not being 
able to advance over a prepared transaction seems like a bad thing in 
such a case.

I fail to see where this repetition would ever be useful.  Is there any 
reason for the current implementation that I'm missing or can this be 
corrected?  Thanks for elaborating.

Regards

Markus

  2. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-08T10:13:31Z

    On Mon, Feb 8, 2021 at 2:01 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
>
> Amit, Ajin, hackers,
>
> testing logical decoding for two-phase transactions, I stumbled over
> what I first thought is a bug.  But comments seems to indicate this is
> intended behavior.  Could you please clarify or elaborate on the design
> decision?  Or indicate this indeed is a bug?
>
> What puzzled me is that if a decoder is restarted in between the PREPARE
> and the COMMIT PREPARED, it repeats the entire transaction, despite it
> being already sent and potentially prepared on the receiving side.
>
> In terms of `pg_logical_slot_get_changes` (and roughly from the
> prepare.sql test), this looks as follows:
>
>                          data
> ----------------------------------------------------
>   BEGIN
>   table public.test_prepared1: INSERT: id[integer]:1
>   PREPARE TRANSACTION 'test_prepared#1'
> (3 rows)
>
>
> This is the first delivery of the transaction.  After a restart, it will
> get all of the changes again, though:
>
>
>                          data
> ----------------------------------------------------
>   BEGIN
>   table public.test_prepared1: INSERT: id[integer]:1
>   PREPARE TRANSACTION 'test_prepared#1'
>   COMMIT PREPARED 'test_prepared#1'
> (4 rows)
>
>
> I did not expect this, as any receiver that wants to have decoded 2PC is
> likely supporting some kind of two-phase commits itself.  And would
> therefore prepare the transaction upon its first reception.  Potentially
> receiving it a second time would require complicated filtering on every
> prepared transaction.
>

The reason was mentioned in ReorderBufferFinishPrepared(). See below
comments in code:
/*
 * It is possible that this transaction is not decoded at prepare time
 * either because by that time we didn't have a consistent snapshot or it
 * was decoded earlier but we have restarted. We can't distinguish between
 * those two cases so we send the prepare in both the cases and let
 * downstream decide whether to process or skip it. We don't need to
 * decode the xact for aborts if it is not done already.
 */
This won't happen when we replicate via pgoutput  (the patch for which
is still not committed) because it won't restart from a previous point
(unless the server needs to be restarted due to some reason) as you
are doing via logical decoding APIs. Now, we don't send again the
prepared xacts on repeated calls of pg_logical_slot_get_changes()
unless we encounter commit. This behavior is already explained in docs
[1] (See, Transaction Begin Prepare Callback in docs) and a way to
skip the prepare.

> Furthermore, this clearly and unnecessarily holds back the restart LSN.
> Meaning even just a single prepared transaction can block advancing the
> restart LSN.  In most cases, these are short lived.  But on the other
> hand, there may be an arbitrary amount of other transactions in between
> a PREPARE and the corresponding COMMIT PREPARED in the WAL.  Not being
> able to advance over a prepared transaction seems like a bad thing in
> such a case.
>

That anyway is true without this work as well where restart_lsn can be
advanced on commits. We haven't changed anything in that regard.

[1] - https://www.postgresql.org/docs/devel/logicaldecoding-output-plugin.html


-- 
With Regards,
Amit Kapila.

  3. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-08T15:06:30Z

    Hello Amit,

thanks for your very quick response.

On 08.02.21 11:13, Amit Kapila wrote:
> /*
>   * It is possible that this transaction is not decoded at prepare time
>   * either because by that time we didn't have a consistent snapshot or it
>   * was decoded earlier but we have restarted. We can't distinguish between
>   * those two cases so we send the prepare in both the cases and let
>   * downstream decide whether to process or skip it. We don't need to
>   * decode the xact for aborts if it is not done already.
>   */

The way I read the surrounding code, the only case a 2PC transaction 
does not get decoded a prepare time is if the transaction is empty.  Or 
are you aware of any other situation that might currently happen?

> (unless the server needs to be restarted due to some reason)

Right, the repetition occurs only after a restart of the walsender in 
between a prepare and a commit prepared record.

> That anyway is true without this work as well where restart_lsn can be
> advanced on commits. We haven't changed anything in that regard.

I did not mean to blame the patch, but merely try to understand some of 
the design decisions behind it.

And as I just learned, even if we managed to avoid the repetition, a 
restarted walsender still needs to see prepared transactions as 
in-progress in its snapshots.  So we cannot move forward the restart_lsn 
to after a prepare record (until the final commit or rollback is consumed).

Best Regards

Markus

  4. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-09T03:02:42Z

    On Mon, Feb 8, 2021 at 8:36 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
>
> Hello Amit,
>
> thanks for your very quick response.
>
> On 08.02.21 11:13, Amit Kapila wrote:
> > /*
> >   * It is possible that this transaction is not decoded at prepare time
> >   * either because by that time we didn't have a consistent snapshot or it
> >   * was decoded earlier but we have restarted. We can't distinguish between
> >   * those two cases so we send the prepare in both the cases and let
> >   * downstream decide whether to process or skip it. We don't need to
> >   * decode the xact for aborts if it is not done already.
> >   */
>
> The way I read the surrounding code, the only case a 2PC transaction
> does not get decoded a prepare time is if the transaction is empty.  Or
> are you aware of any other situation that might currently happen?
>

We also skip decoding at prepare time if we haven't reached a
consistent snapshot by that time. See below code in DecodePrepare().
DecodePrepare()
{
..
/* We can't start streaming unless a consistent state is reached. */
if (SnapBuildCurrentState(builder) < SNAPBUILD_CONSISTENT)
{
ReorderBufferSkipPrepare(ctx->reorder, xid);
return;
}
..
}

There are other reasons as well like the output plugin doesn't want to
allow decoding at prepare time but I don't think those are relevant to
the discussion here.

> > (unless the server needs to be restarted due to some reason)
>
> Right, the repetition occurs only after a restart of the walsender in
> between a prepare and a commit prepared record.
>
> > That anyway is true without this work as well where restart_lsn can be
> > advanced on commits. We haven't changed anything in that regard.
>
> I did not mean to blame the patch, but merely try to understand some of
> the design decisions behind it.
>
> And as I just learned, even if we managed to avoid the repetition, a
> restarted walsender still needs to see prepared transactions as
> in-progress in its snapshots.  So we cannot move forward the restart_lsn
> to after a prepare record (until the final commit or rollback is consumed).
>

Right and say if we forget the prepared transactions and move forward
with restart_lsn once we get the prepare for any transaction. Then we
will open up a window where we haven't actually sent the prepared xact
because of say "snapshot has not yet reached consistent state" and we
have moved the restart_lsn. Then later when we get the commit
corresponding to the prepared transaction by which time say the
"snapshot has reached consistent state" then we will miss sending the
transaction contents and prepare for it. I think for such reasons we
allow restart_lsn to moved only once the transaction is finished
(committed or rolled back).

-- 
With Regards,
Amit Kapila.

  5. Re: repeated decoding of prepared transactions

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2021-02-09T05:59:14Z

    On Tue, Feb 9, 2021 at 8:32 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 8:36 PM Markus Wanner
> <markus.wanner@enterprisedb.com> wrote:
> >
> > Hello Amit,
> >
> > thanks for your very quick response.
> >
> > On 08.02.21 11:13, Amit Kapila wrote:
> > > /*
> > >   * It is possible that this transaction is not decoded at prepare time
> > >   * either because by that time we didn't have a consistent snapshot or it
> > >   * was decoded earlier but we have restarted. We can't distinguish between
> > >   * those two cases so we send the prepare in both the cases and let
> > >   * downstream decide whether to process or skip it. We don't need to
> > >   * decode the xact for aborts if it is not done already.
> > >   */
> >
> > The way I read the surrounding code, the only case a 2PC transaction
> > does not get decoded a prepare time is if the transaction is empty.  Or
> > are you aware of any other situation that might currently happen?
> >
>
> We also skip decoding at prepare time if we haven't reached a
> consistent snapshot by that time. See below code in DecodePrepare().
> DecodePrepare()
> {
> ..
> /* We can't start streaming unless a consistent state is reached. */
> if (SnapBuildCurrentState(builder) < SNAPBUILD_CONSISTENT)
> {
> ReorderBufferSkipPrepare(ctx->reorder, xid);
> return;
> }
> ..
> }

Can you please provide steps which can lead to this situation? If
there is an earlier discussion which has example scenarios, please
point us to the relevant thread.

If we are not sending PREPARED transactions that's fine, but sending
the same prepared transaction as many times as the WAL sender is
restarted between sending prepare and commit prepared is a waste of
network bandwidth. The wastage is proportional to the amount of
changes in the transaction and number of such transactions themselves.
Also this will cause performance degradation. So if we can avoid
resending prepared transactions twice that will help.

-- 
Best Wishes,
Ashutosh Bapat

  6. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-09T06:27:29Z

    On Tue, Feb 9, 2021 at 4:59 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

> Can you please provide steps which can lead to this situation? If
> there is an earlier discussion which has example scenarios, please
> point us to the relevant thread.
>
> If we are not sending PREPARED transactions that's fine, but sending
> the same prepared transaction as many times as the WAL sender is
> restarted between sending prepare and commit prepared is a waste of
> network bandwidth. The wastage is proportional to the amount of
> changes in the transaction and number of such transactions themselves.
> Also this will cause performance degradation. So if we can avoid
> resending prepared transactions twice that will help.

One of this scenario is explained in the test case in

postgres/contrib/test_decoding/specs/twophase_snapshot.spec

regards,
Ajin Cherian
Fujitsu Australia

  7. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-09T11:57:12Z

    On Tue, Feb 9, 2021 at 11:29 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Feb 9, 2021 at 8:32 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Feb 8, 2021 at 8:36 PM Markus Wanner
> > <markus.wanner@enterprisedb.com> wrote:
> > >
> > > Hello Amit,
> > >
> > > thanks for your very quick response.
> > >
> > > On 08.02.21 11:13, Amit Kapila wrote:
> > > > /*
> > > >   * It is possible that this transaction is not decoded at prepare time
> > > >   * either because by that time we didn't have a consistent snapshot or it
> > > >   * was decoded earlier but we have restarted. We can't distinguish between
> > > >   * those two cases so we send the prepare in both the cases and let
> > > >   * downstream decide whether to process or skip it. We don't need to
> > > >   * decode the xact for aborts if it is not done already.
> > > >   */
> > >
> > > The way I read the surrounding code, the only case a 2PC transaction
> > > does not get decoded a prepare time is if the transaction is empty.  Or
> > > are you aware of any other situation that might currently happen?
> > >
> >
> > We also skip decoding at prepare time if we haven't reached a
> > consistent snapshot by that time. See below code in DecodePrepare().
> > DecodePrepare()
> > {
> > ..
> > /* We can't start streaming unless a consistent state is reached. */
> > if (SnapBuildCurrentState(builder) < SNAPBUILD_CONSISTENT)
> > {
> > ReorderBufferSkipPrepare(ctx->reorder, xid);
> > return;
> > }
> > ..
> > }
>
> Can you please provide steps which can lead to this situation?
>

Ajin has already shared the example with you.

> If
> there is an earlier discussion which has example scenarios, please
> point us to the relevant thread.
>

It started in the email [1] and from there you can read later emails
to know more about this.

> If we are not sending PREPARED transactions that's fine,
>

Hmm, I am not sure if that is fine because if the output plugin sets
the two-phase-commit option, it would expect all prepared xacts to
arrive not some only some of them.

> but sending
> the same prepared transaction as many times as the WAL sender is
> restarted between sending prepare and commit prepared is a waste of
> network bandwidth.
>

I think similar happens without any of the work done in PG-14 as well
if we restart the apply worker before the commit completes on the
subscriber. After the restart, we will send the start_decoding_at
point based on some previous commit which will make publisher send the
entire transaction again. I don't think restart of WAL sender or WAL
receiver is such a common thing. It can only happen due to some bug in
code or user wishes to stop the nodes or some crash happened.

[1] - https://www.postgresql.org/message-id/CAA4eK1%2Bd3gzCyzsYjt1m6sfGf_C_uFmo9JK%3D3Wafp6yR8Mg8uQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  8. Re: repeated decoding of prepared transactions

    Robert Haas <robertmhaas@gmail.com> — 2021-02-09T18:38:15Z

    On Tue, Feb 9, 2021 at 6:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I think similar happens without any of the work done in PG-14 as well
> if we restart the apply worker before the commit completes on the
> subscriber. After the restart, we will send the start_decoding_at
> point based on some previous commit which will make publisher send the
> entire transaction again. I don't think restart of WAL sender or WAL
> receiver is such a common thing. It can only happen due to some bug in
> code or user wishes to stop the nodes or some crash happened.

Really? My impression is that the logical replication protocol is
supposed to be designed in such a way that once a transaction is
successfully confirmed, it won't be sent again. Now if something is
not confirmed then it has to be sent again. But if it is confirmed
then it shouldn't happen.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-10T02:32:17Z

    On Wed, Feb 10, 2021 at 12:08 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Feb 9, 2021 at 6:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I think similar happens without any of the work done in PG-14 as well
> > if we restart the apply worker before the commit completes on the
> > subscriber. After the restart, we will send the start_decoding_at
> > point based on some previous commit which will make publisher send the
> > entire transaction again. I don't think restart of WAL sender or WAL
> > receiver is such a common thing. It can only happen due to some bug in
> > code or user wishes to stop the nodes or some crash happened.
>
> Really? My impression is that the logical replication protocol is
> supposed to be designed in such a way that once a transaction is
> successfully confirmed, it won't be sent again. Now if something is
> not confirmed then it has to be sent again. But if it is confirmed
> then it shouldn't happen.
>

If by successfully confirmed, you mean that once the subscriber node
has received, it won't be sent again then as far as I know that is not
true. We rely on the flush location sent by the subscriber to advance
the decoding locations. We update the flush locations after we apply
the transaction's commit successfully. Also, after the restart, we use
the replication origin's last flush location as a point from where we
need the transactions and the origin's progress is updated at commit
time.

OTOH, If by successfully confirmed, you mean that once the subscriber
has applied the complete transaction (including commit), then you are
right that it won't be sent again.

-- 
With Regards,
Amit Kapila.

  10. Re: repeated decoding of prepared transactions

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2021-02-10T04:43:48Z

    On Wed, Feb 10, 2021 at 8:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Wed, Feb 10, 2021 at 12:08 AM Robert Haas <robertmhaas@gmail.com>
> wrote:
> >
> > On Tue, Feb 9, 2021 at 6:57 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > I think similar happens without any of the work done in PG-14 as well
> > > if we restart the apply worker before the commit completes on the
> > > subscriber. After the restart, we will send the start_decoding_at
> > > point based on some previous commit which will make publisher send the
> > > entire transaction again. I don't think restart of WAL sender or WAL
> > > receiver is such a common thing. It can only happen due to some bug in
> > > code or user wishes to stop the nodes or some crash happened.
> >
> > Really? My impression is that the logical replication protocol is
> > supposed to be designed in such a way that once a transaction is
> > successfully confirmed, it won't be sent again. Now if something is
> > not confirmed then it has to be sent again. But if it is confirmed
> > then it shouldn't happen.
> >
>
> If by successfully confirmed, you mean that once the subscriber node
> has received, it won't be sent again then as far as I know that is not
> true. We rely on the flush location sent by the subscriber to advance
> the decoding locations. We update the flush locations after we apply
> the transaction's commit successfully. Also, after the restart, we use
> the replication origin's last flush location as a point from where we
> need the transactions and the origin's progress is updated at commit
> time.
>
> OTOH, If by successfully confirmed, you mean that once the subscriber
> has applied the complete transaction (including commit), then you are
> right that it won't be sent again.
>

I think we need to treat a prepared transaction slightly different from an
uncommitted transaction when sending downstream. We need to send a whole
uncommitted transaction downstream again because previously applied changes
must have been aborted and hence lost by the downstream and thus it needs
to get all of those again. But when a downstream prepares a transaction,
even if it's not committed, those changes are not lost even after restart
of a walsender. If the downstream confirms that it has "flushed" PREPARE,
there is no need to send all the changes again.

--
Best Wishes,
Ashutosh

  11. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-10T06:14:59Z

    On Wed, Feb 10, 2021 at 3:43 PM Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:


> I think we need to treat a prepared transaction slightly different from an uncommitted transaction when sending downstream. We need to send a whole uncommitted transaction downstream again because previously applied changes must have been aborted and hence lost by the downstream and thus it needs to get all of those again. But when a downstream prepares a transaction, even if it's not committed, those changes are not lost even after restart of a walsender. If the downstream confirms that it has "flushed" PREPARE, there is no need to send all the changes again.

But the other side of the problem is that ,without this, if the
prepared transaction is prior to a consistent snapshot when decoding
starts/restarts, then only the "commit prepared" is sent to downstream
(as seen in the test scenario I shared above), and downstream has to
error away the commit prepared because it does not have the
corresponding prepared transaction. We did not find an easy way to
distinguish between these two scenarios for prepared transactions.
a. A consistent snapshot being formed in between a prepare and a
commit prepared for the first time.
b. Decoder restarting between a prepare and a commit prepared.

For plugins to be able to handle this, we have added a special
callback "Begin Prepare" as explained in [1] section 49.6.4.10

"The required begin_prepare_cb callback is called whenever the start
of a prepared transaction has been decoded. The gid field, which is
part of the txn parameter can be used in this callback to check if the
plugin has already received this prepare in which case it can skip the
remaining changes of the transaction. This can only happen if the user
restarts the decoding after receiving the prepare for a transaction
but before receiving the commit prepared say because of some error."

The pgoutput plugin is also being updated to be able to handle this
situation of duplicate prepared transactions.

regards,
Ajin Cherian
Fujitsu Australia

  12. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-10T06:32:20Z

    On Wed, Feb 10, 2021 at 11:45 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Feb 10, 2021 at 3:43 PM Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>
>
> > I think we need to treat a prepared transaction slightly different from an uncommitted transaction when sending downstream. We need to send a whole uncommitted transaction downstream again because previously applied changes must have been aborted and hence lost by the downstream and thus it needs to get all of those again. But when a downstream prepares a transaction, even if it's not committed, those changes are not lost even after restart of a walsender. If the downstream confirms that it has "flushed" PREPARE, there is no need to send all the changes again.
>
> But the other side of the problem is that ,without this, if the
> prepared transaction is prior to a consistent snapshot when decoding
> starts/restarts, then only the "commit prepared" is sent to downstream
> (as seen in the test scenario I shared above), and downstream has to
> error away the commit prepared because it does not have the
> corresponding prepared transaction.
>

I think it is not only simple error handling, it is required for
data-consistency. We need to send the transactions whose commits are
encountered after a consistent snapshot is reached.

-- 
With Regards,
Amit Kapila.

  13. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-10T08:10:19Z

    On 10.02.21 07:32, Amit Kapila wrote:
> On Wed, Feb 10, 2021 at 11:45 AM Ajin Cherian <itsajin@gmail.com> wrote:
>> But the other side of the problem is that ,without this, if the
>> prepared transaction is prior to a consistent snapshot when decoding
>> starts/restarts, then only the "commit prepared" is sent to downstream
>> (as seen in the test scenario I shared above), and downstream has to
>> error away the commit prepared because it does not have the
>> corresponding prepared transaction.
> 
> I think it is not only simple error handling, it is required for
> data-consistency. We need to send the transactions whose commits are
> encountered after a consistent snapshot is reached.

I'm with Ashutosh here.  If a replica is properly in sync, it knows 
about prepared transactions and all the gids of those.  Sending the 
transactional changes and the prepare again is inconsistent.

The point of a two-phase transaction is to have two phases.  An output 
plugin must have the chance of treating them as independent events. 
Once a PREPARE is confirmed, it must not be sent again.  Even if the 
transaction is still in-progress and its changes are not yet visible on 
the origin node.

Regards

Markus

  14. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-10T10:10:37Z

    On Wed, Feb 10, 2021 at 1:40 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
>
> On 10.02.21 07:32, Amit Kapila wrote:
> > On Wed, Feb 10, 2021 at 11:45 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >> But the other side of the problem is that ,without this, if the
> >> prepared transaction is prior to a consistent snapshot when decoding
> >> starts/restarts, then only the "commit prepared" is sent to downstream
> >> (as seen in the test scenario I shared above), and downstream has to
> >> error away the commit prepared because it does not have the
> >> corresponding prepared transaction.
> >
> > I think it is not only simple error handling, it is required for
> > data-consistency. We need to send the transactions whose commits are
> > encountered after a consistent snapshot is reached.
>
> I'm with Ashutosh here.  If a replica is properly in sync, it knows
> about prepared transactions and all the gids of those.  Sending the
> transactional changes and the prepare again is inconsistent.
>
> The point of a two-phase transaction is to have two phases.  An output
> plugin must have the chance of treating them as independent events.
>

I am not sure I understand what problem you are facing to deal with
this in the output plugin, it is explained in docs and Ajin also
pointed out the same. Ajin and I have explained to you the design
constraints on the publisher-side due to which we have done this way.
Do you have any better ideas to deal with this?

-- 
With Regards,
Amit Kapila.

  15. Re: repeated decoding of prepared transactions

    Robert Haas <robertmhaas@gmail.com> — 2021-02-10T15:06:35Z

    On Tue, Feb 9, 2021 at 9:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> If by successfully confirmed, you mean that once the subscriber node
> has received, it won't be sent again then as far as I know that is not
> true. We rely on the flush location sent by the subscriber to advance
> the decoding locations. We update the flush locations after we apply
> the transaction's commit successfully. Also, after the restart, we use
> the replication origin's last flush location as a point from where we
> need the transactions and the origin's progress is updated at commit
> time.
>
> OTOH, If by successfully confirmed, you mean that once the subscriber
> has applied the complete transaction (including commit), then you are
> right that it won't be sent again.

I meant - once the subscriber has advanced the flush location.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  16. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-11T10:36:50Z

    On Mon, Feb 8, 2021 at 2:01 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
>
> I did not expect this, as any receiver that wants to have decoded 2PC is
> likely supporting some kind of two-phase commits itself.  And would
> therefore prepare the transaction upon its first reception.  Potentially
> receiving it a second time would require complicated filtering on every
> prepared transaction.
>

I would like to bring one other scenario to your notice where you
might want to handle things differently for prepared transactions on
the plugin side. Assume we have multiple publications (for simplicity
say 2) on publisher with corresponding subscriptions (say 2, each
corresponding to one publication on the publisher). When a user
performs a transaction on a publisher that involves the tables from
both publications, on the subscriber-side, we do that work via two
different transactions, corresponding to each subscription. But, we
need some way to deal with prepared xacts because they need GID and we
can't use the same GID for both subscriptions. Please see the detailed
example and one idea to deal with the same in the main thread[1]. It
would be really helpful if you or others working on the plugin side
can share your opinion on the same.

Now, coming back to the restart case where the prepared transaction
can be sent again by the publisher. I understand yours and others
point that we should not send prepared transaction if there is a
restart between prepare and commit but there are reasons why we have
done that way and I am open to your suggestions. I'll once again try
to explain the exact case to you which is not very apparent. The basic
idea is that we ship/replay all transactions where commit happens
after the snapshot has a consistent state (SNAPBUILD_CONSISTENT), see
atop snapbuild.c for details. Now, for transactions where prepare is
before snapshot state SNAPBUILD_CONSISTENT and commit prepared is
after SNAPBUILD_CONSISTENT, we need to send the entire transaction
including prepare at the commit time. One might think it is quite easy
to detect that, basically if we skip prepare when the snapshot state
was not SNAPBUILD_CONSISTENT, then mark a flag in ReorderBufferTxn and
use the same to detect during commit and accordingly take the decision
to send prepare but unfortunately it is not that easy. There is always
a chance that on restart we reuse the snapshot serialized by some
other Walsender at a location prior to Prepare and if that happens
then this time the prepare won't be skipped due to snapshot state
(SNAPBUILD_CONSISTENT) but due to start_decodint_at point (considering
we have already shipped some of the later commits but not prepare).
Now, this will actually become the same situation where the restart
has happened after we have sent the prepare but not commit. This is
the reason we have to resend the prepare when the subscriber restarts
between prepare and commit.

You can reproduce the case where we can't distinguish between two
situations by using the test case in twophase_snapshot.spec and
additionally starting a separate session via the debugger. So, the
steps in the test case are as below:

"s2b" "s2txid" "s1init" "s3b" "s3txid" "s2c" "s2b" "s2insert" "s2p"
"s3c" "s1insert" "s1start" "s2cp" "s1start"

Define new steps as

"s4init" {SELECT 'init' FROM
pg_create_logical_replication_slot('isolation_slot_1',
'test_decoding');}
"s4start" {SELECT data  FROM
pg_logical_slot_get_changes('isolation_slot_1', NULL, NULL,
'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
'1');}

The first thing we need to do is s4init and stop the debugger in
SnapBuildProcessRunningXacts. Now perform steps from 's2b' till first
's1start' in twophase_snapshot.spec. Then continue in the s4 session
and perform s4start. After this, if you debug (or add the logs) the
second s1start, you will notice that we are skipping prepare not
because of inconsistent snapshot but a forward location in
start_decoding_at. If you don't involve session-4, then it will always
skip prepare due to an inconsistent snapshot state. This involves a
debugger so not easy to write an automated test for it.

I have used a bit tricky scenario to explain this but not sure if
there was any other simpler way.

[1] - https://www.postgresql.org/message-id/CAA4eK1%2BLvkeX%3DB3xon7RcBwD4CVaFSryPj3pTBAALrDxQVPDwA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  17. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-11T12:16:49Z

    Hello Amit,

thanks a lot for your extensive explanation and examples, I appreciate 
this very much.  I'll need to think this through and see how we can make 
this work for us.

Best Regards

Markus

  18. Re: repeated decoding of prepared transactions

    Robert Haas <robertmhaas@gmail.com> — 2021-02-11T19:40:08Z

    On Thu, Feb 11, 2021 at 5:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> to explain the exact case to you which is not very apparent. The basic
> idea is that we ship/replay all transactions where commit happens
> after the snapshot has a consistent state (SNAPBUILD_CONSISTENT), see
> atop snapbuild.c for details. Now, for transactions where prepare is
> before snapshot state SNAPBUILD_CONSISTENT and commit prepared is
> after SNAPBUILD_CONSISTENT, we need to send the entire transaction
> including prepare at the commit time.

This might be a dumb question, but: why?

Is this because the effects of the prepared transaction might
otherwise be included neither in the initial synchronization of the
data nor in any subsequently decoded transaction, thus leaving the
replica out of sync?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  19. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-12T10:33:11Z

    On Fri, Feb 12, 2021 at 1:10 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Feb 11, 2021 at 5:37 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > to explain the exact case to you which is not very apparent. The basic
> > idea is that we ship/replay all transactions where commit happens
> > after the snapshot has a consistent state (SNAPBUILD_CONSISTENT), see
> > atop snapbuild.c for details. Now, for transactions where prepare is
> > before snapshot state SNAPBUILD_CONSISTENT and commit prepared is
> > after SNAPBUILD_CONSISTENT, we need to send the entire transaction
> > including prepare at the commit time.
>
> This might be a dumb question, but: why?
>
> Is this because the effects of the prepared transaction might
> otherwise be included neither in the initial synchronization of the
> data nor in any subsequently decoded transaction, thus leaving the
> replica out of sync?
>

Yes.

-- 
With Regards,
Amit Kapila.

  20. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-13T16:32:37Z

    Hi,

On 2021-02-10 08:02:17 +0530, Amit Kapila wrote:
> On Wed, Feb 10, 2021 at 12:08 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Tue, Feb 9, 2021 at 6:57 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > I think similar happens without any of the work done in PG-14 as well
> > > if we restart the apply worker before the commit completes on the
> > > subscriber. After the restart, we will send the start_decoding_at
> > > point based on some previous commit which will make publisher send the
> > > entire transaction again. I don't think restart of WAL sender or WAL
> > > receiver is such a common thing. It can only happen due to some bug in
> > > code or user wishes to stop the nodes or some crash happened.
> >
> > Really? My impression is that the logical replication protocol is
> > supposed to be designed in such a way that once a transaction is
> > successfully confirmed, it won't be sent again. Now if something is
> > not confirmed then it has to be sent again. But if it is confirmed
> > then it shouldn't happen.

Correct.


> If by successfully confirmed, you mean that once the subscriber node
> has received, it won't be sent again then as far as I know that is not
> true. We rely on the flush location sent by the subscriber to advance
> the decoding locations. We update the flush locations after we apply
> the transaction's commit successfully. Also, after the restart, we use
> the replication origin's last flush location as a point from where we
> need the transactions and the origin's progress is updated at commit
> time.

That's not quite right. Yes, the flush location isn't guaranteed to be
updated at that point, but a replication client will send the last
location they've received and successfully processed, and that has to
*guarantee* that they won't receive anything twice, or miss
something. Otherwise you've broken the protocol.

Greetings,

Andres Freund

  21. Re: repeated decoding of prepared transactions

    Petr Jelinek <petr.jelinek@enterprisedb.com> — 2021-02-13T16:37:29Z

    > 
> On 13 Feb 2021, at 17:32, Andres Freund <andres@anarazel.de> wrote:
> 
> Hi,
> 
> On 2021-02-10 08:02:17 +0530, Amit Kapila wrote:
>> On Wed, Feb 10, 2021 at 12:08 AM Robert Haas <robertmhaas@gmail.com> wrote:
>>> 
>> If by successfully confirmed, you mean that once the subscriber node
>> has received, it won't be sent again then as far as I know that is not
>> true. We rely on the flush location sent by the subscriber to advance
>> the decoding locations. We update the flush locations after we apply
>> the transaction's commit successfully. Also, after the restart, we use
>> the replication origin's last flush location as a point from where we
>> need the transactions and the origin's progress is updated at commit
>> time.
> 
> That's not quite right. Yes, the flush location isn't guaranteed to be
> updated at that point, but a replication client will send the last
> location they've received and successfully processed, and that has to
> *guarantee* that they won't receive anything twice, or miss
> something. Otherwise you've broken the protocol.
> 

Agreed, if we relied purely on flush location of a slot, there would be no need for origins to track the lsn. AFAIK this is exactly why origins are Wal logged along with transaction, it allows us to guarantee never getting anything that has beed durably written.

—
Petr

  22. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-13T16:53:23Z

    Hi,

On 2021-02-13 17:37:29 +0100, Petr Jelinek wrote:
> Agreed, if we relied purely on flush location of a slot, there would
> be no need for origins to track the lsn.

And we would be latency bound replicating transactions, which'd not be
fun for single-insert ones for example...


> AFAIK this is exactly why origins are Wal logged along with
> transaction, it allows us to guarantee never getting anything that has
> beed durably written.

I think you'd need something like origins in that case, because
something could still go wrong before the other side has received the
flush (network disconnect, primary crash, ...).

Greetings,

Andres Freund

  23. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-15T03:54:13Z

    On Sat, Feb 13, 2021 at 10:23 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2021-02-13 17:37:29 +0100, Petr Jelinek wrote:
>
> > AFAIK this is exactly why origins are Wal logged along with
> > transaction, it allows us to guarantee never getting anything that has
> > beed durably written.
>
> I think you'd need something like origins in that case, because
> something could still go wrong before the other side has received the
> flush (network disconnect, primary crash, ...).
>

We are already using origins in apply-worker to guarantee that and
with each commit, the origin's lsn location is also WAL-logged. That
helps us to send the start location for a slot after the restart. As
far as I understand this is how it works from the apply-worker side. I
am not sure if I am missing something here?

-- 
With Regards,
Amit Kapila.

  24. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-16T04:13:15Z

    On Thu, Feb 11, 2021 at 4:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 2:01 PM Markus Wanner
> <markus.wanner@enterprisedb.com> wrote:
> >
> Now, coming back to the restart case where the prepared transaction
> can be sent again by the publisher. I understand yours and others
> point that we should not send prepared transaction if there is a
> restart between prepare and commit but there are reasons why we have
> done that way and I am open to your suggestions. I'll once again try
> to explain the exact case to you which is not very apparent. The basic
> idea is that we ship/replay all transactions where commit happens
> after the snapshot has a consistent state (SNAPBUILD_CONSISTENT), see
> atop snapbuild.c for details. Now, for transactions where prepare is
> before snapshot state SNAPBUILD_CONSISTENT and commit prepared is
> after SNAPBUILD_CONSISTENT, we need to send the entire transaction
> including prepare at the commit time. One might think it is quite easy
> to detect that, basically if we skip prepare when the snapshot state
> was not SNAPBUILD_CONSISTENT, then mark a flag in ReorderBufferTxn and
> use the same to detect during commit and accordingly take the decision
> to send prepare but unfortunately it is not that easy. There is always
> a chance that on restart we reuse the snapshot serialized by some
> other Walsender at a location prior to Prepare and if that happens
> then this time the prepare won't be skipped due to snapshot state
> (SNAPBUILD_CONSISTENT) but due to start_decodint_at point (considering
> we have already shipped some of the later commits but not prepare).
> Now, this will actually become the same situation where the restart
> has happened after we have sent the prepare but not commit. This is
> the reason we have to resend the prepare when the subscriber restarts
> between prepare and commit.
>

After further thinking on this problem and some off-list discussions
with Ajin, there appears to be another way to solve the above problem
by which we can avoid resending the prepare after restart if it has
already been processed by the subscriber. The main reason why we were
not able to distinguish between the two cases ((a) prepare happened
before SNAPBUILD_CONSISTENT state but commit prepared happened after
we reach SNAPBUILD_CONSISTENT state and (b) prepare is already
decoded, successfully processed by the subscriber and we have
restarted the decoding) is that we can re-use the serialized snapshot
at LSN location prior to Prepare of some concurrent WALSender after
the restart. Now, if we ensure that we don't use serialized snapshots
for decoding via slots where two_phase decoding option is enabled then
we won't have that problem. The drawback is that in some cases it can
take a bit more time for initial snapshot building but maybe that is
better than the current solution.

Any suggestions?

-- 
With Regards,
Amit Kapila.

  25. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-18T08:27:54Z

    On Tue, Feb 16, 2021 at 9:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 11, 2021 at 4:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Feb 8, 2021 at 2:01 PM Markus Wanner
> > <markus.wanner@enterprisedb.com> wrote:
> > >
> > Now, coming back to the restart case where the prepared transaction
> > can be sent again by the publisher. I understand yours and others
> > point that we should not send prepared transaction if there is a
> > restart between prepare and commit but there are reasons why we have
> > done that way and I am open to your suggestions. I'll once again try
> > to explain the exact case to you which is not very apparent. The basic
> > idea is that we ship/replay all transactions where commit happens
> > after the snapshot has a consistent state (SNAPBUILD_CONSISTENT), see
> > atop snapbuild.c for details. Now, for transactions where prepare is
> > before snapshot state SNAPBUILD_CONSISTENT and commit prepared is
> > after SNAPBUILD_CONSISTENT, we need to send the entire transaction
> > including prepare at the commit time. One might think it is quite easy
> > to detect that, basically if we skip prepare when the snapshot state
> > was not SNAPBUILD_CONSISTENT, then mark a flag in ReorderBufferTxn and
> > use the same to detect during commit and accordingly take the decision
> > to send prepare but unfortunately it is not that easy. There is always
> > a chance that on restart we reuse the snapshot serialized by some
> > other Walsender at a location prior to Prepare and if that happens
> > then this time the prepare won't be skipped due to snapshot state
> > (SNAPBUILD_CONSISTENT) but due to start_decodint_at point (considering
> > we have already shipped some of the later commits but not prepare).
> > Now, this will actually become the same situation where the restart
> > has happened after we have sent the prepare but not commit. This is
> > the reason we have to resend the prepare when the subscriber restarts
> > between prepare and commit.
> >
>
> After further thinking on this problem and some off-list discussions
> with Ajin, there appears to be another way to solve the above problem
> by which we can avoid resending the prepare after restart if it has
> already been processed by the subscriber. The main reason why we were
> not able to distinguish between the two cases ((a) prepare happened
> before SNAPBUILD_CONSISTENT state but commit prepared happened after
> we reach SNAPBUILD_CONSISTENT state and (b) prepare is already
> decoded, successfully processed by the subscriber and we have
> restarted the decoding) is that we can re-use the serialized snapshot
> at LSN location prior to Prepare of some concurrent WALSender after
> the restart. Now, if we ensure that we don't use serialized snapshots
> for decoding via slots where two_phase decoding option is enabled then
> we won't have that problem. The drawback is that in some cases it can
> take a bit more time for initial snapshot building but maybe that is
> better than the current solution.
>

I see another thing which we need to address if we have to use the
above solution. The issue is if initially the two-pc option for
subscription is off and we skipped prepare because of that and then
some unrelated commit happened which allowed start_decoding_at point
to move ahead. And then the user enabled the two-pc option for the
subscription, then we will again skip prepare because it is behind
start_decoding_at point which becomes the same case where prepare
seems to have already been sent. So, in such a situation with the
above solution, we will miss sending the prepared transaction and its
data and hence risk making replica out-of-sync. Now, this can be
avoided if we don't allow users to alter the two-pc option once the
subscription is created. I am not sure but maybe for the first version
of this feature that might be okay and we can improve it later if we
have better ideas. This will definitely allow us to avoid checks in
the plugins and or apply-worker which seems like a good trade-off and
it will address the concern most people have raised in this thread.
Any thoughts?

-- 
With Regards,
Amit Kapila.

  26. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-19T02:50:52Z

    On Tue, Feb 16, 2021 at 3:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

>
> After further thinking on this problem and some off-list discussions
> with Ajin, there appears to be another way to solve the above problem
> by which we can avoid resending the prepare after restart if it has
> already been processed by the subscriber. The main reason why we were
> not able to distinguish between the two cases ((a) prepare happened
> before SNAPBUILD_CONSISTENT state but commit prepared happened after
> we reach SNAPBUILD_CONSISTENT state and (b) prepare is already
> decoded, successfully processed by the subscriber and we have
> restarted the decoding) is that we can re-use the serialized snapshot
> at LSN location prior to Prepare of some concurrent WALSender after
> the restart. Now, if we ensure that we don't use serialized snapshots
> for decoding via slots where two_phase decoding option is enabled then
> we won't have that problem. The drawback is that in some cases it can
> take a bit more time for initial snapshot building but maybe that is
> better than the current solution.

Based on this suggestion, I have created a patch on HEAD which now
does not allow repeated decoding
of prepared transactions. For this, the code now enforces
full_snapshot if two-phase decoding is enabled.
Do have a look at the patch and see if you have any comments.

Currently  one problem with this, as you have also mentioned in your
last mail,  is that if initially two-phase is disabled in
test_decoding while
decoding prepare (causing the prepared transaction to not be decoded)
and later enabled after the commit prepared (where it assumes that the
transaction was decoded at prepare time), then the transaction is not
decoded at all. For eg:

postgres=# begin;
BEGIN
postgres=*# INSERT INTO do_write DEFAULT VALUES;
INSERT 0 1
postgres=*# PREPARE TRANSACTION 'test1';
PREPARE TRANSACTION
postgres=# SELECT data  FROM
pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
'0');
data
------
(0 rows)
postgres=# commit prepared 'test1';
COMMIT PREPARED
postgres=# SELECT data  FROM
pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
'1');
         data
-------------------------
COMMIT PREPARED 'test1' (1 row)

1st pg_logical_slot_get_changes is called with two-phase-commit off,
2nd is called with two-phase-commit on. You can see that the
transaction is not decoded at all.
For this, I am planning to change the semantics such that
two-phase-commit can only be specified while creating the slot using
pg_create_logical_replication_slot()
and not in pg_logical_slot_get_changes, thus preventing
two-phase-commit flag from being toggled between restarts of the
decoder. Let me know if anybody objects to this
change, else I will update that in the next patch.


regards,
Ajin Cherian
Fujitsu Australia

  27. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-19T14:53:32Z

    Ajin, Amit,

thank you both a lot for thinking this through and even providing a patch.

The changes in expectation for twophase.out matches exactly with what I 
prepared.  And the switch with pg_logical_slot_get_changes indeed is 
something I had not yet considered, either.

On 19.02.21 03:50, Ajin Cherian wrote:
> For this, I am planning to change the semantics such that
> two-phase-commit can only be specified while creating the slot using
> pg_create_logical_replication_slot()
> and not in pg_logical_slot_get_changes, thus preventing
> two-phase-commit flag from being toggled between restarts of the
> decoder. Let me know if anybody objects to this
> change, else I will update that in the next patch.

This sounds like a good plan to me, yes.


However, more generally speaking, I suspect you are overthinking this. 
All of the complexity arises because of the assumption that an output 
plugin receiving and confirming a PREPARE may not be able to persist 
that first phase of transaction application.  Instead, you are trying to 
somehow resurrect the transactional changes and the prepare at COMMIT 
PREPARED time and decode it in a deferred way.

Instead, I'm arguing that a PREPARE is an atomic operation just like a 
transaction's COMMIT.  The decoder should always feed these in the order 
of appearance in the WAL.  For example, if you have PREAPRE A, COMMIT B, 
COMMIT PREPARED A in the WAL, the decoder should always output these 
events in exactly that order.  And not ever COMMIT B, PREPARE A, COMMIT 
PREPARED A (which is currently violated in the expectation for 
twophase_snapshot, because the COMMIT for `s1insert` there appears after 
the PREPARE of `s2p` in the WAL, but gets decoded before it).

The patch I'm attaching corrects this expectation in twophase_snapshot, 
adds an explanatory diagram, and eliminates any danger of sending 
PREPAREs at COMMIT PREPARED time.  Thereby preserving the ordering of 
PREPAREs vs COMMITs.

Given the output plugin supports two-phase commit, I argue there must be 
a good reason for it setting the start_decoding_at LSN to a point in 
time after a PREPARE.  To me that means the output plugin (or its 
downstream replica) has processed the PREPARE (and the downstream 
replica did whatever it needed to do on its side in order to make the 
transaction ready to be committed in a second phase).

(In the weird case of an output plugin that wants to enable two-phase 
commit but does not really support it downstream, it's still possible 
for it to hold back LSN confirmations for prepared-but-still-in-flight 
transactions.  However, I'm having a hard time justifying this use case.)

With that line of thinking, the point in time (or in WAL) of the COMMIT 
PREPARED does not matter at all to reason about the decoding of the 
PREPARE operation.  Instead, there are only exactly two cases to consider:

a) the PREPARE happened before the start_decoding_at LSN and must not be 
decoded. (But the effects of the PREPARE must then be included in the 
initial synchronization. If that's not supported, the output plugin 
should not enable two-phase commit.)

b) the PREPARE happens after the start_decoding_at LSN and must be 
decoded.  (It obviously is not included in the initial synchronization 
or decoded by a previous instance of the decoder process.)

The case where the PREPARE lies before SNAPBUILD_CONSISTENT must always 
be case a) where we must not repeat the PREPARE, anyway.  And in case b) 
where we need a consistent snapshot to decode the PREPARE, existing 
provisions already guarantee that to be possible (or how would this be 
different from a regular single-phase commit?).

Please let me know what you think and whether this approach is feasible 
for you as well.

Regards

Markus

  28. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-20T03:38:03Z

    On Fri, Feb 19, 2021 at 8:23 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
>
> With that line of thinking, the point in time (or in WAL) of the COMMIT
> PREPARED does not matter at all to reason about the decoding of the
> PREPARE operation.  Instead, there are only exactly two cases to consider:
>
> a) the PREPARE happened before the start_decoding_at LSN and must not be
> decoded. (But the effects of the PREPARE must then be included in the
> initial synchronization. If that's not supported, the output plugin
> should not enable two-phase commit.)
>

I see a problem with this assumption. During the initial
synchronization, this transaction won't be visible to snapshot and we
won't copy it. Then later if we won't decode and send it then the
replica will be out of sync. Such a problem won't happen with Ajin's
patch.

-- 
With Regards,
Amit Kapila.

  29. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-20T04:16:21Z

    On Fri, Feb 19, 2021 at 8:21 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> Based on this suggestion, I have created a patch on HEAD which now
> does not allow repeated decoding
> of prepared transactions. For this, the code now enforces
> full_snapshot if two-phase decoding is enabled.
> Do have a look at the patch and see if you have any comments.
>

Few minor comments:
===================
1.
.git/rebase-apply/patch:135: trailing whitespace.
         * We need to mark the transaction as prepared, so that we
don't resend it on
warning: 1 line adds whitespace errors.

Whitespace issue.

2.
/*
+ * Set snapshot type
+ */
+void
+SetSnapBuildType(SnapBuild *builder, bool need_full_snapshot)

There is no caller which passes the second parameter as false, so why
have it? Can't we have a function with SetSnapBuildFullSnapshot or
something like that?

3.
@@ -431,6 +431,10 @@ CreateInitDecodingContext(const char *plugin,
  startup_cb_wrapper(ctx, &ctx->options, true);
  MemoryContextSwitchTo(old_context);

+ /* If two-phase is on, then only full snapshot can be used */
+ if (ctx->twophase)
+ SetSnapBuildType(ctx->snapshot_builder, true);
+
  ctx->reorder->output_rewrites = ctx->options.receive_rewrites;

  return ctx;
@@ -534,6 +538,10 @@ CreateDecodingContext(XLogRecPtr start_lsn,

  ctx->reorder->output_rewrites = ctx->options.receive_rewrites;

+ /* If two-phase is on, then only full snapshot can be used */
+ if (ctx->twophase)
+ SetSnapBuildType(ctx->snapshot_builder, true);

I think it is better to add a detailed comment on why we are doing
this? You can write the comment in one of the places.

> Currently  one problem with this, as you have also mentioned in your
> last mail,  is that if initially two-phase is disabled in
> test_decoding while
> decoding prepare (causing the prepared transaction to not be decoded)
> and later enabled after the commit prepared (where it assumes that the
> transaction was decoded at prepare time), then the transaction is not
> decoded at all. For eg:
>
> postgres=# begin;
> BEGIN
> postgres=*# INSERT INTO do_write DEFAULT VALUES;
> INSERT 0 1
> postgres=*# PREPARE TRANSACTION 'test1';
> PREPARE TRANSACTION
> postgres=# SELECT data  FROM
> pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
> 'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
> '0');
> data
> ------
> (0 rows)
> postgres=# commit prepared 'test1';
> COMMIT PREPARED
> postgres=# SELECT data  FROM
> pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
> 'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
> '1');
>          data
> -------------------------
> COMMIT PREPARED 'test1' (1 row)
>
> 1st pg_logical_slot_get_changes is called with two-phase-commit off,
> 2nd is called with two-phase-commit on. You can see that the
> transaction is not decoded at all.
> For this, I am planning to change the semantics such that
> two-phase-commit can only be specified while creating the slot using
> pg_create_logical_replication_slot()
> and not in pg_logical_slot_get_changes, thus preventing
> two-phase-commit flag from being toggled between restarts of the
> decoder.
>

+1.

-- 
With Regards,
Amit Kapila.

  30. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-20T09:28:05Z

    On Sat, Feb 20, 2021 at 9:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Feb 19, 2021 at 8:21 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > Based on this suggestion, I have created a patch on HEAD which now
> > does not allow repeated decoding
> > of prepared transactions. For this, the code now enforces
> > full_snapshot if two-phase decoding is enabled.
> > Do have a look at the patch and see if you have any comments.
> >
>
> Few minor comments:
> ===================
> 1.
> .git/rebase-apply/patch:135: trailing whitespace.
>          * We need to mark the transaction as prepared, so that we
> don't resend it on
> warning: 1 line adds whitespace errors.
>
> Whitespace issue.
>
> 2.
> /*
> + * Set snapshot type
> + */
> +void
> +SetSnapBuildType(SnapBuild *builder, bool need_full_snapshot)
>
> There is no caller which passes the second parameter as false, so why
> have it? Can't we have a function with SetSnapBuildFullSnapshot or
> something like that?
>
> 3.
> @@ -431,6 +431,10 @@ CreateInitDecodingContext(const char *plugin,
>   startup_cb_wrapper(ctx, &ctx->options, true);
>   MemoryContextSwitchTo(old_context);
>
> + /* If two-phase is on, then only full snapshot can be used */
> + if (ctx->twophase)
> + SetSnapBuildType(ctx->snapshot_builder, true);
> +
>   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
>
>   return ctx;
> @@ -534,6 +538,10 @@ CreateDecodingContext(XLogRecPtr start_lsn,
>
>   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
>
> + /* If two-phase is on, then only full snapshot can be used */
> + if (ctx->twophase)
> + SetSnapBuildType(ctx->snapshot_builder, true);
>
> I think it is better to add a detailed comment on why we are doing
> this? You can write the comment in one of the places.
>

Few more comments:
==================
1. I think you need to update the examples in the docs as well [1].
2. Also the text in the description of begin_prepare_cb [2] needs some
adjustment. We can say something on lines that if users want they can
check if the same GID exists and then they can either error out or
take appropriate action based on their need.

[1] - https://www.postgresql.org/docs/devel/logicaldecoding-example.html
[2] - https://www.postgresql.org/docs/devel/logicaldecoding-output-plugin.html

-- 
With Regards,
Amit Kapila.

  31. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-20T10:55:19Z

    On 20.02.21 04:38, Amit Kapila wrote:
> I see a problem with this assumption. During the initial
> synchronization, this transaction won't be visible to snapshot and we
> won't copy it. Then later if we won't decode and send it then the
> replica will be out of sync. Such a problem won't happen with Ajin's
> patch.

You are assuming that the initial snapshot is a) logical and b) dumb.

A physical snapshot very well "sees" prepared transactions and will 
restore them to their prepared state.  But even in the logical case, I 
think it's beneficial to keep the decoder simpler and instead require 
some support for two-phase commit in the initial synchronization logic. 
  For example using the following approach (you will recognize 
similarities to what snapbuild does):

1.) create the slot
2.) start to retrieve changes and queue them
3.) wait for the prepared transactions that were pending at the
     point in time of step 1 to complete
4.) take a snapshot (by visibility, w/o requiring to "see" prepared
     transactions)
5.) apply the snapshot
6.) replay the queue, filtering commits already visible in the
     snapshot

Just as with the solution proposed by Ajin and you, this has the danger 
of showing transactions as committed without the effects of the PREPAREs 
being "visible" (after step 5 but before 6).

However, this approach of solving the problem outside of the walsender 
has two advantages:

* The delay in step 3 can be made visible and dealt with.  As there's
   no upper boundary to that delay, it makes sense to e.g. inform the
   user after 10 minutes and provide a list of two-phase transactions
   still in progress.

* Second, it becomes possible to avoid inconsistencies during the
   reconciliation window in between steps 5 and 6 by disallowing
   concurrent (user) transactions to run until after completion of
   step 6.

Whereas the current implementation hides this in the walsender without 
any way to determine how much a PREPARE had been delayed or when 
consistency has been reached.  (Of course, short of using the very same 
initial snapshotting approach outlined above.  For which the reordering 
logic in the walsender does more harm than good.)

Essentially, I think I'm saying that while I agree that some kind of 
snapshot synchronization logic is needed, it should live in a different 
place.

Regards

Markus

  32. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-20T12:15:48Z

    On Sat, Feb 20, 2021 at 4:25 PM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
>
> On 20.02.21 04:38, Amit Kapila wrote:
> > I see a problem with this assumption. During the initial
> > synchronization, this transaction won't be visible to snapshot and we
> > won't copy it. Then later if we won't decode and send it then the
> > replica will be out of sync. Such a problem won't happen with Ajin's
> > patch.
>
> You are assuming that the initial snapshot is a) logical and b) dumb.
>
> A physical snapshot very well "sees" prepared transactions and will
> restore them to their prepared state.  But even in the logical case, I
> think it's beneficial to keep the decoder simpler
>

I think after the patch Ajin proposed decoders won't need any special
checks after receiving the prepared xacts. What additional simplicity
this approach will bring? I rather see that we might need to change
the exiting initial sync (copy) with additional restrictions to
support two-pc for subscribers.

> and instead require
> some support for two-phase commit in the initial synchronization logic.
>   For example using the following approach (you will recognize
> similarities to what snapbuild does):
>
> 1.) create the slot
> 2.) start to retrieve changes and queue them
> 3.) wait for the prepared transactions that were pending at the
>      point in time of step 1 to complete
> 4.) take a snapshot (by visibility, w/o requiring to "see" prepared
>      transactions)
> 5.) apply the snapshot
>

Do you mean to say that after creating the slot we take an additional
pass over WAL (till the LSN where we found a consistent snapshot) to
collect all prepared transactions and wait for them to get
committed/rollbacked?

> 6.) replay the queue, filtering commits already visible in the
>      snapshot
>
> Just as with the solution proposed by Ajin and you, this has the danger
> of showing transactions as committed without the effects of the PREPAREs
> being "visible" (after step 5 but before 6).
>

I think the scheme proposed by you is still not fully clear to me but
can you please explain how in the existing proposed patch there is a
danger of showing transactions as committed without the effects of the
PREPAREs being "visible"?


> However, this approach of solving the problem outside of the walsender
> has two advantages:
>
> * The delay in step 3 can be made visible and dealt with.  As there's
>    no upper boundary to that delay, it makes sense to e.g. inform the
>    user after 10 minutes and provide a list of two-phase transactions
>    still in progress.
>
> * Second, it becomes possible to avoid inconsistencies during the
>    reconciliation window in between steps 5 and 6 by disallowing
>    concurrent (user) transactions to run until after completion of
>    step 6.
>

This second point sounds like a restriction that users might not like.

> Whereas the current implementation hides this in the walsender without
> any way to determine how much a PREPARE had been delayed or when
> consistency has been reached.  (Of course, short of using the very same
> initial snapshotting approach outlined above.  For which the reordering
> logic in the walsender does more harm than good.)
>
> Essentially, I think I'm saying that while I agree that some kind of
> snapshot synchronization logic is needed, it should live in a different
> place.
>

But we need something in existing logic in WALSender or somewhere to
allow supporting 2PC for subscriptions and from your above
description, it is not clear to me how we can achieve that?

-- 
With Regards,
Amit Kapila.

  33. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-20T16:55:46Z

    Hi,

On Fri, Feb 19, 2021, at 19:38, Amit Kapila wrote:
> On Fri, Feb 19, 2021 at 8:23 PM Markus Wanner
> <markus.wanner@enterprisedb.com> wrote:
> >
> > With that line of thinking, the point in time (or in WAL) of the COMMIT
> > PREPARED does not matter at all to reason about the decoding of the
> > PREPARE operation.  Instead, there are only exactly two cases to consider:
> >
> > a) the PREPARE happened before the start_decoding_at LSN and must not be
> > decoded. (But the effects of the PREPARE must then be included in the
> > initial synchronization. If that's not supported, the output plugin
> > should not enable two-phase commit.)
> >
> 
> I see a problem with this assumption. During the initial
> synchronization, this transaction won't be visible to snapshot and we
> won't copy it. Then later if we won't decode and send it then the
> replica will be out of sync. Such a problem won't happen with Ajin's
> patch.

Why isn't the more obvious answer to this to not allow/disable 2pc decoding during the initial sync? You can't really make sense of it before you're synced anyway... 

Regards,

Andres

  34. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-20T20:13:17Z

    Hi,

On 2021-02-19 13:50:52 +1100, Ajin Cherian wrote:
> From 129947ab2d0ba223862ed1c87be0f96b51645ba0 Mon Sep 17 00:00:00 2001
> From: Ajin Cherian <ajinc@fast.au.fujitsu.com>
> Date: Thu, 18 Feb 2021 20:18:16 -0500
> Subject: [PATCH] Don't allow repeated decoding of prepared transactions.
> 
> Enforce full snapshot while decoding with two-phase enabled. This
> allows the decoder to differentiate between prepared transaction that
> were sent prior to restart and prepared transactions that were not sent
> because they were prior to consistent snapshot.

Isn't this an *extremely* expensive solution? Maintaining a full
snapshot is pretty darn expensive - so expensive that it's repeatedly
been a problem even just for building the initial snapshot (to the point
of being inable to do so). And that's typically a comparatively rare
operation, not something continual - but what you're proposing is a cost
paid during ongoing replication.

Greetings,

Andres Freund

  35. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-21T06:02:29Z

    On Sat, Feb 20, 2021 at 10:26 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On Fri, Feb 19, 2021, at 19:38, Amit Kapila wrote:
> > On Fri, Feb 19, 2021 at 8:23 PM Markus Wanner
> > <markus.wanner@enterprisedb.com> wrote:
> > >
> > > With that line of thinking, the point in time (or in WAL) of the COMMIT
> > > PREPARED does not matter at all to reason about the decoding of the
> > > PREPARE operation.  Instead, there are only exactly two cases to consider:
> > >
> > > a) the PREPARE happened before the start_decoding_at LSN and must not be
> > > decoded. (But the effects of the PREPARE must then be included in the
> > > initial synchronization. If that's not supported, the output plugin
> > > should not enable two-phase commit.)
> > >
> >
> > I see a problem with this assumption. During the initial
> > synchronization, this transaction won't be visible to snapshot and we
> > won't copy it. Then later if we won't decode and send it then the
> > replica will be out of sync. Such a problem won't happen with Ajin's
> > patch.
>
> Why isn't the more obvious answer to this to not allow/disable 2pc decoding during the initial sync?
>

Here, I am assuming you are asking to disable 2PC both via
apply-worker and tablesync worker till the initial sync (aka all
tables are in SUBREL_STATE_READY state) phase is complete. If we do
that and what if commit prepared happened after the initial sync phase
but prepare happened before that? At Commit prepared because the 2PC
is enabled, we will just send Commit Prepared without the actual data
and prepare. Now, to solve that say we remember in TXN that at prepare
time 2PC was not enabled so at commit prepared time consider that 2PC
is disabled for that TXN and send the entire transaction along with
commit as we do for non-2PC TXNs. But it is possible that a restart
might happen before the commit prepared and then it is possible that
prepare falls before start_decoding_at point so we will still skip
sending it even though 2PC is enabled after the restart and just send
the commit prepared. So, again that can lead to replica going out of
sync.

The other thing related to this is to see to ensure that via SQL APIs
we don't skip any prepared xacts and just return commit prepared.
Basically, the example case, I have described in my email above [1].

One of the ideas I have previously speculated to overcome these
challenges is to someway persist the information of Prepares that are
decoded. Say, after sending prepare, we update the slot information on
disk to indicate that the particular GID is sent. Then next time
whenever we have to skip prepare due to whatever reason, we can check
the existence of persistent information on disk for that GID, if it
exists then we need to send just Commit Prepared, otherwise, the
entire transaction. We can remove this information during or after
CheckPointSnapBuild, basically, we can remove the information of all
GID's that are after cutoff LSN computed via
ReplicationSlotsComputeLogicalRestartLSN. But that seems to be costly
so we didn't pursue it.

[1] - https://www.postgresql.org/message-id/CAA4eK1L5aX1BL9Xg-wSULbFeB417G0v9qk5qZ6NbYCkCo6JUGQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  36. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-21T22:26:26Z

    Hi,

On 2021-02-21 11:32:29 +0530, Amit Kapila wrote:
> Here, I am assuming you are asking to disable 2PC both via
> apply-worker and tablesync worker till the initial sync (aka all
> tables are in SUBREL_STATE_READY state) phase is complete. If we do
> that and what if commit prepared happened after the initial sync phase
> but prepare happened before that?

Isn't that pretty easy to detect? You compare the LSN of the tx prepare
with the LSN of achieving consistency? Any restart will recover the
LSNs, because restart_lsn will be before the start of the tx.


> At Commit prepared because the 2PC is enabled, we will just send
> Commit Prepared without the actual data and prepare. Now, to solve
> that say we remember in TXN that at prepare time 2PC was not enabled
> so at commit prepared time consider that 2PC is disabled for that TXN
> and send the entire transaction along with commit as we do for non-2PC
> TXNs. But it is possible that a restart might happen before the commit
> prepared and then it is possible that prepare falls before
> start_decoding_at point so we will still skip sending it even though
> 2PC is enabled after the restart and just send the commit
> prepared. So, again that can lead to replica going out of sync.

I don't think that an LSN based approach is susceptible to this. And it
wouldn't require more memory etc than we'd now.

Greetings,

Andres Freund

  37. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-22T02:52:35Z

    On Mon, Feb 22, 2021 at 3:56 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2021-02-21 11:32:29 +0530, Amit Kapila wrote:
> > Here, I am assuming you are asking to disable 2PC both via
> > apply-worker and tablesync worker till the initial sync (aka all
> > tables are in SUBREL_STATE_READY state) phase is complete. If we do
> > that and what if commit prepared happened after the initial sync phase
> > but prepare happened before that?
>
> Isn't that pretty easy to detect? You compare the LSN of the tx prepare
> with the LSN of achieving consistency?
>

I think by LSN of achieving consistency, you mean start_decoding_at
LSN. It is possible that start_decoding_at point has been moved ahead
because of some other unrelated commit that happens between prepare
and commit prepared. Something like below:

LSN for Prepare of xact t1 at 500
LSN for Commit of xact t2 at 520
LSN for Commit Prepared at 550

Say we skipped prepare because 2PC was not enabled but then decoded
and sent Commit of xact t2. I think after this start_decoding_at LSN
will be at 520. So comparing the prepare LSN of xact t1 with
start_decoding_at will lead to skipping the prepare after the restart
and we will just send the commit prepared without prepare and data
when we process LSN of Commit Prepared at 550.

-- 
With Regards,
Amit Kapila.

  38. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-22T04:09:21Z

    Hi,

On 2021-02-22 08:22:35 +0530, Amit Kapila wrote:
> On Mon, Feb 22, 2021 at 3:56 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2021-02-21 11:32:29 +0530, Amit Kapila wrote:
> > > Here, I am assuming you are asking to disable 2PC both via
> > > apply-worker and tablesync worker till the initial sync (aka all
> > > tables are in SUBREL_STATE_READY state) phase is complete. If we do
> > > that and what if commit prepared happened after the initial sync phase
> > > but prepare happened before that?
> >
> > Isn't that pretty easy to detect? You compare the LSN of the tx prepare
> > with the LSN of achieving consistency?
> >
>
> I think by LSN of achieving consistency, you mean start_decoding_at
> LSN.

Kinda, but not in the way you suggest. I mean the LSN at which the slot
reached SNAPBUILD_CONSISTENT. Which also is the point in the WAL stream
we exported the initial snapshot for.

My understanding of why you need to have special handling of 2pc PREPARE
is that the initial snapshot will not contain the contents of the
prepared transaction, therefore you need to send it out at some point
(or be incorrect).

Your solution to this is:
	/*
	 * It is possible that this transaction is not decoded at prepare time
	 * either because by that time we didn't have a consistent snapshot or it
	 * was decoded earlier but we have restarted. We can't distinguish between
	 * those two cases so we send the prepare in both the cases and let
	 * downstream decide whether to process or skip it. We don't need to
	 * decode the xact for aborts if it is not done already.
	 */
	if (!rbtxn_prepared(txn) && is_commit)

but IMO this violates a pretty fundamental tenant of how logical
decoding is supposed to work, i.e. that data that the client
acknowledges as having received (via lsn passed to START_REPLICATION)
shouldn't be sent out again.

What I am proposing is to instead track the point at which the slot
gained consistency - a simple LSN. That way you can change the above
logic to instead be

if (txn->final_lsn > snapshot_was_exported_at_lsn)
   ReorderBufferReplay();
else
   ...

That will easily work across restarts, won't lead to sending data twice,
etc.

Greetings,

Andres Freund

  39. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-22T04:22:43Z

    Hi,

On 2021-02-19 15:53:32 +0100, Markus Wanner wrote:
> However, more generally speaking, I suspect you are overthinking this. All
> of the complexity arises because of the assumption that an output plugin
> receiving and confirming a PREPARE may not be able to persist that first
> phase of transaction application.  Instead, you are trying to somehow
> resurrect the transactional changes and the prepare at COMMIT PREPARED time
> and decode it in a deferred way.

The output plugin should never persist anything. That's the job of the
client, not the output plugin. The output plugin simply doesn't have the
information to know whether the client received data and successfully
applied data or not.


> Given the output plugin supports two-phase commit, I argue there must be a
> good reason for it setting the start_decoding_at LSN to a point in time
> after a PREPARE.  To me that means the output plugin (or its downstream
> replica) has processed the PREPARE (and the downstream replica did whatever
> it needed to do on its side in order to make the transaction ready to be
> committed in a second phase).

The output plugin doesn't set / influence start_decoding_at (unless you
want to count just ERRORing out).


> With that line of thinking, the point in time (or in WAL) of the COMMIT
> PREPARED does not matter at all to reason about the decoding of the PREPARE
> operation.  Instead, there are only exactly two cases to consider:
> 
> a) the PREPARE happened before the start_decoding_at LSN and must not be
> decoded. (But the effects of the PREPARE must then be included in the
> initial synchronization. If that's not supported, the output plugin should
> not enable two-phase commit.)

I don't think that can be made work without disproportionate
complexity. Especially not in cases where we start to be CONSISTENT
based on pre-existing on-disk snapshots.


Greetings,

Andres Freund

  40. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-22T08:25:17Z

    On 20.02.21 13:15, Amit Kapila wrote:
> I think after the patch Ajin proposed decoders won't need any special
> checks after receiving the prepared xacts. What additional simplicity
> this approach will bring?

The API becomes clearer in that all PREPAREs are always decoded in WAL 
stream order and are not ever deferred (possibly until after the commits 
of many other transactions).  No output plugin will need to check 
against this peculiarity, but can rely on WAL ordering of events.

(And if an output plugin does not want prepares to be individual events, 
it should simply not enable two-phase support.  That seems like 
something the output plugin could even do on a per-transaction basis.)

> Do you mean to say that after creating the slot we take an additional
> pass over WAL (till the LSN where we found a consistent snapshot) to
> collect all prepared transactions and wait for them to get
> committed/rollbacked?

No.  A single pass is enough, the decoder won't need any further change 
beyond the code removal in my patch.

I'm proposing for the synchronization logic (in e.g. pgoutput) to defer 
the snapshot taking.  So that there's some time in between creating the 
logical slot (at step 1.) and taking a snapshot (at step 4.).  Another 
CATCHUP phase, if you want.

So that all two-phase commit transactions are delivered via either:

* the transferred snapshot (because their COMMIT PREPARED took place
   before the snapshot was taken in (4)), or

* the decoder stream (because their PREPARE took place after the slot
   was fully created and snapbuilder reached a consistent snapshot)

No transaction can have PREPAREd before (1) but not committed until 
after (4), because we waited for all prepared transactions to commit in 
step (3).

> I think the scheme proposed by you is still not fully clear to me but
> can you please explain how in the existing proposed patch there is a
> danger of showing transactions as committed without the effects of the
> PREPAREs being "visible"?

Please see the `twophase_snapshot` isolation test.  The expected output 
there shows the insert from s1 being committed prior to the prepare of 
the transaction in s2.

On a replica applying the stream in that order, a transaction in between 
these two events would see the results from s1 while still being allowed 
to lock the row that s2 is about to update.  Something I'd expect the 
PREPARE to prevent.

That is (IMO) wrong in `master` and Ajin's patch doesn't correct it. 
(While my patch does, so don't look at my patch for this example.)

>> * Second, it becomes possible to avoid inconsistencies during the
>>     reconciliation window in between steps 5 and 6 by disallowing
>>     concurrent (user) transactions to run until after completion of
>>     step 6.
> 
> This second point sounds like a restriction that users might not like.

"It becomes possible" cannot be a restriction.  If a user (or 
replication solution) wants to allow for these inconsistencies, it still 
can.  I want to make sure that solutions which *want* to prevent 
inconsistencies can be implemented.

Your concern applies to step (3), though.  The current approach is 
clearly quicker to restore the backup and start to apply transactions. 
Until you start to think about reordering the "early" commits until 
after the deferred PREPAREs in the output plugin or on the replica side, 
so as to lock rows by prepared transactions before making other commits 
visible so as to prevent inconsistencies...

> But we need something in existing logic in WALSender or somewhere to
> allow supporting 2PC for subscriptions and from your above
> description, it is not clear to me how we can achieve that?

I agree that some more code is required somewhere, outside of the walsender.

Regards

Markus

  41. Re: repeated decoding of prepared transactions

    Markus Wanner <markus.wanner@enterprisedb.com> — 2021-02-22T08:25:52Z

    On 22.02.21 05:22, Andres Freund wrote:
> Hi,
> 
> On 2021-02-19 15:53:32 +0100, Markus Wanner wrote:
>> However, more generally speaking, I suspect you are overthinking this. All
>> of the complexity arises because of the assumption that an output plugin
>> receiving and confirming a PREPARE may not be able to persist that first
>> phase of transaction application.  Instead, you are trying to somehow
>> resurrect the transactional changes and the prepare at COMMIT PREPARED time
>> and decode it in a deferred way.
> 
> The output plugin should never persist anything.

Sure, sorry, I was sloppy in formulation.  I meant the replica or client 
that receives the data from the output plugin.  Given it asked for 
two-phase commits in the output plugin, it clearly is interested in the 
PREPARE.

> That's the job of the
> client, not the output plugin. The output plugin simply doesn't have the
> information to know whether the client received data and successfully
> applied data or not.

Exactly.  Therefore, it should not randomly reshuffle or reorder 
PREPAREs until after other COMMITs.

> The output plugin doesn't set / influence start_decoding_at (unless you
> want to count just ERRORing out).

Yeah, same sloppiness, sorry.

>> With that line of thinking, the point in time (or in WAL) of the COMMIT
>> PREPARED does not matter at all to reason about the decoding of the PREPARE
>> operation.  Instead, there are only exactly two cases to consider:
>>
>> a) the PREPARE happened before the start_decoding_at LSN and must not be
>> decoded. (But the effects of the PREPARE must then be included in the
>> initial synchronization. If that's not supported, the output plugin should
>> not enable two-phase commit.)
> 
> I don't think that can be made work without disproportionate
> complexity. Especially not in cases where we start to be CONSISTENT
> based on pre-existing on-disk snapshots.

Well, the PREPARE to happen before the start_decoding_at LSN is a case 
the output plugin needs to deal with.  I pointed out why the current way 
of dealing with it clearly is wrong.

What issues do you see with the approach I proposed?

Regards

Markus

  42. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-22T08:59:09Z

    On Mon, Feb 22, 2021 at 9:39 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-02-22 08:22:35 +0530, Amit Kapila wrote:
> > On Mon, Feb 22, 2021 at 3:56 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > On 2021-02-21 11:32:29 +0530, Amit Kapila wrote:
> > > > Here, I am assuming you are asking to disable 2PC both via
> > > > apply-worker and tablesync worker till the initial sync (aka all
> > > > tables are in SUBREL_STATE_READY state) phase is complete. If we do
> > > > that and what if commit prepared happened after the initial sync phase
> > > > but prepare happened before that?
> > >
> > > Isn't that pretty easy to detect? You compare the LSN of the tx prepare
> > > with the LSN of achieving consistency?
> > >
> >
> > I think by LSN of achieving consistency, you mean start_decoding_at
> > LSN.
>
> Kinda, but not in the way you suggest. I mean the LSN at which the slot
> reached SNAPBUILD_CONSISTENT. Which also is the point in the WAL stream
> we exported the initial snapshot for.
>

Okay, that's an interesting idea. I have few questions on this, see below.

> My understanding of why you need to have special handling of 2pc PREPARE
> is that the initial snapshot will not contain the contents of the
> prepared transaction, therefore you need to send it out at some point
> (or be incorrect).
>
> Your solution to this is:
>         /*
>          * It is possible that this transaction is not decoded at prepare time
>          * either because by that time we didn't have a consistent snapshot or it
>          * was decoded earlier but we have restarted. We can't distinguish between
>          * those two cases so we send the prepare in both the cases and let
>          * downstream decide whether to process or skip it. We don't need to
>          * decode the xact for aborts if it is not done already.
>          */
>         if (!rbtxn_prepared(txn) && is_commit)
>
> but IMO this violates a pretty fundamental tenant of how logical
> decoding is supposed to work, i.e. that data that the client
> acknowledges as having received (via lsn passed to START_REPLICATION)
> shouldn't be sent out again.
>

I agree that this is not acceptable that is why trying to explore
other solutions including what you have proposed.

> What I am proposing is to instead track the point at which the slot
> gained consistency - a simple LSN. That way you can change the above
> logic to instead be
>
> if (txn->final_lsn > snapshot_was_exported_at_lsn)
>    ReorderBufferReplay();
> else
>    ...
>

With this if the prepare is prior to consistent_snapshot
(snapshot_was_exported_at_lsn)) and commit prepared is after then we
won't send the prepare and data. Won't we need to send such prepares?
If the condition is other way (if (txn->final_lsn <
snapshot_was_exported_at_lsn)) then we would send such prepares?

Just to clarify, after the initial copy, say when we start/restart the
streaming and we picked the serialized snapshot of some other
WALSender, we don't need to use snapshot_was_exported_at_lsn
corresponding to the serialized snapshot of some other slot?

I am not sure for the matter of this problem enabling 2PC during
initial sync (initial snapshot + copy) matters. Because, if we follow
the above, then it should be fine even if 2PC is enabled?

> That will easily work across restarts, won't lead to sending data twice,
> etc.
>

Yeah, we need to probably store this new point as slot's persistent information.

-- 
With Regards,
Amit Kapila.

  43. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-22T09:25:25Z

    Hi,

On 2021-02-22 09:25:52 +0100, Markus Wanner wrote:
> What issues do you see with the approach I proposed?

Very significant increase in complexity for initializing a logical
replica, because there's no easy way to just use the initial slot.

- Andres

  44. Re: repeated decoding of prepared transactions

    Andres Freund <andres@anarazel.de> — 2021-02-22T09:27:07Z

    Hi,

On 2021-02-22 14:29:09 +0530, Amit Kapila wrote:
> On Mon, Feb 22, 2021 at 9:39 AM Andres Freund <andres@anarazel.de> wrote:
> > What I am proposing is to instead track the point at which the slot
> > gained consistency - a simple LSN. That way you can change the above
> > logic to instead be
> >
> > if (txn->final_lsn > snapshot_was_exported_at_lsn)
> >    ReorderBufferReplay();
> > else
> >    ...
> >
> 
> With this if the prepare is prior to consistent_snapshot
> (snapshot_was_exported_at_lsn)) and commit prepared is after then we
> won't send the prepare and data. Won't we need to send such prepares?
> If the condition is other way (if (txn->final_lsn <
> snapshot_was_exported_at_lsn)) then we would send such prepares?

Yea, I inverted the condition...


> Just to clarify, after the initial copy, say when we start/restart the
> streaming and we picked the serialized snapshot of some other
> WALSender, we don't need to use snapshot_was_exported_at_lsn
> corresponding to the serialized snapshot of some other slot?

Correct.


> Yeah, we need to probably store this new point as slot's persistent information.

Should be fine I think...

Greetings,

Andres Freund

  45. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-22T09:41:28Z

    On Mon, Feb 22, 2021 at 2:55 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-02-22 09:25:52 +0100, Markus Wanner wrote:
> > What issues do you see with the approach I proposed?
>
> Very significant increase in complexity for initializing a logical
> replica, because there's no easy way to just use the initial slot.
>

+1. The solution proposed by Andres seems to be better than other
ideas we have discussed so far.

-- 
With Regards,
Amit Kapila.

  46. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-23T09:36:03Z

    On Mon, Feb 22, 2021 at 8:27 PM Andres Freund <andres@anarazel.de> wrote:

> > Yeah, we need to probably store this new point as slot's persistent information.
>
> Should be fine I think...

This idea looks convincing. I'll write up a patch incorporating these changes.

regards,
Ajin Cherian
Fujitsu Australia

  47. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-23T09:53:49Z

    On Mon, Feb 22, 2021 at 2:57 PM Andres Freund <andres@anarazel.de> wrote:
>
> > Yeah, we need to probably store this new point as slot's persistent information.
>
> Should be fine I think...
>

So, we are in agreement that the above solution will work and we won't
need to resend the prepare after the restart. I would like to once
again describe few other points which we are discussing in this and
other thread [1] to see if you or others have any different opinion on
those:

1. With respect to SQL APIs, currently 'two-phase-commit' is a plugin
option so it is possible that the first time when it gets changes
(pg_logical_slot_get_changes) *without* 2PC enabled it will not get
the prepared even though prepare is after consistent snapshot. Now
next time during getting changes (pg_logical_slot_get_changes) if the
2PC option is enabled it will skip prepare because by that time
start_decoding_at has been moved. So the user will only get commit
prepared as shown in the example in the email above [2]. I think it
might be better to allow enable/disable of 2PC only at create_slot
time. Markus, Ajin, and I seem to be in agreement on this point. I
think the same will be true for subscriber-side solution as well.

2. There is a possibility that subscribers miss some prepared xacts.
Let me explain the problem and solution. Currently, when we create a
subscription, we first launch apply-worker and create the main apply
worker slot and then launch table sync workers as required. Now,
assume, the apply worker slot is created and after that, we launch
tablesync worker, which will initiate its slot (sync_slot) creation.
Then, on the publisher-side, the situation is such that there is a
prepared transaction that happens before we reach a consistent
snapshot for sync_slot.

Because the WALSender corresponding to apply worker is already running
so it will be in consistent state, for it, such a prepared xact can be
decoded and it will send the same to the subscriber. On the
subscriber-side, it can skip applying the data-modification operations
because the corresponding rel is still not in a ready state (see
should_apply_changes_for_rel and its callers) simply because the
corresponding table sync worker is not finished yet. But prepare will
occur and it will lead to a prepared transaction on the subscriber.

In this situation, tablesync worker has skipped prepare because the
snapshot was not consistent and then it exited because it is in sync
with the apply worker. And apply worker has skipped because tablesync
was in-progress. Later when Commit prepared will come, the
apply-worker will simply commit the previously prepared transaction
and we will never see the prepared transaction data.

For example, consider below situation:
LSN of Prepare t1 = 490, tablesync skipped because it was prior to a
consistent point
LSN of Commit t2 = 500
LSN of commit t3 = 510
LSN of Commit Prepared t1 = 520.

Tablesync worker initially (via copy) got till xact t3 (LSN = 510).
For the apply worker, we get all the above LSN's as it is started
before tablesync worker and reached a consistent point before it. In
the above example, there is a possibility that we miss applying data
for xact t1 as explained in previous paragraphs.

So, the basic premise is that we can't allow tablesync workers to skip
prepared transactions (which can be processed by apply worker) and
process later commits.

I have one idea to address this. When we get the first begin (for
prepared xact) in the apply-worker, we can check if there are any
relations in "not_ready" state and if so then just wait till all the
relations become in sync with the apply worker. This is to avoid that
any of the tablesync workers might skip prepared xact and we don't
want apply worker to also skip the same.

Now, it is possible that some tablesync worker has copied the data and
moved the sync position ahead of where the current apply worker's
position is. In such a case, we need to process transactions in apply
worker such that we can process commits if any, and write prepared
transactions to file. For prepared transactions, we can take decisions
only once the commit prepared for them has arrived.

The other idea I have thought of for this is to only enable 2PC after
initial sync (when both apply worker and tablesync workers are in
sync) is over but I think that can lead to the problem described in
point 1.

[1] - https://www.postgresql.org/message-id/CAA4eK1L%3DdhuCRvyDvrXX5wZgc7s1hLRD29CKCK6oaHtVCPgiFA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAFPTHDbbth0XVwf%3DWXcmp%3D_2nU5oNaK4CxetUr22qi1UM5v6rw%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  48. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-24T05:48:20Z

    On Tue, Feb 23, 2021 at 8:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> 1. With respect to SQL APIs, currently 'two-phase-commit' is a plugin
> option so it is possible that the first time when it gets changes
> (pg_logical_slot_get_changes) *without* 2PC enabled it will not get
> the prepared even though prepare is after consistent snapshot. Now
> next time during getting changes (pg_logical_slot_get_changes) if the
> 2PC option is enabled it will skip prepare because by that time
> start_decoding_at has been moved. So the user will only get commit
> prepared as shown in the example in the email above [2]. I think it
> might be better to allow enable/disable of 2PC only at create_slot
> time. Markus, Ajin, and I seem to be in agreement on this point. I
> think the same will be true for subscriber-side solution as well.
>

Attaching a patch which avoids repeated decoding of prepares using the
approach suggest by Andres. Added snapshot_was_exported_at_lsn;
fields in ReplicationSlotPersistentData and SnapBuild which now stores
the LSN at which the slot snapshot is exported the time it is created.
This patch also modifies the API pg_create_logical_replication_slot()
to take an extra parameter to enable two-phase commits
and disables pg_logical_slot_get_changes() from enabling two-phase.
I plan to split this into two patches next. But do review and let me
know if you have any comments.

regards,
Ajin

  49. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-24T11:36:04Z

    On Wed, Feb 24, 2021 at 4:48 PM Ajin Cherian <itsajin@gmail.com> wrote:

> I plan to split this into two patches next. But do review and let me
> know if you have any comments.

Attaching an updated patch-set with the changes for
snapshot_was_exported_at_lsn separated out from the changes for the
APIs pg_create_logical_replication_slot() and
pg_logical_slot_get_changes(). Along with a rebase that takes in a few
more commits since my last patch.

regards,
Ajin Cherian
Fujitsu Australia

  50. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-02-25T11:34:01Z

    On Wed, Feb 24, 2021 at 5:06 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Feb 24, 2021 at 4:48 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > I plan to split this into two patches next. But do review and let me
> > know if you have any comments.
>
> Attaching an updated patch-set with the changes for
> snapshot_was_exported_at_lsn separated out from the changes for the
> APIs pg_create_logical_replication_slot() and
> pg_logical_slot_get_changes(). Along with a rebase that takes in a few
> more commits since my last patch.

One observation while verifying the patch I noticed that most of
ReplicationSlotPersistentData structure members are displayed in
pg_replication_slots, but I did not see snapshot_was_exported_at_lsn
being displayed. Is this intentional? If not intentional we can
include snapshot_was_exported_at_lsn in pg_replication_slots.

Regards,
Vignesh

  51. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-25T11:36:00Z

    On Wed, Feb 24, 2021 at 5:06 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Feb 24, 2021 at 4:48 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > I plan to split this into two patches next. But do review and let me
> > know if you have any comments.
>
> Attaching an updated patch-set with the changes for
> snapshot_was_exported_at_lsn separated out from the changes for the
> APIs pg_create_logical_replication_slot() and
> pg_logical_slot_get_changes(). Along with a rebase that takes in a few
> more commits since my last patch.
>

Few comments on the first patch:
1. We can't remove ReorderBufferSkipPrepare because we rely on that in
SnapBuildDistributeNewCatalogSnapshot.
2. I have changed the name of the variable from
snapshot_was_exported_at_lsn to snapshot_was_exported_at but I am
still not very sure about this naming because there are times when we
don't export snapshot and we still set this like when creating slots
with CRS_NOEXPORT_SNAPSHOT or when creating via SQL APIs. The other
name that comes to mind is initial_consistency_at, what do you think?
3. Changed comments at various places.

Please find the above changes as a separate patch, if you like you can
include these in the main patch.

Apart from the above, I think the comments related to docs in my
previous email [1] are still valid, can you please take care of those.

[1] - https://www.postgresql.org/message-id/CAA4eK1Kr34_TiREr57Wpd%3D3%3D03x%3D1n55DAjwJPGpHAEc4dWfUQ%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  52. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-26T08:47:45Z

    On Thu, Feb 25, 2021 at 10:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> Few comments on the first patch:
> 1. We can't remove ReorderBufferSkipPrepare because we rely on that in
> SnapBuildDistributeNewCatalogSnapshot.
> 2. I have changed the name of the variable from
> snapshot_was_exported_at_lsn to snapshot_was_exported_at but I am
> still not very sure about this naming because there are times when we
> don't export snapshot and we still set this like when creating slots
> with CRS_NOEXPORT_SNAPSHOT or when creating via SQL APIs. The other
> name that comes to mind is initial_consistency_at, what do you think?
> 3. Changed comments at various places.
>
> Please find the above changes as a separate patch, if you like you can
> include these in the main patch.
>
> Apart from the above, I think the comments related to docs in my
> previous email [1] are still valid, can you please take care of those.
>
> [1] - https://www.postgresql.org/message-id/CAA4eK1Kr34_TiREr57Wpd%3D3%3D03x%3D1n55DAjwJPGpHAEc4dWfUQ%40mail.gmail.com

I've added Amit's changes-patch as well as addressed comments related
to docs in the attached patch.

>On Thu, Feb 25, 2021 at 10:34 PM vignesh C <vignesh21@gmail.com> wrote:
>One observation while verifying the patch I noticed that most of
>ReplicationSlotPersistentData structure members are displayed in
>pg_replication_slots, but I did not see snapshot_was_exported_at_lsn
>being displayed. Is this intentional? If not intentional we can
>include snapshot_was_exported_at_lsn in pg_replication_slots.

I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
Do have a look and let me know if there are any comments.

regards,
Ajin Cherian
Fujitsu Australia

  53. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-26T10:42:47Z

    On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:

> I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> Do have a look and let me know if there are any comments.

Update with both patches.

regards,
Ajin Cherian
Fujitsu Australia

  54. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-02-26T13:56:02Z

    On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> > Do have a look and let me know if there are any comments.
>
> Update with both patches.

Thanks for fixing and providing an updated patch. Patch applies, make
check and make check-world passes. I could see the issue working fine.

Few minor comments:
+       <structfield>snapshot_was_exported_at</structfield> <type>pg_lsn</type>
+      </para>
+      <para>
+       The address (<literal>LSN</literal>) at which the logical
+       slot found a consistent point at the time of slot creation.
+       <literal>NULL</literal> for physical slots.
+      </para></entry>
+     </row>


I had seen earlier also we had some discussion on naming
snapshot_was_exported_at. Can we change snapshot_was_exported_at to
snapshot_exported_lsn, I felt if we can include the lsn in the name,
the user will be able to interpret easily and also it will be similar
to other columns in pg_replication_slots view.


             L.restart_lsn,
             L.confirmed_flush_lsn,
+                       L.snapshot_was_exported_at,
             L.wal_status,
             L.safe_wal_size

Looks like there is some indentation issue here.

Regards,
Vignesh

  55. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-27T02:59:31Z

    On Fri, Feb 26, 2021 at 7:26 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> > > Do have a look and let me know if there are any comments.
> >
> > Update with both patches.
>
> Thanks for fixing and providing an updated patch. Patch applies, make
> check and make check-world passes. I could see the issue working fine.
>
> Few minor comments:
> +       <structfield>snapshot_was_exported_at</structfield> <type>pg_lsn</type>
> +      </para>
> +      <para>
> +       The address (<literal>LSN</literal>) at which the logical
> +       slot found a consistent point at the time of slot creation.
> +       <literal>NULL</literal> for physical slots.
> +      </para></entry>
> +     </row>
>
>
> I had seen earlier also we had some discussion on naming
> snapshot_was_exported_at. Can we change snapshot_was_exported_at to
> snapshot_exported_lsn, I felt if we can include the lsn in the name,
> the user will be able to interpret easily and also it will be similar
> to other columns in pg_replication_slots view.
>

I have recommended above to change this name to initial_consistency_at
because there are times when we don't export snapshot and we still set
this like when creating slots with CRS_NOEXPORT_SNAPSHOT or when
creating via SQL APIs.  I am not sure why Ajin neither changed the
name nor responded to that comment. What is your opinion?

-- 
With Regards,
Amit Kapila.

  56. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-27T04:02:19Z

    On Thu, Feb 25, 2021 at 5:04 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Feb 24, 2021 at 5:06 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Wed, Feb 24, 2021 at 4:48 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > I plan to split this into two patches next. But do review and let me
> > > know if you have any comments.
> >
> > Attaching an updated patch-set with the changes for
> > snapshot_was_exported_at_lsn separated out from the changes for the
> > APIs pg_create_logical_replication_slot() and
> > pg_logical_slot_get_changes(). Along with a rebase that takes in a few
> > more commits since my last patch.
>
> One observation while verifying the patch I noticed that most of
> ReplicationSlotPersistentData structure members are displayed in
> pg_replication_slots, but I did not see snapshot_was_exported_at_lsn
> being displayed. Is this intentional? If not intentional we can
> include snapshot_was_exported_at_lsn in pg_replication_slots.
>

On thinking about this point, I feel we don't need this new parameter
in the view because I am not able to see how it is of any use to the
user. Over time, corresponding to that LSN there won't be any WAL
record or maybe WAL would be overwritten. I think this is primarily
for our internal use so let's not expose it. I intend to remove it
from the patch unless you have some reason for exposing this to the
user.


-- 
With Regards,
Amit Kapila.

  57. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-02-27T06:02:17Z

    On Sat, 27 Feb, 2021, 1:59 pm Amit Kapila, <amit.kapila16@gmail.com> wrote:

>
> I have recommended above to change this name to initial_consistency_at
> because there are times when we don't export snapshot and we still set
> this like when creating slots with CRS_NOEXPORT_SNAPSHOT or when
> creating via SQL APIs.  I am not sure why Ajin neither changed the
> name nor responded to that comment. What is your opinion?
>

I am fine with the name initial_consistency_at. I am also fine with not
showing this in the pg_replication_slot view and keeping this internal.

Regards,
Ajin Cherian
Fujitsu Australia

>
>

  58. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-27T06:08:37Z

    On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> > Do have a look and let me know if there are any comments.
>
> Update with both patches.
>

Thanks, I have made some minor changes to the first patch and now it
looks good to me. The changes are as below:
1. Removed the changes related to exposing this new parameter via view
as mentioned in my previous email.
2. Changed the variable name initial_consistent_point.
3. Ran pgindent, minor changes in comments, and modified the commit message.

Let me know what you think about these changes.

Next, I'll review your second patch which allows the 2PC option to be
enabled only at slot creation time.


--
With Regards,
Amit Kapila.

  59. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-02-27T12:06:11Z

    On Sat, Feb 27, 2021 at 11:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > > I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> > > Do have a look and let me know if there are any comments.
> >
> > Update with both patches.
> >
>
> Thanks, I have made some minor changes to the first patch and now it
> looks good to me. The changes are as below:
> 1. Removed the changes related to exposing this new parameter via view
> as mentioned in my previous email.
> 2. Changed the variable name initial_consistent_point.
> 3. Ran pgindent, minor changes in comments, and modified the commit message.
>
> Let me know what you think about these changes.
>

In the attached, I have just bumped SNAPBUILD_VERSION  as we are
adding a new member in the SnapBuild structure.

> Next, I'll review your second patch which allows the 2PC option to be
> enabled only at slot creation time.
>

Few comments on 0002 patch:
=========================
1.
+
+ /*
+ * Disable two-phase here, it will be set in the core if it was
+ * enabled whole creating the slot.
+ */
+ ctx->twophase = false;

Typo, /whole/while. I think we don't need to initialize this variable
here at all.

2.
+ /* If twophase is set on the slot at create time, then
+ * make sure the field in the context is also updated
+ */
+ if (MyReplicationSlot->data.twophase)
+ {
+ ctx->twophase = true;
+ }
+

For multi-line comments, the first line of comment should be empty.
Also, I think this is not the right place because the WALSender path
needs to set it separately. I guess you can set it in
CreateInitDecodingContext/CreateDecodingContext by doing something
like

ctx->twophase &= MyReplicationSlot->data.twophase

3. I think we can support this option at the protocol level in a
separate patch where we need to allow it via replication commands (say
when we support it in CreateSubscription). Right now, there is nothing
to test all the code you have added in repl_gram.y.

4. I think we can expose this new option via pg_replication_slots.


-- 
With Regards,
Amit Kapila.

  60. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-02-27T14:55:05Z

    On Sat, Feb 27, 2021 at 8:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Feb 26, 2021 at 7:26 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > > I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> > > > Do have a look and let me know if there are any comments.
> > >
> > > Update with both patches.
> >
> > Thanks for fixing and providing an updated patch. Patch applies, make
> > check and make check-world passes. I could see the issue working fine.
> >
> > Few minor comments:
> > +       <structfield>snapshot_was_exported_at</structfield> <type>pg_lsn</type>
> > +      </para>
> > +      <para>
> > +       The address (<literal>LSN</literal>) at which the logical
> > +       slot found a consistent point at the time of slot creation.
> > +       <literal>NULL</literal> for physical slots.
> > +      </para></entry>
> > +     </row>
> >
> >
> > I had seen earlier also we had some discussion on naming
> > snapshot_was_exported_at. Can we change snapshot_was_exported_at to
> > snapshot_exported_lsn, I felt if we can include the lsn in the name,
> > the user will be able to interpret easily and also it will be similar
> > to other columns in pg_replication_slots view.
> >
>
> I have recommended above to change this name to initial_consistency_at
> because there are times when we don't export snapshot and we still set
> this like when creating slots with CRS_NOEXPORT_SNAPSHOT or when
> creating via SQL APIs.  I am not sure why Ajin neither changed the
> name nor responded to that comment. What is your opinion?

initial_consistency_at looks good to me. That is more understandable.

Regards,
Vignesh

  61. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-02-27T15:04:07Z

    On Sat, Feb 27, 2021 at 5:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Feb 27, 2021 at 11:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > > I've updated snapshot_was_exported_at_  member to pg_replication_slots as well.
> > > > Do have a look and let me know if there are any comments.
> > >
> > > Update with both patches.
> > >
> >
> > Thanks, I have made some minor changes to the first patch and now it
> > looks good to me. The changes are as below:
> > 1. Removed the changes related to exposing this new parameter via view
> > as mentioned in my previous email.
> > 2. Changed the variable name initial_consistent_point.
> > 3. Ran pgindent, minor changes in comments, and modified the commit message.
> >
> > Let me know what you think about these changes.
> >
>
> In the attached, I have just bumped SNAPBUILD_VERSION  as we are
> adding a new member in the SnapBuild structure.
>

Few minor comments:

git am v6-0001-Avoid-repeated-decoding-of-prepared-transactions-.patch
Applying: Avoid repeated decoding of prepared transactions after the restart.
/home/vignesh/postgres/.git/rebase-apply/patch:286: trailing whitespace.
#define SNAPBUILD_VERSION 4
warning: 1 line adds whitespace errors.

There is one whitespace error.

In commit a271a1b50e, we allowed decoding at prepare time and the prepare
was decoded again if there is a restart after decoding it. It was done
that way because we can't distinguish between the cases where we have not
decoded the prepare because it was prior to consistent snapshot or we have
decoded it earlier but restarted. To distinguish between these two cases,
we have introduced an initial_consisten_point at the slot level which is
an LSN at which we found a consistent point at the time of slot creation.

One minor typo in commit message, initial_consisten_point should be
initial_consistent_point

Regards,
Vignesh

  62. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-03-01T01:53:17Z

    On Sat, Feb 27, 2021 at 11:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> Few comments on 0002 patch:
> =========================
> 1.
> +
> + /*
> + * Disable two-phase here, it will be set in the core if it was
> + * enabled whole creating the slot.
> + */
> + ctx->twophase = false;
>
> Typo, /whole/while. I think we don't need to initialize this variable
> here at all.
>
> 2.
> + /* If twophase is set on the slot at create time, then
> + * make sure the field in the context is also updated
> + */
> + if (MyReplicationSlot->data.twophase)
> + {
> + ctx->twophase = true;
> + }
> +
>
> For multi-line comments, the first line of comment should be empty.
> Also, I think this is not the right place because the WALSender path
> needs to set it separately. I guess you can set it in
> CreateInitDecodingContext/CreateDecodingContext by doing something
> like
>
> ctx->twophase &= MyReplicationSlot->data.twophase

Updated accordingly.

>
> 3. I think we can support this option at the protocol level in a
> separate patch where we need to allow it via replication commands (say
> when we support it in CreateSubscription). Right now, there is nothing
> to test all the code you have added in repl_gram.y.
>

Removed that.


> 4. I think we can expose this new option via pg_replication_slots.
>

Done. Added,

regards,
Ajin Cherian
Fujitsu Australia

  63. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-03-01T05:00:01Z

    On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Sat, Feb 27, 2021 at 11:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > Few comments on 0002 patch:
> > =========================
> > 1.
> > +
> > + /*
> > + * Disable two-phase here, it will be set in the core if it was
> > + * enabled whole creating the slot.
> > + */
> > + ctx->twophase = false;
> >
> > Typo, /whole/while. I think we don't need to initialize this variable
> > here at all.
> >
> > 2.
> > + /* If twophase is set on the slot at create time, then
> > + * make sure the field in the context is also updated
> > + */
> > + if (MyReplicationSlot->data.twophase)
> > + {
> > + ctx->twophase = true;
> > + }
> > +
> >
> > For multi-line comments, the first line of comment should be empty.
> > Also, I think this is not the right place because the WALSender path
> > needs to set it separately. I guess you can set it in
> > CreateInitDecodingContext/CreateDecodingContext by doing something
> > like
> >
> > ctx->twophase &= MyReplicationSlot->data.twophase
>
> Updated accordingly.
>
> >
> > 3. I think we can support this option at the protocol level in a
> > separate patch where we need to allow it via replication commands (say
> > when we support it in CreateSubscription). Right now, there is nothing
> > to test all the code you have added in repl_gram.y.
> >
>
> Removed that.
>
>
> > 4. I think we can expose this new option via pg_replication_slots.
> >
>
> Done. Added,
>

v7-0002-Add-option-to-enable-two-phase-commits-in-pg_crea.patch adds
twophase to pg_create_logical_replication_slot, I feel this option
should be documented in src/sgml/func.sgml.

Regards,
Vignesh

  64. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-03-01T06:01:32Z

    On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian <itsajin@gmail.com> wrote:
>

Pushed, the first patch in the series.

-- 
With Regards,
Amit Kapila.

  65. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-03-01T09:08:45Z

    On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
Few minor comments on 0002 patch
=============================
1.
  ctx->streaming &= enable_streaming;
- ctx->twophase &= enable_twophase;
+
 }

Spurious line addition.

2.
-  proallargtypes =>
'{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames =>
'{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
+  proallargtypes =>
'{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames =>
'{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
   prosrc => 'pg_get_replication_slots' },
 { oid => '3786', descr => 'set up a logical replication slot',
   proname => 'pg_create_logical_replication_slot', provolatile => 'v',
-  proparallel => 'u', prorettype => 'record', proargtypes => 'name name bool',
-  proallargtypes => '{name,name,bool,name,pg_lsn}',
-  proargmodes => '{i,i,i,o,o}',
-  proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
+  proparallel => 'u', prorettype => 'record', proargtypes => 'name
name bool bool',
+  proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
+  proargmodes => '{i,i,i,i,o,o}',
+  proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',

I think it is better to use two_phase here and at other places as well
to be consistent with similar parameters.

3.
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
             L.restart_lsn,
             L.confirmed_flush_lsn,
             L.wal_status,
-            L.safe_wal_size
+            L.safe_wal_size,
+ L.twophase
     FROM pg_get_replication_slots() AS L

Indentation issue. Here, you need you spaces instead of tabs.

4.
@@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,

  ctx->reorder->output_rewrites = ctx->options.receive_rewrites;

+ /*
+ * If twophase is set on the slot at create time, then
+ * make sure the field in the context is also updated.
+ */
+ ctx->twophase &= MyReplicationSlot->data.twophase;
+

Why didn't you made similar change in CreateInitDecodingContext when I
already suggested the same in my previous email? If we don't make that
change then during slot initialization two_phase will always be true
even though user passed in as false. It looks inconsistent and even
though there is no direct problem due to that but it could be cause of
possible problem in future.

-- 
With Regards,
Amit Kapila.

  66. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-03-02T01:07:06Z

    On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> Few minor comments on 0002 patch
> =============================
> 1.
>   ctx->streaming &= enable_streaming;
> - ctx->twophase &= enable_twophase;
> +
>  }
>
> Spurious line addition.

Deleted.

>
> 2.
> -  proallargtypes =>
> '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
> -  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
> -  proargnames =>
> '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
> +  proallargtypes =>
> '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
> +  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> +  proargnames =>
> '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
>    prosrc => 'pg_get_replication_slots' },
>  { oid => '3786', descr => 'set up a logical replication slot',
>    proname => 'pg_create_logical_replication_slot', provolatile => 'v',
> -  proparallel => 'u', prorettype => 'record', proargtypes => 'name name bool',
> -  proallargtypes => '{name,name,bool,name,pg_lsn}',
> -  proargmodes => '{i,i,i,o,o}',
> -  proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
> +  proparallel => 'u', prorettype => 'record', proargtypes => 'name
> name bool bool',
> +  proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
> +  proargmodes => '{i,i,i,i,o,o}',
> +  proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',
>
> I think it is better to use two_phase here and at other places as well
> to be consistent with similar parameters.

Updated as requested.
>
> 3.
> --- a/src/backend/catalog/system_views.sql
> +++ b/src/backend/catalog/system_views.sql
> @@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
>              L.restart_lsn,
>              L.confirmed_flush_lsn,
>              L.wal_status,
> -            L.safe_wal_size
> +            L.safe_wal_size,
> + L.twophase
>      FROM pg_get_replication_slots() AS L
>
> Indentation issue. Here, you need you spaces instead of tabs.

Updated.
>
> 4.
> @@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,
>
>   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
>
> + /*
> + * If twophase is set on the slot at create time, then
> + * make sure the field in the context is also updated.
> + */
> + ctx->twophase &= MyReplicationSlot->data.twophase;
> +
>
> Why didn't you made similar change in CreateInitDecodingContext when I
> already suggested the same in my previous email? If we don't make that
> change then during slot initialization two_phase will always be true
> even though user passed in as false. It looks inconsistent and even
> though there is no direct problem due to that but it could be cause of
> possible problem in future.

Updated.

regards,
Ajin Cherian
Fujitsu Australia

  67. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-03-02T02:50:33Z

    On Tue, Mar 2, 2021 at 6:37 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Mon, Mar 1, 2021 at 8:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > Few minor comments on 0002 patch
> > =============================
> > 1.
> >   ctx->streaming &= enable_streaming;
> > - ctx->twophase &= enable_twophase;
> > +
> >  }
> >
> > Spurious line addition.
>
> Deleted.
>
> >
> > 2.
> > -  proallargtypes =>
> > '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8}',
> > -  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > -  proargnames =>
> > '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size}',
> > +  proallargtypes =>
> > '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool}',
> > +  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> > +  proargnames =>
> > '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,twophase}',
> >    prosrc => 'pg_get_replication_slots' },
> >  { oid => '3786', descr => 'set up a logical replication slot',
> >    proname => 'pg_create_logical_replication_slot', provolatile => 'v',
> > -  proparallel => 'u', prorettype => 'record', proargtypes => 'name name bool',
> > -  proallargtypes => '{name,name,bool,name,pg_lsn}',
> > -  proargmodes => '{i,i,i,o,o}',
> > -  proargnames => '{slot_name,plugin,temporary,slot_name,lsn}',
> > +  proparallel => 'u', prorettype => 'record', proargtypes => 'name
> > name bool bool',
> > +  proallargtypes => '{name,name,bool,bool,name,pg_lsn}',
> > +  proargmodes => '{i,i,i,i,o,o}',
> > +  proargnames => '{slot_name,plugin,temporary,twophase,slot_name,lsn}',
> >
> > I think it is better to use two_phase here and at other places as well
> > to be consistent with similar parameters.
>
> Updated as requested.
> >
> > 3.
> > --- a/src/backend/catalog/system_views.sql
> > +++ b/src/backend/catalog/system_views.sql
> > @@ -894,7 +894,8 @@ CREATE VIEW pg_replication_slots AS
> >              L.restart_lsn,
> >              L.confirmed_flush_lsn,
> >              L.wal_status,
> > -            L.safe_wal_size
> > +            L.safe_wal_size,
> > + L.twophase
> >      FROM pg_get_replication_slots() AS L
> >
> > Indentation issue. Here, you need you spaces instead of tabs.
>
> Updated.
> >
> > 4.
> > @@ -533,6 +533,12 @@ CreateDecodingContext(XLogRecPtr start_lsn,
> >
> >   ctx->reorder->output_rewrites = ctx->options.receive_rewrites;
> >
> > + /*
> > + * If twophase is set on the slot at create time, then
> > + * make sure the field in the context is also updated.
> > + */
> > + ctx->twophase &= MyReplicationSlot->data.twophase;
> > +
> >
> > Why didn't you made similar change in CreateInitDecodingContext when I
> > already suggested the same in my previous email? If we don't make that
> > change then during slot initialization two_phase will always be true
> > even though user passed in as false. It looks inconsistent and even
> > though there is no direct problem due to that but it could be cause of
> > possible problem in future.
>
> Updated.
>

I have a minor comment regarding the below:
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>two_phase</structfield> <type>bool</type>
+      </para>
+      <para>
+      True if two-phase commits are enabled on this slot.
+      </para></entry>
+     </row>

Can we change something like:
True if the slot is enabled for decoding prepared transaction
information. Refer link for more information.(link should point where
more detailed information is available for two-phase in
pg_create_logical_replication_slot).

Also there is one small indentation in that line, I think there should
be one space before "True if....".

Regards,
Vignesh

  68. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-03-02T04:03:37Z

    On Tue, Mar 2, 2021 at 8:20 AM vignesh C <vignesh21@gmail.com> wrote:
>
>
> I have a minor comment regarding the below:
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>two_phase</structfield> <type>bool</type>
> +      </para>
> +      <para>
> +      True if two-phase commits are enabled on this slot.
> +      </para></entry>
> +     </row>
>
> Can we change something like:
> True if the slot is enabled for decoding prepared transaction
> information. Refer link for more information.(link should point where
> more detailed information is available for two-phase in
> pg_create_logical_replication_slot).
>
> Also there is one small indentation in that line, I think there should
> be one space before "True if....".
>

Okay, fixed these but I added a slightly different description. I have
also added the parameter description for
pg_create_logical_replication_slot in docs and changed the comments at
various places in the code. Apart from that ran pgindent. The patch
looks good to me now. Let me know what do you think?

-- 
With Regards,
Amit Kapila.

  69. Re: repeated decoding of prepared transactions

    Ajin Cherian <itsajin@gmail.com> — 2021-03-02T05:08:34Z

    On Tue, Mar 2, 2021 at 3:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

One minor comment:
+      </para>
+      <para>
+       True if the slot is enabled for decoding prepared transactions.  Always
+       false for physical slots.
+      </para></entry>
+     </row>

There is an extra space before Always. But when rendered in html this
is not seen, so this might not be a problem.

Other than that no more comments about the patch. Looks good.

regards,
Ajin Cherian
Fujitsu Australia

  70. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-03-02T05:28:21Z

    On Tue, Mar 2, 2021 at 10:38 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Mar 2, 2021 at 3:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> One minor comment:
> +      </para>
> +      <para>
> +       True if the slot is enabled for decoding prepared transactions.  Always
> +       false for physical slots.
> +      </para></entry>
> +     </row>
>
> There is an extra space before Always. But when rendered in html this
> is not seen, so this might not be a problem.
>

I am just trying to be consistent with the nearby description. For example, see:
"The number of bytes that can be written to WAL such that this slot is
not in danger of getting in state "lost".  It is NULL for lost slots,
as well as if <varname>max_slot_wal_keep_size</varname> is
<literal>-1</literal>."

In Pg docs, comments, you will find that there are places where we use
a single space before the new line and also places where we use two
spaces. In this case, for the sake of consistency with the nearby
description, I used two spaces.

-- 
With Regards,
Amit Kapila.

  71. Re: repeated decoding of prepared transactions

    vignesh C <vignesh21@gmail.com> — 2021-03-02T07:13:46Z

    On Tue, Mar 2, 2021 at 9:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 2, 2021 at 8:20 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> >
> > I have a minor comment regarding the below:
> > +     <row>
> > +      <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>two_phase</structfield> <type>bool</type>
> > +      </para>
> > +      <para>
> > +      True if two-phase commits are enabled on this slot.
> > +      </para></entry>
> > +     </row>
> >
> > Can we change something like:
> > True if the slot is enabled for decoding prepared transaction
> > information. Refer link for more information.(link should point where
> > more detailed information is available for two-phase in
> > pg_create_logical_replication_slot).
> >
> > Also there is one small indentation in that line, I think there should
> > be one space before "True if....".
> >
>
> Okay, fixed these but I added a slightly different description. I have
> also added the parameter description for
> pg_create_logical_replication_slot in docs and changed the comments at
> various places in the code. Apart from that ran pgindent. The patch
> looks good to me now. Let me know what do you think?

Patch applies cleanly, make check and make check-world passes. I did
not find any other issue. The patch looks good to me.

Regards,
Vignesh

  72. Re: repeated decoding of prepared transactions

    Amit Kapila <amit.kapila16@gmail.com> — 2021-03-03T03:42:44Z

    On Tue, Mar 2, 2021 at 12:43 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Mar 2, 2021 at 9:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 2, 2021 at 8:20 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > >
> > > I have a minor comment regarding the below:
> > > +     <row>
> > > +      <entry role="catalog_table_entry"><para role="column_definition">
> > > +       <structfield>two_phase</structfield> <type>bool</type>
> > > +      </para>
> > > +      <para>
> > > +      True if two-phase commits are enabled on this slot.
> > > +      </para></entry>
> > > +     </row>
> > >
> > > Can we change something like:
> > > True if the slot is enabled for decoding prepared transaction
> > > information. Refer link for more information.(link should point where
> > > more detailed information is available for two-phase in
> > > pg_create_logical_replication_slot).
> > >
> > > Also there is one small indentation in that line, I think there should
> > > be one space before "True if....".
> > >
> >
> > Okay, fixed these but I added a slightly different description. I have
> > also added the parameter description for
> > pg_create_logical_replication_slot in docs and changed the comments at
> > various places in the code. Apart from that ran pgindent. The patch
> > looks good to me now. Let me know what do you think?
>
> Patch applies cleanly, make check and make check-world passes. I did
> not find any other issue. The patch looks good to me.
>

Thanks, I have pushed this patch.

-- 
With Regards,
Amit Kapila.