Re: BUG #16643: PG13 - Logical replication - initial startup never finishes and gets stuck in startup loop

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.
* process_syncing_tables_for_apply belongs not in tablesync.c (maybe 
  worker.c?)
* DropSubscription bug / logicalrep_find_workers
  DropSubscription believes that it's valid to get a list of workers, then 
  kill them one by one. But this is racy since one worker could end for other 
  reasons and be replaced by a worker to another subscription.  This code 
  should use a loop that restarts after killing each worker.
* subscription_change_cb too coarse
  It seems that we should only reset "subscription valid" when *our* 
  subscription is changed, not other subscriptions.  Otherwise we read our own 
  sub data far too often.
* PostgresNode uses print where it should use Test::More's note()