From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Ajin Cherian <itsajin(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] logical decoding of two-phase transactions |
Date: | 2020-10-21 08:37:53 |
Message-ID: | CAA4eK1+njaug+7RLMuos2zrXst3QskG+m_uGQMe6nME1W9-vVg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Oct 20, 2020 at 4:32 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, Oct 16, 2020 at 5:21 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
>
> Comments:
>
> src/backend/replication/logical/worker.c
> @@ -888,6 +888,319 @@ apply_handle_prepare(StringInfo s)
> + /*
> + * FIXME - Following condition was in apply_handle_prepare_txn except
> I found it was ALWAYS IsTransactionState() == false
> + * The synchronization worker runs in single transaction. *
> + if (IsTransactionState() && !am_tablesync_worker())
> + */
> + if (!am_tablesync_worker())
>
> Comment: I dont think a tablesync worker will use streaming, none of
> the other stream APIs check this, this might not be relevant for
> stream_prepare either.
>
Yes, I think this is right. See pgoutput_startup where we are
disabling the streaming for init phase. But it is always good to once
test this and ensure the same.
>
> + /*
> + * ==================================================================================================
> + * The following chunk of code is largely cut/paste from the existing
> apply_handle_prepare_commit_txn
>
> Comment: Here, I think you meant apply_handle_stream_commit. Also
> rather than duplicating this chunk of code, you could put it in a new
> function.
>
> + /* open the spool file for the committed transaction */
> + changes_filename(path, MyLogicalRepWorker->subid, xid);
>
> Comment: Here the comment should read "committed/prepared" rather than
> "committed"
>
>
> + else
> + {
> + /* Process any invalidation messages that might have accumulated. */
> + AcceptInvalidationMessages();
> + maybe_reread_subscription();
> + }
>
> Comment: This else block might not be necessary as a tablesync worker
> will not initiate the streaming APIs.
>
I think it is better to have an Assert here for streaming-mode?
> + BeginTransactionBlock();
> + CommitTransactionCommand();
> + StartTransactionCommand();
>
> Comment: Rereading the code and the transaction state description in
> src/backend/access/transam/README. I am not entirely sure if the
> BeginTransactionBlock followed by CommitTransactionBlock is really
> needed here.
>
Yeah, I also find this strange. I guess the patch is doing so because
it needs to call PrepareTransactionBlock later but I am not sure. How
can we call CommitTransactionCommand(), won't it commit the on-going
transaction and make it visible before even it is visible on the
publisher. I think you can verify by having a breakpoint after
CommitTransactionCommand() and see if the changes for which we are
doing prepare become visible.
> I understand this code was copied over from apply_handle_prepare_txn,
> but now looking back I'm not so sure if it is correct. The transaction
> would have already begin as part of applying the changes, why begin it
> again?
> Maybe Amit could confirm this.
>
I hope the above suggestions will help to proceed here.
--
With Regards,
Amit Kapila.
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2020-10-21 08:43:08 | Re: [HACKERS] logical decoding of two-phase transactions |
Previous Message | Dilip Kumar | 2020-10-21 08:29:50 | Re: [HACKERS] Custom compression methods |