MIME-Version: 1.0
References: 
 <CAA4eK1JQTDXTfvJ5d+L0ggG4+doyd0Xji=e0OJsb=qcn_jWALA@mail.gmail.com>
 <OS0PR01MB5716D6D2765E54DC739F288E940A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716997A7115715F9E4EE520940D9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JEFVZcymteMYXhYghiX7Lb=MKuEsfxgD1VfaRrLCHyzg@mail.gmail.com>
 <OS0PR01MB571663F65904D00895AD159994109@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JuRPUY2Dx=vUToRksTmW0ptqMVT3K32g0368f_ZCk-zg@mail.gmail.com>
 <OS0PR01MB5716B802A1733548A99761AE94129@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LGKYUDFZ_jFPrU497wQf2HNvt5a+tCTpqSeWSG6kfpSA@mail.gmail.com>
 <CAA4eK1Jwh7j86Egk1cye=x2R_yrTjzXGj7Fx12wVybBAEq91kA@mail.gmail.com>
 <OS0PR01MB571690D6FF24E9D000ECBDBD94199@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571639E4141EBF8A2C501E6994189@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB571639E4141EBF8A2C501E6994189@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 6 Dec 2022 13:20:24 +0530
Message-ID: 
 <CAA4eK1+ELh2vPre3JHyoeKV0A9_V7aXQD0QBPv86WEn7P_rK-g@mail.gmail.com>
Subject: Re: Perform streaming logical transactions by background workers and
 parallel apply
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
 Peter Smith <smithpb2250@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
 "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1%2BELh2vPre3JHyoeKV0A9_V7aXQD0QBPv86WEn7P_rK-g%40mail.gmail.com>
Precedence: bulk

On Mon, Dec 5, 2022 at 9:59 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Attach a new version patch set which fixed a testcase failure on CFbot.
>

Few comments:
============
1.
+ /*
+ * Break the loop if the parallel apply worker has finished applying
+ * the transaction. The parallel apply worker should have closed the
+ * file before committing.
+ */
+ if (am_parallel_apply_worker() &&
+ MyParallelShared->xact_state == PARALLEL_TRANS_FINISHED)
+ goto done;

This looks hackish to me because ideally, this API should exit after
reading and applying all the messages in the spool file. This check is
primarily based on the knowledge that once we reach some state, the
file won't have more data. I think it would be better to explicitly
ensure the same.

2.
+ /*
+ * No need to output the DEBUG message here in the parallel apply
+ * worker as similar messages will be output when handling STREAM_STOP
+ * message.
+ */
+ if (!am_parallel_apply_worker() && nchanges % 1000 == 0)
  elog(DEBUG1, "replayed %d changes from file \"%s\"",
  nchanges, path);
  }

I think this check appeared a bit ugly to me. I think it is okay to
get a similar DEBUG message at another place (on stream_stop) because
(a) this is logged every 1000 messages whereas stream_stop can be
after many more messages, so there doesn't appear to be a direct
correlation; (b) due to this, we can identify whether it is due to
spooled messages or due to direct apply; ideally we can use another
DEBUG message to differentiate but this doesn't appear bad to me.

3. The function names for serialize_stream_start(),
serialize_stream_stop(), and serialize_stream_abort() don't seem to
match the functionality they provide because none of these
write/serialize changes to the file. Can we rename these? Some
possible options could be stream_start_internal or stream_start_guts.

-- 
With Regards,
Amit Kapila.