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

On Fri, Mar 5, 2021 at 12:21 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Thu, Mar 4, 2021 at 9:53 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> > [05a] Now syncing the psf file at prepare time
>
> The patch v46-0008 does not handle spooling of streaming prepare if
> the Subscription is configured for both two-phase and streaming.
> I feel that it would be best if we don't support both two-phase and
> streaming together in a subscription in this release.
> Probably a future release could handle this. So, changing the patch to
> not allow streaming and two-phase together.
> This new patch v49 has the following changes.
>
> * Don't support creating a subscription with both streaming and
> two-phase enabled.
> * Don't support altering a subscription enabling streaming if it was
> created with two-phase enabled.
> * Remove stream_prepare callback as a "required" callback, make it an
> optional callback and remove all code related to stream_prepare in the
> pgoutput plugin as well as in worker.c
>
> Also fixed
> * Don't support the alter of subscription setting two-phase. Toggling
> of two-phase mode using the alter command on the subscription can
> cause transactions to be missed and result in an inconsistent replica.
>

Thanks for the updated patch.
Few minor comments:

I'm not sure if we plan to change this workaround, if we are not
planning to change this workaround. We can reword the comments
suitably. We generally don't use workaround in our comments.
+ /*
+ * Workaround Part 1 of 2:
+ *
+ * Make sure every tablesync has reached at least SYNCDONE state
+ * before letting the apply worker proceed.
+ */
+ elog(DEBUG1,
+ "apply_handle_begin_prepare, end_lsn = %X/%X,
final_lsn = %X/%X, lstate_lsn = %X/%X",
+ LSN_FORMAT_ARGS(begin_data.end_lsn),
+ LSN_FORMAT_ARGS(begin_data.final_lsn),
+ LSN_FORMAT_ARGS(MyLogicalRepWorker->relstate_lsn));
+

We should include two_phase in tab completion (tab-complete.c file
psql_completion(const char *text, int start, int end) function) :
postgres=# create subscription sub1 connection 'port=5441
dbname=postgres' publication pub1 with (
CONNECT COPY_DATA CREATE_SLOT ENABLED
SLOT_NAME SYNCHRONOUS_COMMIT

+
+ <para>
+ It is not allowed to combine <literal>streaming</literal> set to
+ <literal>true</literal> and <literal>two_phase</literal> set to
+ <literal>true</literal>.
+ </para>
+
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>two_phase</literal> (<type>boolean</type>)</term>
+ <listitem>
+ <para>
+ Specifies whether two-phase commit is enabled for this subscription.
+ The default is <literal>false</literal>.
+ </para>
+
+ <para>
+ When two-phase commit is enabled then the decoded
transactions are sent
+ to the subscriber on the PREPARE TRANSACTION. By default,
the transaction
+ preapred on publisher is decoded as normal transaction at commit.
+ </para>
+
+ <para>
+ It is not allowed to combine <literal>two_phase</literal> set to
+ <literal>true</literal> and <literal>streaming</literal> set to
+ <literal>true</literal>.
+ </para>

It is not allowed to combine streaming set to true and two_phase set to true.
Should this be:
streaming option is not supported along with two_phase option.

Similarly here too:
It is not allowed to combine two_phase set to true and streaming set to true.
Should this be:
two_phase option is not supported along with streaming option.

Few indentation issues are present, we can run pgindent:
+extern void logicalrep_write_prepare(StringInfo out, ReorderBufferTXN *txn,
+
XLogRecPtr prepare_lsn);
+extern void logicalrep_read_prepare(StringInfo in,
+
LogicalRepPreparedTxnData *prepare_data);
+extern void logicalrep_write_commit_prepared(StringInfo out,
ReorderBufferTXN* txn,
+
XLogRecPtr commit_lsn);

ReorderBufferTXN * should be ReorderBufferTXN*

Line exceeds 80 chars:
+ /*
+ * Now that we replayed the psf it is no longer
needed. Just delete it.
+ */
+ prepare_spoolfile_delete(psfpath);

There is a typo, preapred should be prepared.
+ <para>
+ When two-phase commit is enabled then the decoded
transactions are sent
+ to the subscriber on the PREPARE TRANSACTION. By default,
the transaction
+ preapred on publisher is decoded as normal transaction at commit.
+ </para>

Regards,
Vignesh