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

On Fri, Sep 18, 2020 at 6:02 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Sep 15, 2020 at 10:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > I don't think it is complete yet.
> > *
> > * This error can only occur when we are sending the data in
> >   * streaming mode and the streaming is not finished yet.
> >   */
> > - Assert(streaming);
> > - Assert(stream_started);
> > + Assert(streaming || rbtxn_prepared(txn));
> > + Assert(stream_started  || rbtxn_prepared(txn));
> >
> > Here, you have updated the code but comments are still not updated.
> >
>
> Updated the comments.
>
>
> > I don't think we need to perform abort here. Later we will anyway
> > encounter the WAL for Rollback Prepared for which we will call
> > abort_prepared_cb. As we have set the 'concurrent_abort' flag, it will
> > allow us to skip all the intermediate records. Here, we need only
> > enough state in ReorderBufferTxn that it can be later used for
> > ReorderBufferFinishPrepared(). Basically, you need functionality
> > similar to ReorderBufferTruncateTXN where except for invalidations you
> > can free memory for everything else. You can either write a new
> > function ReorderBufferTruncatePreparedTxn or pass another bool
> > parameter in ReorderBufferTruncateTXN to indicate it is prepared_xact
> > and then clean up additional things that are not required for prepared
> > xact.
>
> Added a new parameter to ReorderBufferTruncatePreparedTxn  for
> prepared transactions and did cleanup of tupulecids as well, I have
> left snapshots and transactions.
> As a result of this, I also had to create a new function
> ReorderBufferCleanupPreparedTXN which will clean up the rest as part
> of FinishPrepared handling as we can't call
> ReorderBufferCleanupTXN again after this.
>

Why can't we call ReorderBufferCleanupTXN() from
ReorderBufferFinishPrepared after your changes?

+ * If streaming, keep the remaining info - transactions, tuplecids,
invalidations and
+ * snapshots.If after a PREPARE, keep only the invalidations and snapshots.
  */
 static void
-ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
+ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
bool txn_prepared)

Why do we need even snapshot for Prepared transactions? Also, note
that in the comment there is no space before you start a new line.

>
> > >>
> > >>
> > >> I don't know why the patch has used this way to implement an option to
> > >> enable two-phase. Can't we use how we implement 'stream-changes'
> > >> option in commit 7259736a6e? Just refer how we set ctx->streaming and
> > >> you can use a similar way to set this parameter.
> > >
> > >
> > > Done, I've moved the checks for callbacks to inside the corresponding wrappers.
> > >
> >
> > This is not what I suggested. Please study the commit 7259736a6e and
> > see how streaming option is implemented. I want later subscribers can
> > specify whether they want transactions to be decoded at prepare time
> > similar to what we have done for streaming. Also, search for
> > ctx->streaming in the code and see how it is set to get the idea.
> >
>
> Changed it similar to ctx->streaming logic.
>

Hmm, I still don't see changes relevant changes in pg_decode_startup().

-- 
With Regards,
Amit Kapila.