public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
Date: Sun, 27 Nov 2022 04:16:15 +0000
Message-ID: <OS0PR01MB571623B343E30E0219F4B86194109@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <TYAPR01MB5866051F824309A2B50737ECF50D9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
References: <CAA4eK1JQTDXTfvJ5d+L0ggG4+doyd0Xji=e0OJsb=qcn_jWALA@mail.gmail.com>
<OS0PR01MB5716D6D2765E54DC739F288E940A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<OS0PR01MB5716997A7115715F9E4EE520940D9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<TYAPR01MB5866051F824309A2B50737ECF50D9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
On Tuesday, November 22, 2022 9:53 PM Kuroda, Hayato <[email protected]> wroteL
>
> Thanks for updating the patch!
> I tested the case whether the deadlock caused by foreign key constraint could
> be detected, and it worked well.
>
> Followings are my review comments. They are basically related with 0001, but
> some contents may be not. It takes time to understand 0002 correctly...
Thanks for the comments!
> 01. typedefs.list
>
> LeaderFileSetState should be added to typedefs.list.
>
>
> 02. 032_streaming_parallel_apply.pl
>
> As I said in [1]: the test name may be not matched. Do you have reasons to
> revert the change?
The original parallel safety check has been removed, so I changed the name.
After rethinking about this, I named it to stream_parallel_conflict.
>
> 03. 032_streaming_parallel_apply.pl
>
> The test does not cover the case that the backend process relates with the
> deadlock. IIUC this is another motivation to use a stream/transaction lock.
> I think it should be added.
The main deadlock cases that stream/transaction lock can detect is 1) LA->PA 2)
LA->PA->PA as explained atop applyparallelworker.c. So I think backend process
related one is a variant which I think have been covered by the existing
tests in the patch.
> 04. log output
>
> While being applied spooled changes by PA, there are so many messages like
> "replayed %d changes from file..." and "applied %u changes...". They comes
> from
> apply_handle_stream_stop() and apply_spooled_messages(). They have same
> meaning, so I think one of them can be removed.
Changed.
> 05. system_views.sql
>
> In the previous version you modified pg_stat_subscription system view. Why
> do you revert that?
I was not sure should we include that in the main patch set.
I added a top-up patch that change the view.
> 06. interrupt.c - SignalHandlerForShutdownRequest()
>
> In the comment atop SignalHandlerForShutdownRequest(), some processes
> that assign the function except SIGTERM are clarified. We may be able to add
> the parallel apply worker.
Changed
> 08. guc_tables.c - ConfigureNamesInt
>
> ```
> &max_sync_workers_per_subscription,
> + 2, 0, MAX_PARALLEL_WORKER_LIMIT,
> + NULL, NULL, NULL
> + },
> ```
>
> The upper limit for max_sync_workers_per_subscription seems to be wrong, it
> should be used for max_parallel_apply_workers_per_subscription.
That's my miss, sorry for that.
> 10. worker.c - maybe_reread_subscription()
>
>
> ```
> + if (am_parallel_apply_worker())
> + ereport(LOG,
> + /* translator: first %s is the name of logical replication
> worker */
> + (errmsg("%s for subscription \"%s\"
> will stop because of a parameter change",
> +
> + get_worker_name(), MySubscription->name)));
> ```
>
> I was not sure get_worker_name() is needed. I think "logical replication apply
> worker"
> should be embedded.
Changed.
>
> 11. worker.c - ApplyWorkerMain()
>
> ```
> + (errmsg_internal("%s for subscription \"%s\"
> two_phase is %s",
> +
> + get_worker_name(),
> ```
Changed
Best regards,
Hou zj
view thread (625+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
In-Reply-To: <OS0PR01MB571623B343E30E0219F4B86194109@OS0PR01MB5716.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox