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

On Thu, Jul 29, 2021 at 9:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jul 27, 2021 at 11:41 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Please find attached the latest patch set v99*
> >
> > v98-0001 --> split into v99-0001 + v99-0002
> >
>
> Pushed the first refactoring patch after making few modifications as below.
> 1.
> - /* open the spool file for the committed transaction */
> + /* Open the spool file for the committed/prepared transaction */
> changes_filename(path, MyLogicalRepWorker->subid, xid);
>
> In the above comment, we don't need to say prepared. It can be done as
> part of the second patch.
>
Updated comment in v100.

> 2.
> +apply_handle_prepare_internal(LogicalRepPreparedTxnData
> *prepare_data, char *gid)
>
> I don't think there is any need for this function to take gid as
> input. It can compute by itself instead of callers doing it.
>
OK.

> 3.
> +static TransactionId+logicalrep_read_prepare_common(StringInfo in,
> char *msgtype,
> + LogicalRepPreparedTxnData *prepare_data)
>
> I don't think the above function needs to return xid because it is
> already present as part of prepare_data. Even, if it is required due
> to some reason for the second patch then let's do it as part of if but
> I don't think it is required for the second patch.
>
OK.

> 4.
> /*
> - * Write PREPARE to the output stream.
> + * Common code for logicalrep_write_prepare and
> logicalrep_write_stream_prepare.
> */
>
> Here and at a similar another place, we don't need to refer to
> logicalrep_write_stream_prepare as that is part of the second patch.
>
Updated comment in v100

> Few comments on 0002 patch:
> ==========================
> 1.
> +# ---------------------
> +# 2PC + STREAMING TESTS
> +# ---------------------
> +
> +# Setup logical replication (streaming = on)
> +
> +$node_B->safe_psql('postgres', "
> + ALTER SUBSCRIPTION tap_sub_B
> + SET (streaming = on);");
> +
> +$node_C->safe_psql('postgres', "
> + ALTER SUBSCRIPTION tap_sub_C
> + SET (streaming = on)");
> +
> +# Wait for subscribers to finish initialization
> +$node_A->wait_for_catchup($appname_B);
> +$node_B->wait_for_catchup($appname_C);
>
> This is not the right way to determine if the new streaming option is
> enabled on the publisher. Even if there is no restart of apply workers
> (and walsender) after you have enabled the option, the above wait will
> succeed. You need to do something like below as we are doing in
> 001_rep_changes.pl:
>
> $oldpid = $node_publisher->safe_psql('postgres',
> "SELECT pid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
> );
> $node_subscriber->safe_psql('postgres',
> "ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only WITH
> (copy_data = false)"
> );
> $node_publisher->poll_query_until('postgres',
> "SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name
> = 'tap_sub';"
> ) or die "Timed out while waiting for apply to restart";
>
Fixed in v100 as suggested.

> 2.
> +# Create some pre-existing content on publisher (uses same DDL as
> 015_stream test)
>
> Here, in the comments, I don't see the need to same uses same DDL ...
>
Fixed in v100. Comment removed.

------
Kind Regards,
Peter Smith.
Fujitsu Australia