public inbox for [email protected]  
help / color / mirror / Atom feed
From: Amit Kapila <[email protected]>
To: [email protected] <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Perform streaming logical transactions by background workers and parallel apply
Date: Wed, 15 Jun 2022 17:42:44 +0530
Message-ID: <CAA4eK1+GM6dmuFBnVGqiOs6UocN3c8mQZXD1Un=Sa2x_8vVfbw@mail.gmail.com> (raw)
In-Reply-To: <OS3PR01MB62756BD9482EB6BB1CA4CD4D9EAA9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
References: <CAA4eK1+wyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw@mail.gmail.com>
	<OS0PR01MB57160DFDEF5B1C4668346B7D94E99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<OS0PR01MB57166BC3CAA873364A2CC07A94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<CAA4eK1Kc0VAhJAQe4_5YGrnTpfKYA9=F6sxqEKPrrax5Hy0+jw@mail.gmail.com>
	<OS0PR01MB571616192F70A18AF88F3B2894F59@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<OS0PR01MB5716A6C18E69FB1EA9A27EED94F59@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<CAHut+PuqYP5eD5wcSCtk=a6KuMjat2UCzqyGoE7sieCaBsVskQ@mail.gmail.com>
	<OS0PR01MB57160D1B0D29DC66ED035F5694F89@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<OS0PR01MB5716E8D536552467EFB512EF94FC9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<OSZPR01MB6310F0FABB05F8E5BB31A5B0FDFC9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
	<CAHut+Pv3FV+WibRBuGCyXd_ri+O4L3iY+UnHO2SHRbmC+xR6dw@mail.gmail.com>
	<OSZPR01MB63106EADF50E0E710D36CE5DFDCA9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
	<CAHut+PuAxW57fowiMrn=3=53sagmehiTSW0o1Q52MpR3phUmyw@mail.gmail.com>
	<OS3PR01MB6275797F66EF0A47EEB2D8FC9EDD9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
	<CAA4eK1Jt08SYbRt_-rbSWNg=X9-m8+RdP5PosfnQgyF-z8bkxQ@mail.gmail.com>
	<CAA4eK1+Z6ahpTQK2KzkvQ1kN-urVS9-N_RDM11MS+btqaB8Bpw@mail.gmail.com>
	<CAD21AoC1ME-xQjdcTeMpVMADKJDjfGVZATdGNuR+Qcimov5TPA@mail.gmail.com>
	<CAA4eK1L=KsaU4fCpM_xbwK08E+NpYMviH23+uD0Be_2pKNtw_g@mail.gmail.com>
	<OS3PR01MB62758A881FF3240171B7B21B9EDE9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
	<OS3PR01MB6275208A2F8ED832710F65E09EA49@OS3PR01MB6275.jpnprd01.prod.outlook.com>
	<OS3PR01MB62756BD9482EB6BB1CA4CD4D9EAA9@OS3PR01MB6275.jpnprd01.prod.outlook.com>

On Tue, Jun 14, 2022 at 9:07 AM [email protected]
<[email protected]> wrote:
>
>
> Attach the new patches.
> Only changed patches 0001, 0004 and added new separate patch 0005.
>

Few questions/comments on 0001
===========================
1.
In the commit message, I see: "We also need to allow stream_stop to
complete by the apply background worker to avoid deadlocks because
T-1's current stream of changes can update rows in conflicting order
with T-2's next stream of changes."

Thinking about this, won't the T-1 and T-2 deadlock on the publisher
node as well if the above statement is true?

2.
+       <para>
+        The apply background workers are taken from the pool defined by
+        <varname>max_logical_replication_workers</varname>.
+       </para>
+       <para>
+        The default value is 3. This parameter can only be set in the
+        <filename>postgresql.conf</filename> file or on the server command
+        line.
+       </para>

Is there a reason to choose this number as 3? Why not 2 similar to
max_sync_workers_per_subscription?

3.
+
+  <para>
+   Setting streaming mode to <literal>apply</literal> could export invalid LSN
+   as finish LSN of failed transaction. Changing the streaming mode and making
+   the same conflict writes the finish LSN of the failed transaction in the
+   server log if required.
+  </para>

How will the user identify that this is an invalid LSN value and she
shouldn't use it to SKIP the transaction? Can we change the second
sentence to: "User should change the streaming mode to 'on' if they
would instead wish to see the finish LSN on error. Users can use
finish LSN to SKIP applying the transaction." I think we can give
reference to docs where the SKIP feature is explained.

4.
+ * This file contains routines that are intended to support setting up, using,
+ * and tearing down a ApplyBgworkerState.
+ * Refer to the comments in file header of logical/worker.c to see more
+ * informations about apply background worker.

Typo. /informations/information.

Consider having an empty line between the above two lines.

5.
+ApplyBgworkerState *
+apply_bgworker_find_or_start(TransactionId xid, bool start)
{
...
...
+ if (!TransactionIdIsValid(xid))
+ return NULL;
+
+ /*
+ * We don't start new background worker if we are not in streaming apply
+ * mode.
+ */
+ if (MySubscription->stream != SUBSTREAM_APPLY)
+ return NULL;
+
+ /*
+ * We don't start new background worker if user has set skiplsn as it's
+ * possible that user want to skip the streaming transaction. For
+ * streaming transaction, we need to spill the transaction to disk so that
+ * we can get the last LSN of the transaction to judge whether to skip
+ * before starting to apply the change.
+ */
+ if (start && !XLogRecPtrIsInvalid(MySubscription->skiplsn))
+ return NULL;
+
+ /*
+ * For streaming transactions that are being applied in apply background
+ * worker, we cannot decide whether to apply the change for a relation
+ * that is not in the READY state (see should_apply_changes_for_rel) as we
+ * won't know remote_final_lsn by that time. So, we don't start new apply
+ * background worker in this case.
+ */
+ if (start && !AllTablesyncsReady())
+ return NULL;
...
...
}

Can we move some of these starting checks to a separate function like
canstartapplybgworker()?

-- 
With Regards,
Amit Kapila.





view thread (633+ 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]
  Subject: Re: Perform streaming logical transactions by background workers and parallel apply
  In-Reply-To: <CAA4eK1+GM6dmuFBnVGqiOs6UocN3c8mQZXD1Un=Sa2x_8vVfbw@mail.gmail.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