Horton

Re: [HACKERS] logical decoding of two-phase transactions

Ajin Cherian <itsajin@gmail.com>

View thread View on postgr.es
From: Ajin Cherian <itsajin@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2021-05-28T03:44:12Z
Lists: pgsql-hackers

Attachments

On Wed, May 26, 2021 at 6:53 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, May 25, 2021 at 8:54 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, May 21, 2021 at 6:43 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > > Fixed in v77-0001, v77-0002
> >
> > Attaching a new patch-set that rebases the patch, addresses review
> > comments from Peter as well as a test failure reported by Tang. I've
> > also added some new test case into patch-2 authored by Tang.
>
> Thanks for the updated patch, few comments:
> 1)  Should "The end LSN of the prepare." be changed to "end LSN of the
> prepare transaction."?

No, this is the end LSN of the prepare. The prepare consists of multiple LSNs.

> 2) Should the ";" be "," here?
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -7639,6 +7639,18 @@ SCRAM-SHA-256$<replaceable>&lt;iteration
> count&gt;</replaceable>:<replaceable>&l
>
>       <row>
>        <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>subtwophasestate</structfield> <type>char</type>
> +      </para>
> +      <para>
> +       State code:
> +       <literal>d</literal> = two_phase mode was not requested, so is disabled;
> +       <literal>p</literal> = two_phase mode was requested, but is
> pending enablement;
> +       <literal>e</literal> = two_phase mode was requested, and is enabled.
> +      </para></entry>

no, I think the ";" is correct here, connecting multiple parts of the sentence.

>
> 3) Should end_lsn be commit_end_lsn?
> +       prepare_data->commit_end_lsn = pq_getmsgint64(in);
> +       if (prepare_data->commit_end_lsn == InvalidXLogRecPtr)
>                 elog(ERROR, "end_lsn is not set in commit prepared message");
> +       prepare_data->prepare_time = pq_getmsgint64(in);

Changed this.

>
> 4) This change is not required
>
> diff --git a/src/include/replication/pgoutput.h
> b/src/include/replication/pgoutput.h
> index 0dc460f..93c6731 100644
> --- a/src/include/replication/pgoutput.h
> +++ b/src/include/replication/pgoutput.h
> @@ -29,5 +29,4 @@ typedef struct PGOutputData
>         bool            messages;
>         bool            two_phase;
>  } PGOutputData;
> -

removed.


>  #endif                                                 /* PGOUTPUT_H */
>
> 5) Will the worker receive commit prepared/rollback prepared as we
> have skip logic to skip commit prepared / commit rollback in
> pgoutput_rollback_prepared_txn and pgoutput_commit_prepared_txn:
>
> +        * It is possible that we haven't received the prepare because
> +        * the transaction did not have any changes relevant to this
> +        * subscription and was essentially an empty prepare. In which case,
> +        * the walsender is optimized to drop the empty transaction and the
> +        * accompanying prepare. Silently ignore if we don't find the prepared
> +        * transaction.
>          */
> -       replorigin_session_origin_lsn = prepare_data.end_lsn;
> -       replorigin_session_origin_timestamp = prepare_data.commit_time;
> +       if (LookupGXact(gid, prepare_data.prepare_end_lsn,
> +                                       prepare_data.prepare_time))
> +       {
>
Commit prepared will be skipped if it happens in the same walsender's
lifetime. But if the walsender restarts it no longer
knows about the skipped prepare. In this case walsender will not skip
the commit prepared. Hence, the logic for handling
stray commit prepared in the apply worker.


> 6) I'm not sure if we could add some tests for skip empty prepare
> transactions, if possible add few tests.

I've added a test case using pg_logical_slot_peek_binary_changes() for
empty prepares
 have a look.

>
> 7) We could add some debug level log messages for the transaction that
> will be skipped.

If this is for the test, I was able to add a test without debug messages.

regards,
Ajin Cherian
Fujitsu Australia

Commits

  1. Add prepare API support for streaming transactions in logical replication.

  2. Doc: minor improvements for logical replication protocol documentation.

  3. Fix test failure in 021_twophase.pl.

  4. Refactor to make common functions in proto.c and worker.c.

  5. Unify PostgresNode's new() and get_new_node() methods

  6. Fix potential buffer overruns in proto.c.

  7. Add support for prepared transactions to built-in logical replication.

  8. Refactor function parse_subscription_options.

  9. Allow enabling two-phase option via replication protocol.

  10. Don't use Asserts to check for violations of replication protocol.

  11. Improve psql tab completion for options of subcriptions and publications

  12. Rearrange logrep worker's snapshot handling some more.

  13. doc: Update information of new messages for logical replication.

  14. ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION

  15. Allow pgoutput to send logical decoding messages.

  16. Refactor function parse_output_parameters.

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

  18. Fix an oversight in ReorderBufferFinishPrepared.

  19. Allow multiple xacts during table sync in logical replication.

  20. Fix replication of in-progress transactions in tablesync worker.

  21. Fix 'skip-empty-xacts' option in test_decoding for streaming mode.

  22. Use Enum for top level logical replication message types.

  23. Add support for streaming to built-in logical replication.

  24. Fix the logical streaming test.

  25. Implement streaming mode in ReorderBuffer.

  26. Extend the logical decoding output plugin API with stream methods.

  27. Store 2PC GID in commit/abort WAL recs for logical decoding