On Thu, Aug 4, 2022 at 1:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Thank you for working on this. I have a comment and a question:
>
> * This has to be done after updating the state
> because otherwise if
> * there is an error while doing the database
> operations we won't be
> - * able to rollback dropped slot.
> + * able to rollback dropped slot or origin tracking.
>
> I think we can actually roll back dropping the replication origin. So
> the above comment is true for only replication slots.
>
Right, this comment should be updated.
> ---
> + replorigin_session_reset();
> + replorigin_session_origin = InvalidRepOriginId;
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
> +
> + replorigin_drop_by_name(originname, true, false);
>
> With this change, the committing the ongoing transaction will be done
> without replication origin. Is this okay? it's probably okay, but
> since tablesync worker commits other changes with replication origin
> I'm concerned a bit there might be a corner case.
>
AFAICS, we always apply the changes and commit/rollback those before
calling process_syncing_tables(). For example, see
apply_handle_commit(). So, I don't see how we can miss a corner case.
It seems to me that we shouldn't be in a transaction in
process_syncing_tables().
--
With Regards,
Amit Kapila.