Horton
Menu

Re: Column Filtering in Logical Replication

Masahiko Sawada <sawada.mshk@gmail.com>

View thread
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Justin Pryzby <pryzby@telsasoft.com>, Rahila Syed <rahilasyed90@gmail.com>, Peter Smith <smithpb2250@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>
Date: 2022-04-14T03:02:16Z
Lists: pgsql-hackers 
(

On Wed, Apr 13, 2022 at 6:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 13, 2022 at 1:41 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've looked at this issue and had the same analysis. Also, I could
> > reproduce this issue with the steps shared by Amit.
> >
> > As I mentioned in another thread[1], the fact that the tablesync
> > worker doesn't check the return value from
> > wait_for_worker_state_change() seems a bug to me. So my initial
> > thought of the solution is that we can have the tablesync worker check
> > the return value and exit if it's false. That way, the apply worker
> > can restart and request to launch the tablesync worker again. What do
> > you think?
> >
>
> I think that will fix this symptom but I am not sure if that would be
> the best way to deal with this because we have a mechanism where the
> sync worker can continue even if we don't do anything as a result of
> wait_for_worker_state_change() provided apply worker restarts.

I think we can think this is a separate issue. That is, if tablesync
worker can start streaming changes even without waiting for the apply
worker to set SUBREL_STATE_CATCHUP, do we really need the wait? I'm
not sure it's really safe. If it's safe, the tablesync worker will no
longer need to wait there.

>
> The other part of the puzzle is the below check in the code:
> /*
> * If we reached the sync worker limit per subscription, just exit
> * silently as we might get here because of an otherwise harmless race
> * condition.
> */
> if (nsyncworkers >= max_sync_workers_per_subscription)
>
> It is not clear to me why this check is there, if this wouldn't be
> there, the user would have got either a WARNING to increase the
> max_logical_replication_workers or the apply worker would have been
> restarted. Do you have any idea about this?

Yeah, I'm also puzzled with this check. It seems that this function
doesn't work well when the apply worker is not running and some
tablesync workers are running. I initially thought that the apply
worker calls to this function as many as tables that needs to be
synced, but it checks the max_sync_workers_per_subscription limit
before calling to logicalrep_worker_launch(). So I'm not really sure
we need this check.

>
> Yet another option is that we ensure that before launching sync
> workers (say in process_syncing_tables_for_apply->FetchTableStates,
> when we have to start a new transaction) we again call
> maybe_reread_subscription(), which should also fix this symptom. But
> again, I am not sure why it should be compulsory to call
> maybe_reread_subscription() in such a situation, there are no comments
> which suggest it,

Yes, it will fix this issue.

>
> Now, the reason why it appeared recently in commit c91f71b9dc is that
> I think we have increased the number of initial table syncs in that
> test, and probably increasing
> max_sync_workers_per_subscription/max_logical_replication_workers
> should fix that test.

I think so too.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: Explain about Column List feature.

  2. Doc: fix column list vs. replica identity rules.

  3. Prohibit combining publications with different column lists.

  4. Fix the check to limit sync workers.

  5. Wait for subscription to sync in t/031_column_list.sql

  6. Move prattrs to the pg_publication_rel section in docs

  7. Allow specifying column lists for logical replication

  8. Fix row filters with multiple publications

  9. Fix publish_as_relid with multiple publications

  10. Add some additional tests for row filters in logical replication.

  11. Add index on pg_publication_rel.prpubid

  12. Avoid using DefElemAction in AlterPublicationStmt

  13. Small cleanups related to PUBLICATION framework code

  14. Add PublicationTable and PublicationRelInfo structs

  15. Fix various concurrency issues in logical replication worker launching