On Thu, Aug 4, 2022 at 9:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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().
I checked the code again and agreed with that.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/