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

On Mon, Mar 8, 2021 at 7:17 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Please find attached the latest patch set v52*
>

Few more comments:
==================
1.
/* CREATE_REPLICATION_SLOT slot TEMPORARY LOGICAL plugin */
- | K_CREATE_REPLICATION_SLOT IDENT opt_temporary K_LOGICAL IDENT
create_slot_opt_list
+ | K_CREATE_REPLICATION_SLOT IDENT opt_temporary opt_two_phase
K_LOGICAL IDENT create_slot_opt_list

I think the comment above can have TWO_PHASE option listed.

2.
+static void
+apply_handle_begin_prepare(StringInfo s)
+{
..
/*
+ * From now, until the handle_prepare we are spooling to the
+ * current psf.
+ */
+ psf_cur.is_spooling = true;
+ }
+ }
+
+ remote_final_lsn = begin_data.final_lsn;
+
+ in_remote_transaction = true;
+
+ pgstat_report_activity(STATE_RUNNING, NULL);

In case you are spooling the changes, you don't need to set
remote_final_lsn and in_remote_transaction. You only need to probably
do pgstat_report_activity.

3.
Similarly, you don't need to set remote_final_lsn as false in
apply_handle_prepare for the spooling case, rather there should be an
Assert stating that remote_final_lsn is false.

4.
snprintf(path, MAXPGPATH, "pg_twophase/%u-%s.prep_changes", subid, gid);

I feel it is better to create these in pg_logical/twophase as that is
where we store other logical replication related files.

--
With Regards,
Amit Kapila.