Re: repeated decoding of prepared transactions

On Fri, Feb 19, 2021 at 8:21 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Based on this suggestion, I have created a patch on HEAD which now
> does not allow repeated decoding
> of prepared transactions. For this, the code now enforces
> full_snapshot if two-phase decoding is enabled.
> Do have a look at the patch and see if you have any comments.
>

Few minor comments:
===================
1.
.git/rebase-apply/patch:135: trailing whitespace.
* We need to mark the transaction as prepared, so that we
don't resend it on
warning: 1 line adds whitespace errors.

Whitespace issue.

2.
/*
+ * Set snapshot type
+ */
+void
+SetSnapBuildType(SnapBuild *builder, bool need_full_snapshot)

There is no caller which passes the second parameter as false, so why
have it? Can't we have a function with SetSnapBuildFullSnapshot or
something like that?

3.
@@ -431,6 +431,10 @@ CreateInitDecodingContext(const char *plugin,
startup_cb_wrapper(ctx, &ctx->options, true);
MemoryContextSwitchTo(old_context);

+ /* If two-phase is on, then only full snapshot can be used */
+ if (ctx->twophase)
+ SetSnapBuildType(ctx->snapshot_builder, true);
+
ctx->reorder->output_rewrites = ctx->options.receive_rewrites;

return ctx;
@@ -534,6 +538,10 @@ CreateDecodingContext(XLogRecPtr start_lsn,

ctx->reorder->output_rewrites = ctx->options.receive_rewrites;

+ /* If two-phase is on, then only full snapshot can be used */
+ if (ctx->twophase)
+ SetSnapBuildType(ctx->snapshot_builder, true);

I think it is better to add a detailed comment on why we are doing
this? You can write the comment in one of the places.

> Currently one problem with this, as you have also mentioned in your
> last mail, is that if initially two-phase is disabled in
> test_decoding while
> decoding prepare (causing the prepared transaction to not be decoded)
> and later enabled after the commit prepared (where it assumes that the
> transaction was decoded at prepare time), then the transaction is not
> decoded at all. For eg:
>
> postgres=# begin;
> BEGIN
> postgres=*# INSERT INTO do_write DEFAULT VALUES;
> INSERT 0 1
> postgres=*# PREPARE TRANSACTION 'test1';
> PREPARE TRANSACTION
> postgres=# SELECT data FROM
> pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
> 'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
> '0');
> data
> ------
> (0 rows)
> postgres=# commit prepared 'test1';
> COMMIT PREPARED
> postgres=# SELECT data FROM
> pg_logical_slot_get_changes('isolation_slot', NULL, NULL,
> 'include-xids', 'false', 'skip-empty-xacts', '1', 'two-phase-commit',
> '1');
> data
> -------------------------
> COMMIT PREPARED 'test1' (1 row)
>
> 1st pg_logical_slot_get_changes is called with two-phase-commit off,
> 2nd is called with two-phase-commit on. You can see that the
> transaction is not decoded at all.
> For this, I am planning to change the semantics such that
> two-phase-commit can only be specified while creating the slot using
> pg_create_logical_replication_slot()
> and not in pg_logical_slot_get_changes, thus preventing
> two-phase-commit flag from being toggled between restarts of the
> decoder.
>

+1.

--
With Regards,
Amit Kapila.