Horton
Menu

Re: Column Filtering in Logical Replication

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Petr Jelinek <petr.jelinek@enterprisedb.com>, 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-19T04:53:22Z
Lists: pgsql-hackers 
On Tue, Apr 19, 2022 at 6:58 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Apr 18, 2022 at 8:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Apr 14, 2022 at 9:09 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Thu, Apr 14, 2022 at 8:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > >
> > > I just hope that the original author Petr J. responds to this point. I
> > > have added him to this email. This will help us to find the best
> > > solution for this problem.
> > >
> >
> > I did some more investigation for this code. It is added by commit [1]
> > and the patch that led to this commit is first time posted on -hackers
> > in email [2]. Now, neither the commit message nor the patch (comments)
> > gives much idea as to why this part of code is added but I think there
> > is some hint in the email [2]. In particular, read the paragraph in
> > the email [2] that has the lines: ".... and limiting sync workers per
> > subscription theoretically wasn't either (although I don't think it
> > could happen in practice).".
> >
> > It seems that this check has been added to theoretically limit the
> > sync workers even though that can't happen because apply worker
> > ensures that before trying to launch the sync worker. Does this theory
> > make sense to me? If so, I think we can change the check as: "if
> > (OidIsValid(relid) && nsyncworkers >=
> > max_sync_workers_per_subscription)" in launcher.c. This will serve the
> > purpose of the original code and will solve the issue being discussed
> > here. I think we can even backpatch this. What do you think?
>
> +1. I also think it's a bug so back-patching makes sense to me.
>

Pushed. Thanks Tomas and Sawada-San.

-- 
With Regards,
Amit Kapila.

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