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

On Mon, Mar 8, 2021 at 4:59 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:

> 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 comments:
>
> +logicalrep_read_begin_prepare(StringInfo in,
> LogicalRepBeginPrepareData *begin_data)
> +{
> + /* read fields */
> + begin_data->final_lsn = pq_getmsgint64(in);
> + if (begin_data->final_lsn == InvalidXLogRecPtr)
> + elog(ERROR, "final_lsn not set in begin message");
> + begin_data->end_lsn = pq_getmsgint64(in);
> + if (begin_data->end_lsn == InvalidXLogRecPtr)
> + elog(ERROR, "end_lsn not set in begin message");
> + begin_data->committime = pq_getmsgint64(in);
> + begin_data->xid = pq_getmsgint(in, 4);
> +
> + /* read gid (copy it into a pre-allocated buffer) */
> + strcpy(begin_data->gid, pq_getmsgstring(in));
> +}
> In logicalrep_read_begin_prepare we validate final_lsn & end_lsn. But
> this validation is not done in logicalrep_read_commit_prepared and
> logicalrep_read_rollback_prepared. Should we keep it consistent?
>

Updated.

>
> @@ -170,5 +237,4 @@ extern void
> logicalrep_write_stream_abort(StringInfo out, TransactionId xid,
>
> TransactionId subxid);
> extern void logicalrep_read_stream_abort(StringInfo in, TransactionId
> *xid,
>
> TransactionId *subxid);
> -
> #endif /* LOGICAL_PROTO_H
> */
> This change is not required.
>

Removed.

>
> @@ -242,15 +244,16 @@ create_replication_slot:
> $$ = (Node *) cmd;
> }
> /* 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
> {
> CreateReplicationSlotCmd *cmd;
> cmd =
> makeNode(CreateReplicationSlotCmd);
> cmd->kind =
> REPLICATION_KIND_LOGICAL;
> cmd->slotname = $2;
> cmd->temporary = $3;
> - cmd->plugin = $5;
> - cmd->options = $6;
> + cmd->two_phase = $4;
> + cmd->plugin = $6;
> + cmd->options = $7;
> $$ = (Node *) cmd;
> }
> Should we document two_phase in the below section:
> CREATE_REPLICATION_SLOT slot_name [ TEMPORARY ] { PHYSICAL [
> RESERVE_WAL ] | LOGICAL output_plugin [ EXPORT_SNAPSHOT |
> NOEXPORT_SNAPSHOT | USE_SNAPSHOT ] }
> Create a physical or logical replication slot. See Section 27.2.6 for
> more about replication slots.
>
> Updated in protocol.sgml as well as the comment above.

regards,
Ajin Cherian
Fujitsu Australia