On Thu, Oct 15, 2020 at 8:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2020-Oct-14, Petr Jelinek wrote:
>
> > It would be nice if the new sentences at the beginning of tablesync.c
> > started with uppercase, but that's about as nitpicky as I can be :)
>
> OK, fixed that :-) And pushed (to master only). There's one more
> change I added at the last minute, which is to remove the 'missing_ok'
> parameter of GetSubscriptionRelState. There are some other cosmetic
> changes, but nothing of substance.
>
> > > If I understand correcly, the early exit in tablesync.c is not saving *a
> > > lot* of time (we don't actually skip replaying any WAL), even if it's
> > > saving execution of a bunch of code. So I stand by my position that
> > > removing the code is better because it's clearer about what is actually
> > > happening.
> >
> > I don't really have any problems with the simplification you propose. The
> > saved time is probably in order of hundreds of ms which for table sync is
> > insignificant.
>
> Great, thanks.
>
> If you think this is done ... I have taken a few notes in the process:
>
> * STREAM COMMIT bug?
> In apply_handle_stream_commit, we do CommitTransactionCommand, but
> apparently in a tablesync worker we shouldn't do it.
>
In the tablesync stage, we don't allow streaming. See pgoutput_startup
where we disable streaming for the init phase. As far as I understand,
for tablesync we create the initial slot during which streaming will
be disabled then we will copy the table (here logical decoding won't
be used) and then allow the apply worker to get any other data which
is inserted in the meantime. Now, I might be missing something here
but if you can explain it a bit more or share some test to show how we
can reach here via tablesync worker then we can discuss the possible
solution.
Sorry for responding late.
--
With Regards,
Amit Kapila.
