On Monday, December 19, 2022 8:47 PMs Amit Kapila <amit.kapil...@gmail.com>: > > On Sat, Dec 17, 2022 at 7:34 PM houzj.f...@fujitsu.com > <houzj.f...@fujitsu.com> wrote: > > > > Agreed. I have addressed all the comments and did some cosmetic changes. > > Attach the new version patch set. > > > > Few comments: > ============ > 1. > + if (fileset_state == FS_SERIALIZE_IN_PROGRESS) { > + pa_lock_stream(MyParallelShared->xid, AccessShareLock); > + pa_unlock_stream(MyParallelShared->xid, AccessShareLock); } > + > + /* > + * We cannot read the file immediately after the leader has serialized > + all > + * changes to the file because there may still be messages in the > + memory > + * queue. We will apply all spooled messages the next time we call this > + * function, which should ensure that there are no messages left in the > + * memory queue. > + */ > + else if (fileset_state == FS_SERIALIZE_DONE) { > > Once we have waited in the FS_SERIALIZE_IN_PROGRESS, the file state can be > FS_SERIALIZE_DONE immediately after that. So, won't it be better to have a > separate if block for FS_SERIALIZE_DONE state? If you agree to do so then we > can probably remove the comment: "* XXX It is possible that immediately after > we have waited for a lock in ...".
Changed and slightly adjust the comments. > 2. > +void > +pa_decr_and_wait_stream_block(void) > +{ > + Assert(am_parallel_apply_worker()); > + > + if (pg_atomic_sub_fetch_u32(&MyParallelShared->pending_stream_count, > + 1) == 0) > > I think here the count can go negative when we are in serialize mode because > we don't increase it for serialize mode. I can't see any problem due to that > but > OTOH, this doesn't seem to be intended because in the future if we decide to > implement the functionality of switching back to non-serialize mode, this > could > be a problem. Also, I guess we don't even need to try locking/unlocking the > stream lock in that case. > One idea to avoid this is to check if the pending count is zero then if > file_set in > not available raise an error (elog ERROR), otherwise, simply return from here. Added the check. > > 3. In apply_handle_stream_stop(), we are setting backendstate as idle for > cases > TRANS_LEADER_SEND_TO_PARALLEL and TRANS_PARALLEL_APPLY. For other > cases, it is set by stream_stop_internal. I think it would be better to set > the state > explicitly for all cases to make the code look consistent and remove it from > stream_stop_internal(). The other reason to remove setting the state from > stream_stop_internal() is that when that function is invoked from other places > like apply_handle_stream_commit(), it seems to be setting the idle before > actually we reach the idle state. Changed. Besides, I notice that the pgstat_report_activity in pa_stream_abort for sub transaction is unnecessary since the state should be consistent with the state set at last stream_stop, so I have removed that as well. > > 4. Apart from the above, I have made a few changes in the comments, see > attached. Thanks, I have merged the patch. Best regards, Hou zj