Обсуждение: Some problems of recovery conflict wait events
Hi all, When recovery conflicts happen on the streaming replication standby, the wait event of startup process is null when max_standby_streaming_delay = 0 (to be exact, when the limit time calculated by max_standby_streaming_delay is behind the last WAL data receipt time is behind). Moreover the process title of waiting startup process looks odd in the case of lock conflicts. 1. When max_standby_streaming_delay > 0 and the startup process conflicts with a lock, * wait event backend_type | wait_event_type | wait_event --------------+-----------------+------------ startup | Lock | relation (1 row) * ps 42513 ?? Ss 0:00.05 postgres: startup recovering 000000010000000000000003 waiting Looks good. 2. When max_standby_streaming_delay > 0 and the startup process conflicts with a snapshot, * wait event backend_type | wait_event_type | wait_event --------------+-----------------+------------ startup | | (1 row) * ps 44299 ?? Ss 0:00.05 postgres: startup recovering 000000010000000000000003 waiting wait_event_type and wait_event are null in spite of waiting for conflict resolution. 3. When max_standby_streaming_delay > 0 and the startup process conflicts with a lock, * wait event backend_type | wait_event_type | wait_event --------------+-----------------+------------ startup | | (1 row) * ps 46510 ?? Ss 0:00.05 postgres: startup recovering 000000010000000000000003 waiting waiting wait_event_type and wait_event are null and the process title is wrong; "waiting" appears twice. The cause of the first problem, wait_event_type and wait_event are not set, is that WaitExceedsMaxStandbyDelay which is called by ResolveRecoveryConflictWithVirtualXIDs waits for other transactions using pg_usleep rather than WaitLatch. I think we can change it so that it uses WaitLatch and those caller passes wait event information. For the second problem, wrong process title, the cause is also relevant with ResolveRecoveryConflictWithVirtualXIDs; in case of lock conflicts we add "waiting" to the process title in WaitOnLock but we add it again in ResolveRecoveryConflictWithVirtualXIDs. I think we can have WaitOnLock not set process title in recovery case. This problem exists on 12, 11 and 10. I'll submit the patch. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, 18 Feb 2020 at 17:58, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > Hi all, > > When recovery conflicts happen on the streaming replication standby, > the wait event of startup process is null when > max_standby_streaming_delay = 0 (to be exact, when the limit time > calculated by max_standby_streaming_delay is behind the last WAL data > receipt time is behind). Moreover the process title of waiting startup > process looks odd in the case of lock conflicts. > > 1. When max_standby_streaming_delay > 0 and the startup process > conflicts with a lock, > > * wait event > backend_type | wait_event_type | wait_event > --------------+-----------------+------------ > startup | Lock | relation > (1 row) > > * ps > 42513 ?? Ss 0:00.05 postgres: startup recovering > 000000010000000000000003 waiting > > Looks good. > > 2. When max_standby_streaming_delay > 0 and the startup process > conflicts with a snapshot, > > * wait event > backend_type | wait_event_type | wait_event > --------------+-----------------+------------ > startup | | > (1 row) > > * ps > 44299 ?? Ss 0:00.05 postgres: startup recovering > 000000010000000000000003 waiting > > wait_event_type and wait_event are null in spite of waiting for > conflict resolution. > > 3. When max_standby_streaming_delay > 0 and the startup process > conflicts with a lock, > > * wait event > backend_type | wait_event_type | wait_event > --------------+-----------------+------------ > startup | | > (1 row) > > * ps > 46510 ?? Ss 0:00.05 postgres: startup recovering > 000000010000000000000003 waiting waiting > > wait_event_type and wait_event are null and the process title is > wrong; "waiting" appears twice. > > The cause of the first problem, wait_event_type and wait_event are not > set, is that WaitExceedsMaxStandbyDelay which is called by > ResolveRecoveryConflictWithVirtualXIDs waits for other transactions > using pg_usleep rather than WaitLatch. I think we can change it so > that it uses WaitLatch and those caller passes wait event information. > > For the second problem, wrong process title, the cause is also > relevant with ResolveRecoveryConflictWithVirtualXIDs; in case of lock > conflicts we add "waiting" to the process title in WaitOnLock but we > add it again in ResolveRecoveryConflictWithVirtualXIDs. I think we can > have WaitOnLock not set process title in recovery case. > > This problem exists on 12, 11 and 10. I'll submit the patch. > I've attached patches that fix the above two issues. 0001 patch fixes the first problem. Currently there are 5 types of recovery conflict resolution: snapshot, tablespace, lock, database and buffer pin, and we set wait events to only 2 events out of 5: lock (only when doing ProcWaitForSignal) and buffer pin. Therefore, users cannot know that the startup process is waiting or not, and what waiting for. This patch sets wait events to more 3 events: snapshot, tablespace and lock. For wait events of those 3 events, I thought that we can create a new more appropriate wait event type, say RecoveryConflict, and set it for them. However, considering back-patching to existing versions, adding new wait event type would not be acceptable. So this patch sets existing wait events such as PG_WAIT_LOCK to those 3 places and doesn't not set a wait event for conflict resolution on dropping database because there is not an appropriate existing one. I'll start a separate thread about improvement on wait events of recovery conflict resolution for PG13 if necessary. 0002 patch fixes the second problem. With this patch, the process title is updated properly in all recovery conflict resolution cases. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Wed, 26 Feb 2020 at 16:19, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > On Tue, 18 Feb 2020 at 17:58, Masahiko Sawada > <masahiko.sawada@2ndquadrant.com> wrote: > > > > Hi all, > > > > When recovery conflicts happen on the streaming replication standby, > > the wait event of startup process is null when > > max_standby_streaming_delay = 0 (to be exact, when the limit time > > calculated by max_standby_streaming_delay is behind the last WAL data > > receipt time is behind). Moreover the process title of waiting startup > > process looks odd in the case of lock conflicts. > > > > 1. When max_standby_streaming_delay > 0 and the startup process > > conflicts with a lock, > > > > * wait event > > backend_type | wait_event_type | wait_event > > --------------+-----------------+------------ > > startup | Lock | relation > > (1 row) > > > > * ps > > 42513 ?? Ss 0:00.05 postgres: startup recovering > > 000000010000000000000003 waiting > > > > Looks good. > > > > 2. When max_standby_streaming_delay > 0 and the startup process > > conflicts with a snapshot, > > > > * wait event > > backend_type | wait_event_type | wait_event > > --------------+-----------------+------------ > > startup | | > > (1 row) > > > > * ps > > 44299 ?? Ss 0:00.05 postgres: startup recovering > > 000000010000000000000003 waiting > > > > wait_event_type and wait_event are null in spite of waiting for > > conflict resolution. > > > > 3. When max_standby_streaming_delay > 0 and the startup process > > conflicts with a lock, > > > > * wait event > > backend_type | wait_event_type | wait_event > > --------------+-----------------+------------ > > startup | | > > (1 row) > > > > * ps > > 46510 ?? Ss 0:00.05 postgres: startup recovering > > 000000010000000000000003 waiting waiting > > > > wait_event_type and wait_event are null and the process title is > > wrong; "waiting" appears twice. > > > > The cause of the first problem, wait_event_type and wait_event are not > > set, is that WaitExceedsMaxStandbyDelay which is called by > > ResolveRecoveryConflictWithVirtualXIDs waits for other transactions > > using pg_usleep rather than WaitLatch. I think we can change it so > > that it uses WaitLatch and those caller passes wait event information. > > > > For the second problem, wrong process title, the cause is also > > relevant with ResolveRecoveryConflictWithVirtualXIDs; in case of lock > > conflicts we add "waiting" to the process title in WaitOnLock but we > > add it again in ResolveRecoveryConflictWithVirtualXIDs. I think we can > > have WaitOnLock not set process title in recovery case. > > > > This problem exists on 12, 11 and 10. I'll submit the patch. > > > > I've attached patches that fix the above two issues. > > 0001 patch fixes the first problem. Currently there are 5 types of > recovery conflict resolution: snapshot, tablespace, lock, database and > buffer pin, and we set wait events to only 2 events out of 5: lock > (only when doing ProcWaitForSignal) and buffer pin. Therefore, users > cannot know that the startup process is waiting or not, and what > waiting for. This patch sets wait events to more 3 events: snapshot, > tablespace and lock. For wait events of those 3 events, I thought that > we can create a new more appropriate wait event type, say > RecoveryConflict, and set it for them. However, considering > back-patching to existing versions, adding new wait event type would > not be acceptable. So this patch sets existing wait events such as > PG_WAIT_LOCK to those 3 places and doesn't not set a wait event for > conflict resolution on dropping database because there is not an > appropriate existing one. I'll start a separate thread about > improvement on wait events of recovery conflict resolution for PG13 if > necessary. Attached a patch improves wait events of recovery conflict resolution. It's for PG13. I added new RecoveryConflict wait_event_type and some wait_event. This patch can be applied on top of two patches I already proposed. Regards, [1] https://www.postgresql.org/message-id/CA%2Bfd4k63ukOtdNx2f-fUZ2vuB3RgE%3DPo%2BxSnpmcPJbKqsJMtiA%40mail.gmail.com -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/02/29 12:36, Masahiko Sawada wrote: > On Wed, 26 Feb 2020 at 16:19, Masahiko Sawada > <masahiko.sawada@2ndquadrant.com> wrote: >> >> On Tue, 18 Feb 2020 at 17:58, Masahiko Sawada >> <masahiko.sawada@2ndquadrant.com> wrote: >>> >>> Hi all, >>> >>> When recovery conflicts happen on the streaming replication standby, >>> the wait event of startup process is null when >>> max_standby_streaming_delay = 0 (to be exact, when the limit time >>> calculated by max_standby_streaming_delay is behind the last WAL data >>> receipt time is behind). Moreover the process title of waiting startup >>> process looks odd in the case of lock conflicts. >>> >>> 1. When max_standby_streaming_delay > 0 and the startup process >>> conflicts with a lock, >>> >>> * wait event >>> backend_type | wait_event_type | wait_event >>> --------------+-----------------+------------ >>> startup | Lock | relation >>> (1 row) >>> >>> * ps >>> 42513 ?? Ss 0:00.05 postgres: startup recovering >>> 000000010000000000000003 waiting >>> >>> Looks good. >>> >>> 2. When max_standby_streaming_delay > 0 and the startup process >>> conflicts with a snapshot, >>> >>> * wait event >>> backend_type | wait_event_type | wait_event >>> --------------+-----------------+------------ >>> startup | | >>> (1 row) >>> >>> * ps >>> 44299 ?? Ss 0:00.05 postgres: startup recovering >>> 000000010000000000000003 waiting >>> >>> wait_event_type and wait_event are null in spite of waiting for >>> conflict resolution. >>> >>> 3. When max_standby_streaming_delay > 0 and the startup process >>> conflicts with a lock, >>> >>> * wait event >>> backend_type | wait_event_type | wait_event >>> --------------+-----------------+------------ >>> startup | | >>> (1 row) >>> >>> * ps >>> 46510 ?? Ss 0:00.05 postgres: startup recovering >>> 000000010000000000000003 waiting waiting >>> >>> wait_event_type and wait_event are null and the process title is >>> wrong; "waiting" appears twice. >>> >>> The cause of the first problem, wait_event_type and wait_event are not >>> set, is that WaitExceedsMaxStandbyDelay which is called by >>> ResolveRecoveryConflictWithVirtualXIDs waits for other transactions >>> using pg_usleep rather than WaitLatch. I think we can change it so >>> that it uses WaitLatch and those caller passes wait event information. >>> >>> For the second problem, wrong process title, the cause is also >>> relevant with ResolveRecoveryConflictWithVirtualXIDs; in case of lock >>> conflicts we add "waiting" to the process title in WaitOnLock but we >>> add it again in ResolveRecoveryConflictWithVirtualXIDs. I think we can >>> have WaitOnLock not set process title in recovery case. >>> >>> This problem exists on 12, 11 and 10. I'll submit the patch. >>> >> >> I've attached patches that fix the above two issues. >> >> 0001 patch fixes the first problem. Currently there are 5 types of >> recovery conflict resolution: snapshot, tablespace, lock, database and >> buffer pin, and we set wait events to only 2 events out of 5: lock >> (only when doing ProcWaitForSignal) and buffer pin. +1 to add those new wait events in the master. But adding them sounds like new feature rather than bug fix. So ISTM that it's not be back-patchable... Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
On Wed, 4 Mar 2020 at 11:04, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/02/29 12:36, Masahiko Sawada wrote: > > On Wed, 26 Feb 2020 at 16:19, Masahiko Sawada > > <masahiko.sawada@2ndquadrant.com> wrote: > >> > >> On Tue, 18 Feb 2020 at 17:58, Masahiko Sawada > >> <masahiko.sawada@2ndquadrant.com> wrote: > >>> > >>> Hi all, > >>> > >>> When recovery conflicts happen on the streaming replication standby, > >>> the wait event of startup process is null when > >>> max_standby_streaming_delay = 0 (to be exact, when the limit time > >>> calculated by max_standby_streaming_delay is behind the last WAL data > >>> receipt time is behind). Moreover the process title of waiting startup > >>> process looks odd in the case of lock conflicts. > >>> > >>> 1. When max_standby_streaming_delay > 0 and the startup process > >>> conflicts with a lock, > >>> > >>> * wait event > >>> backend_type | wait_event_type | wait_event > >>> --------------+-----------------+------------ > >>> startup | Lock | relation > >>> (1 row) > >>> > >>> * ps > >>> 42513 ?? Ss 0:00.05 postgres: startup recovering > >>> 000000010000000000000003 waiting > >>> > >>> Looks good. > >>> > >>> 2. When max_standby_streaming_delay > 0 and the startup process > >>> conflicts with a snapshot, > >>> > >>> * wait event > >>> backend_type | wait_event_type | wait_event > >>> --------------+-----------------+------------ > >>> startup | | > >>> (1 row) > >>> > >>> * ps > >>> 44299 ?? Ss 0:00.05 postgres: startup recovering > >>> 000000010000000000000003 waiting > >>> > >>> wait_event_type and wait_event are null in spite of waiting for > >>> conflict resolution. > >>> > >>> 3. When max_standby_streaming_delay > 0 and the startup process > >>> conflicts with a lock, > >>> > >>> * wait event > >>> backend_type | wait_event_type | wait_event > >>> --------------+-----------------+------------ > >>> startup | | > >>> (1 row) > >>> > >>> * ps > >>> 46510 ?? Ss 0:00.05 postgres: startup recovering > >>> 000000010000000000000003 waiting waiting > >>> > >>> wait_event_type and wait_event are null and the process title is > >>> wrong; "waiting" appears twice. > >>> > >>> The cause of the first problem, wait_event_type and wait_event are not > >>> set, is that WaitExceedsMaxStandbyDelay which is called by > >>> ResolveRecoveryConflictWithVirtualXIDs waits for other transactions > >>> using pg_usleep rather than WaitLatch. I think we can change it so > >>> that it uses WaitLatch and those caller passes wait event information. > >>> > >>> For the second problem, wrong process title, the cause is also > >>> relevant with ResolveRecoveryConflictWithVirtualXIDs; in case of lock > >>> conflicts we add "waiting" to the process title in WaitOnLock but we > >>> add it again in ResolveRecoveryConflictWithVirtualXIDs. I think we can > >>> have WaitOnLock not set process title in recovery case. > >>> > >>> This problem exists on 12, 11 and 10. I'll submit the patch. > >>> > >> > >> I've attached patches that fix the above two issues. > >> > >> 0001 patch fixes the first problem. Currently there are 5 types of > >> recovery conflict resolution: snapshot, tablespace, lock, database and > >> buffer pin, and we set wait events to only 2 events out of 5: lock > >> (only when doing ProcWaitForSignal) and buffer pin. > > +1 to add those new wait events in the master. But adding them sounds like > new feature rather than bug fix. So ISTM that it's not be back-patchable... > Yeah, so 0001 patch sets existing wait events to recovery conflict resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) to the recovery conflict on a snapshot. 0003 patch improves these wait events by adding the new type of wait event such as WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch is the fix for existing versions and 0003 patch is an improvement for only PG13. Did you mean even 0001 patch doesn't fit for back-patching? Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: > Yeah, so 0001 patch sets existing wait events to recovery conflict > resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) > to the recovery conflict on a snapshot. 0003 patch improves these wait > events by adding the new type of wait event such as > WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch > is the fix for existing versions and 0003 patch is an improvement for > only PG13. Did you mean even 0001 patch doesn't fit for back-patching? I got my eyes on this patch set. The full patch set is in my opinion just a set of improvements, and not bug fixes, so I would refrain from back-backpatching. -- Michael
Вложения
On 2020/03/04 13:27, Michael Paquier wrote: > On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: >> Yeah, so 0001 patch sets existing wait events to recovery conflict >> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) >> to the recovery conflict on a snapshot. 0003 patch improves these wait >> events by adding the new type of wait event such as >> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch >> is the fix for existing versions and 0003 patch is an improvement for >> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? Yes, it looks like a improvement rather than bug fix. > I got my eyes on this patch set. The full patch set is in my opinion > just a set of improvements, and not bug fixes, so I would refrain from > back-backpatching. I think that the issue (i.e., "waiting" is reported twice needlessly in PS display) that 0002 patch tries to fix is a bug. So it should be fixed even in the back branches. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/03/04 13:27, Michael Paquier wrote: > > On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: > >> Yeah, so 0001 patch sets existing wait events to recovery conflict > >> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) > >> to the recovery conflict on a snapshot. 0003 patch improves these wait > >> events by adding the new type of wait event such as > >> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch > >> is the fix for existing versions and 0003 patch is an improvement for > >> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? > > Yes, it looks like a improvement rather than bug fix. > Okay, understand. > > I got my eyes on this patch set. The full patch set is in my opinion > > just a set of improvements, and not bug fixes, so I would refrain from > > back-backpatching. > > I think that the issue (i.e., "waiting" is reported twice needlessly > in PS display) that 0002 patch tries to fix is a bug. So it should be > fixed even in the back branches. So we need only two patches: one fixes process title issue and another improve wait event. I've attached updated patches. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/03/04 14:31, Masahiko Sawada wrote:
> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/04 13:27, Michael Paquier wrote:
>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>> events by adding the new type of wait event such as
>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>
>> Yes, it looks like a improvement rather than bug fix.
>>
>
> Okay, understand.
>
>>> I got my eyes on this patch set. The full patch set is in my opinion
>>> just a set of improvements, and not bug fixes, so I would refrain from
>>> back-backpatching.
>>
>> I think that the issue (i.e., "waiting" is reported twice needlessly
>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>> fixed even in the back branches.
>
> So we need only two patches: one fixes process title issue and another
> improve wait event. I've attached updated patches.
Thanks for updating the patches! I started reading 0001 patch.
- /*
- * Report via ps if we have been waiting for more than 500 msec
- * (should that be configurable?)
- */
- if (update_process_title && new_status == NULL &&
- TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
- 500))
The patch changes ResolveRecoveryConflictWithSnapshot() and
ResolveRecoveryConflictWithTablespace() so that they always add
"waiting" into the PS display, whether wait is really necessary or not.
But isn't it better to display "waiting" in PS basically when wait is
necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
does as the above?
ResolveRecoveryConflictWithDatabase(Oid dbid)
{
+ char *new_status = NULL;
+
+ /* Report via ps we are waiting */
+ new_status = set_process_title_waiting();
In ResolveRecoveryConflictWithDatabase(), there seems no need to
display "waiting" in PS because no wait occurs when recovery conflict
with database happens.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/04 14:31, Masahiko Sawada wrote:
> > On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/04 13:27, Michael Paquier wrote:
> >>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>> events by adding the new type of wait event such as
> >>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>
> >> Yes, it looks like a improvement rather than bug fix.
> >>
> >
> > Okay, understand.
> >
> >>> I got my eyes on this patch set. The full patch set is in my opinion
> >>> just a set of improvements, and not bug fixes, so I would refrain from
> >>> back-backpatching.
> >>
> >> I think that the issue (i.e., "waiting" is reported twice needlessly
> >> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >> fixed even in the back branches.
> >
> > So we need only two patches: one fixes process title issue and another
> > improve wait event. I've attached updated patches.
>
> Thanks for updating the patches! I started reading 0001 patch.
Thank you for reviewing this patch.
>
> - /*
> - * Report via ps if we have been waiting for more than 500 msec
> - * (should that be configurable?)
> - */
> - if (update_process_title && new_status == NULL &&
> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> - 500))
>
> The patch changes ResolveRecoveryConflictWithSnapshot() and
> ResolveRecoveryConflictWithTablespace() so that they always add
> "waiting" into the PS display, whether wait is really necessary or not.
> But isn't it better to display "waiting" in PS basically when wait is
> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> does as the above?
You're right. Will fix it.
>
> ResolveRecoveryConflictWithDatabase(Oid dbid)
> {
> + char *new_status = NULL;
> +
> + /* Report via ps we are waiting */
> + new_status = set_process_title_waiting();
>
> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> display "waiting" in PS because no wait occurs when recovery conflict
> with database happens.
Isn't the startup process waiting for other backend to terminate?
Regards
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020/03/05 16:58, Masahiko Sawada wrote:
> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>> events by adding the new type of wait event such as
>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>
>>>> Yes, it looks like a improvement rather than bug fix.
>>>>
>>>
>>> Okay, understand.
>>>
>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>> back-backpatching.
>>>>
>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>> fixed even in the back branches.
>>>
>>> So we need only two patches: one fixes process title issue and another
>>> improve wait event. I've attached updated patches.
>>
>> Thanks for updating the patches! I started reading 0001 patch.
>
> Thank you for reviewing this patch.
>
>>
>> - /*
>> - * Report via ps if we have been waiting for more than 500 msec
>> - * (should that be configurable?)
>> - */
>> - if (update_process_title && new_status == NULL &&
>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>> - 500))
>>
>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>> ResolveRecoveryConflictWithTablespace() so that they always add
>> "waiting" into the PS display, whether wait is really necessary or not.
>> But isn't it better to display "waiting" in PS basically when wait is
>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>> does as the above?
>
> You're right. Will fix it.
>
>>
>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>> {
>> + char *new_status = NULL;
>> +
>> + /* Report via ps we are waiting */
>> + new_status = set_process_title_waiting();
>>
>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>> display "waiting" in PS because no wait occurs when recovery conflict
>> with database happens.
>
> Isn't the startup process waiting for other backend to terminate?
Yeah, you're right. I agree that "waiting" should be reported in this case.
Currently ResolveRecoveryConflictWithLock() and
ResolveRecoveryConflictWithBufferPin() don't call
ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
in PS display. You changed them so that they report "waiting". I agree
to have this change. But this change is an improvement rather than
a bug fix, i.e., we should apply this change only in v13?
Of course, the other part in the patch, i.e., fixing the issue that
"waiting" is doubly reported, should be back-patched, I think,
though.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/05 16:58, Masahiko Sawada wrote:
> > On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2020/03/04 13:27, Michael Paquier wrote:
> >>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>>>> events by adding the new type of wait event such as
> >>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>>>
> >>>> Yes, it looks like a improvement rather than bug fix.
> >>>>
> >>>
> >>> Okay, understand.
> >>>
> >>>>> I got my eyes on this patch set. The full patch set is in my opinion
> >>>>> just a set of improvements, and not bug fixes, so I would refrain from
> >>>>> back-backpatching.
> >>>>
> >>>> I think that the issue (i.e., "waiting" is reported twice needlessly
> >>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >>>> fixed even in the back branches.
> >>>
> >>> So we need only two patches: one fixes process title issue and another
> >>> improve wait event. I've attached updated patches.
> >>
> >> Thanks for updating the patches! I started reading 0001 patch.
> >
> > Thank you for reviewing this patch.
> >
> >>
> >> - /*
> >> - * Report via ps if we have been waiting for more than 500 msec
> >> - * (should that be configurable?)
> >> - */
> >> - if (update_process_title && new_status == NULL &&
> >> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >> - 500))
> >>
> >> The patch changes ResolveRecoveryConflictWithSnapshot() and
> >> ResolveRecoveryConflictWithTablespace() so that they always add
> >> "waiting" into the PS display, whether wait is really necessary or not.
> >> But isn't it better to display "waiting" in PS basically when wait is
> >> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> >> does as the above?
> >
> > You're right. Will fix it.
> >
> >>
> >> ResolveRecoveryConflictWithDatabase(Oid dbid)
> >> {
> >> + char *new_status = NULL;
> >> +
> >> + /* Report via ps we are waiting */
> >> + new_status = set_process_title_waiting();
> >>
> >> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> >> display "waiting" in PS because no wait occurs when recovery conflict
> >> with database happens.
> >
> > Isn't the startup process waiting for other backend to terminate?
>
> Yeah, you're right. I agree that "waiting" should be reported in this case.
>
> Currently ResolveRecoveryConflictWithLock() and
> ResolveRecoveryConflictWithBufferPin() don't call
> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
> in PS display. You changed them so that they report "waiting". I agree
> to have this change. But this change is an improvement rather than
> a bug fix, i.e., we should apply this change only in v13?
>
Did you mean ResolveRecoveryConflictWithDatabase and
ResolveRecoveryConflictWithBufferPin? In the current code as far as I
researched there are two cases where we don't add "waiting" and one
case where we doubly add "waiting".
ResolveRecoveryConflictWithDatabase and
ResolveRecoveryConflictWithBufferPin don't update the ps title.
Although the path where GetCurrentTimestamp() >= ltime is false in
ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
already updated in WaitOnLock. On the other hand, the path where
GetCurrentTimestamp() >= ltime is true in
ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
I reported.
I've split the patch into two patches: 0001 patch fixes the issue
about doubly updating ps title, 0002 patch makes the recovery conflict
resolution on database and buffer pin update the ps title.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/03/08 13:52, Masahiko Sawada wrote:
> On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/05 16:58, Masahiko Sawada wrote:
>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>>>> events by adding the new type of wait event such as
>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>>>
>>>>>> Yes, it looks like a improvement rather than bug fix.
>>>>>>
>>>>>
>>>>> Okay, understand.
>>>>>
>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>>>> back-backpatching.
>>>>>>
>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>>>> fixed even in the back branches.
>>>>>
>>>>> So we need only two patches: one fixes process title issue and another
>>>>> improve wait event. I've attached updated patches.
>>>>
>>>> Thanks for updating the patches! I started reading 0001 patch.
>>>
>>> Thank you for reviewing this patch.
>>>
>>>>
>>>> - /*
>>>> - * Report via ps if we have been waiting for more than 500 msec
>>>> - * (should that be configurable?)
>>>> - */
>>>> - if (update_process_title && new_status == NULL &&
>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>>> - 500))
>>>>
>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>>>> ResolveRecoveryConflictWithTablespace() so that they always add
>>>> "waiting" into the PS display, whether wait is really necessary or not.
>>>> But isn't it better to display "waiting" in PS basically when wait is
>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>>>> does as the above?
>>>
>>> You're right. Will fix it.
>>>
>>>>
>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>>>> {
>>>> + char *new_status = NULL;
>>>> +
>>>> + /* Report via ps we are waiting */
>>>> + new_status = set_process_title_waiting();
>>>>
>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>>>> display "waiting" in PS because no wait occurs when recovery conflict
>>>> with database happens.
>>>
>>> Isn't the startup process waiting for other backend to terminate?
>>
>> Yeah, you're right. I agree that "waiting" should be reported in this case.
>>
>> Currently ResolveRecoveryConflictWithLock() and
>> ResolveRecoveryConflictWithBufferPin() don't call
>> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
>> in PS display. You changed them so that they report "waiting". I agree
>> to have this change. But this change is an improvement rather than
>> a bug fix, i.e., we should apply this change only in v13?
>>
>
> Did you mean ResolveRecoveryConflictWithDatabase and
> ResolveRecoveryConflictWithBufferPin?
Yes! Sorry for my typo.
> In the current code as far as I
> researched there are two cases where we don't add "waiting" and one
> case where we doubly add "waiting".
>
> ResolveRecoveryConflictWithDatabase and
> ResolveRecoveryConflictWithBufferPin don't update the ps title.
> Although the path where GetCurrentTimestamp() >= ltime is false in
> ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
> already updated in WaitOnLock. On the other hand, the path where
> GetCurrentTimestamp() >= ltime is true in
> ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
> I reported.
>
> I've split the patch into two patches: 0001 patch fixes the issue
> about doubly updating ps title, 0002 patch makes the recovery conflict
> resolution on database and buffer pin update the ps title.
Thanks for splitting the patches. I think that 0001 patch can be back-patched.
- /*
- * Report via ps if we have been waiting for more than 500 msec
- * (should that be configurable?)
- */
- if (update_process_title && new_status == NULL &&
- TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
- 500))
Originally, "waiting" is reported in PS if we've been waiting for more than
500 msec, as the above does. But you got rid of those codes in the patch.
Did you confirm that it's safe to do that? If not, isn't it better to apply
the attached patch? The attached patch makes
ResolveRecoveryConflictWithVirtualXIDs() report "waiting" as it does now,
and allows its caller to choose whether to report that.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Вложения
On Mon, 9 Mar 2020 at 13:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/08 13:52, Masahiko Sawada wrote:
> > On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/05 16:58, Masahiko Sawada wrote:
> >>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
> >>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>>>>>> events by adding the new type of wait event such as
> >>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>>>>>
> >>>>>> Yes, it looks like a improvement rather than bug fix.
> >>>>>>
> >>>>>
> >>>>> Okay, understand.
> >>>>>
> >>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
> >>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
> >>>>>>> back-backpatching.
> >>>>>>
> >>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
> >>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >>>>>> fixed even in the back branches.
> >>>>>
> >>>>> So we need only two patches: one fixes process title issue and another
> >>>>> improve wait event. I've attached updated patches.
> >>>>
> >>>> Thanks for updating the patches! I started reading 0001 patch.
> >>>
> >>> Thank you for reviewing this patch.
> >>>
> >>>>
> >>>> - /*
> >>>> - * Report via ps if we have been waiting for more than 500 msec
> >>>> - * (should that be configurable?)
> >>>> - */
> >>>> - if (update_process_title && new_status == NULL &&
> >>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >>>> - 500))
> >>>>
> >>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
> >>>> ResolveRecoveryConflictWithTablespace() so that they always add
> >>>> "waiting" into the PS display, whether wait is really necessary or not.
> >>>> But isn't it better to display "waiting" in PS basically when wait is
> >>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> >>>> does as the above?
> >>>
> >>> You're right. Will fix it.
> >>>
> >>>>
> >>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
> >>>> {
> >>>> + char *new_status = NULL;
> >>>> +
> >>>> + /* Report via ps we are waiting */
> >>>> + new_status = set_process_title_waiting();
> >>>>
> >>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> >>>> display "waiting" in PS because no wait occurs when recovery conflict
> >>>> with database happens.
> >>>
> >>> Isn't the startup process waiting for other backend to terminate?
> >>
> >> Yeah, you're right. I agree that "waiting" should be reported in this case.
> >>
> >> Currently ResolveRecoveryConflictWithLock() and
> >> ResolveRecoveryConflictWithBufferPin() don't call
> >> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
> >> in PS display. You changed them so that they report "waiting". I agree
> >> to have this change. But this change is an improvement rather than
> >> a bug fix, i.e., we should apply this change only in v13?
> >>
> >
> > Did you mean ResolveRecoveryConflictWithDatabase and
> > ResolveRecoveryConflictWithBufferPin?
>
> Yes! Sorry for my typo.
>
> > In the current code as far as I
> > researched there are two cases where we don't add "waiting" and one
> > case where we doubly add "waiting".
> >
> > ResolveRecoveryConflictWithDatabase and
> > ResolveRecoveryConflictWithBufferPin don't update the ps title.
> > Although the path where GetCurrentTimestamp() >= ltime is false in
> > ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
> > already updated in WaitOnLock. On the other hand, the path where
> > GetCurrentTimestamp() >= ltime is true in
> > ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
> > I reported.
> >
> > I've split the patch into two patches: 0001 patch fixes the issue
> > about doubly updating ps title, 0002 patch makes the recovery conflict
> > resolution on database and buffer pin update the ps title.
>
> Thanks for splitting the patches. I think that 0001 patch can be back-patched.
>
> - /*
> - * Report via ps if we have been waiting for more than 500 msec
> - * (should that be configurable?)
> - */
> - if (update_process_title && new_status == NULL &&
> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> - 500))
>
> Originally, "waiting" is reported in PS if we've been waiting for more than
> 500 msec, as the above does. But you got rid of those codes in the patch.
> Did you confirm that it's safe to do that? If not, isn't it better to apply
> the attached patch?
In WaitOnLock() we update the ps title regardless of waiting time. So
I thought we can change it to make these behavior consistent. But
considering back-patch, your patch looks better than mine.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020/03/09 14:10, Masahiko Sawada wrote:
> On Mon, 9 Mar 2020 at 13:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/08 13:52, Masahiko Sawada wrote:
>>> On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
>>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>>>>>> events by adding the new type of wait event such as
>>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>>>>>
>>>>>>>> Yes, it looks like a improvement rather than bug fix.
>>>>>>>>
>>>>>>>
>>>>>>> Okay, understand.
>>>>>>>
>>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>>>>>> back-backpatching.
>>>>>>>>
>>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>>>>>> fixed even in the back branches.
>>>>>>>
>>>>>>> So we need only two patches: one fixes process title issue and another
>>>>>>> improve wait event. I've attached updated patches.
>>>>>>
>>>>>> Thanks for updating the patches! I started reading 0001 patch.
>>>>>
>>>>> Thank you for reviewing this patch.
>>>>>
>>>>>>
>>>>>> - /*
>>>>>> - * Report via ps if we have been waiting for more than 500 msec
>>>>>> - * (should that be configurable?)
>>>>>> - */
>>>>>> - if (update_process_title && new_status == NULL &&
>>>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>>>>> - 500))
>>>>>>
>>>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>>>>>> ResolveRecoveryConflictWithTablespace() so that they always add
>>>>>> "waiting" into the PS display, whether wait is really necessary or not.
>>>>>> But isn't it better to display "waiting" in PS basically when wait is
>>>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>>>>>> does as the above?
>>>>>
>>>>> You're right. Will fix it.
>>>>>
>>>>>>
>>>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>>>>>> {
>>>>>> + char *new_status = NULL;
>>>>>> +
>>>>>> + /* Report via ps we are waiting */
>>>>>> + new_status = set_process_title_waiting();
>>>>>>
>>>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>>>>>> display "waiting" in PS because no wait occurs when recovery conflict
>>>>>> with database happens.
>>>>>
>>>>> Isn't the startup process waiting for other backend to terminate?
>>>>
>>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
>>>>
>>>> Currently ResolveRecoveryConflictWithLock() and
>>>> ResolveRecoveryConflictWithBufferPin() don't call
>>>> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
>>>> in PS display. You changed them so that they report "waiting". I agree
>>>> to have this change. But this change is an improvement rather than
>>>> a bug fix, i.e., we should apply this change only in v13?
>>>>
>>>
>>> Did you mean ResolveRecoveryConflictWithDatabase and
>>> ResolveRecoveryConflictWithBufferPin?
>>
>> Yes! Sorry for my typo.
>>
>>> In the current code as far as I
>>> researched there are two cases where we don't add "waiting" and one
>>> case where we doubly add "waiting".
>>>
>>> ResolveRecoveryConflictWithDatabase and
>>> ResolveRecoveryConflictWithBufferPin don't update the ps title.
>>> Although the path where GetCurrentTimestamp() >= ltime is false in
>>> ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
>>> already updated in WaitOnLock. On the other hand, the path where
>>> GetCurrentTimestamp() >= ltime is true in
>>> ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
>>> I reported.
>>>
>>> I've split the patch into two patches: 0001 patch fixes the issue
>>> about doubly updating ps title, 0002 patch makes the recovery conflict
>>> resolution on database and buffer pin update the ps title.
>>
>> Thanks for splitting the patches. I think that 0001 patch can be back-patched.
>>
>> - /*
>> - * Report via ps if we have been waiting for more than 500 msec
>> - * (should that be configurable?)
>> - */
>> - if (update_process_title && new_status == NULL &&
>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>> - 500))
>>
>> Originally, "waiting" is reported in PS if we've been waiting for more than
>> 500 msec, as the above does. But you got rid of those codes in the patch.
>> Did you confirm that it's safe to do that? If not, isn't it better to apply
>> the attached patch?
>
> In WaitOnLock() we update the ps title regardless of waiting time. So
> I thought we can change it to make these behavior consistent. But
> considering back-patch, your patch looks better than mine.
Yeah, so I pushed the 0001 patch at first!
I will review the other patches later.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On Tue, 10 Mar 2020 at 00:57, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/09 14:10, Masahiko Sawada wrote:
> > On Mon, 9 Mar 2020 at 13:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/08 13:52, Masahiko Sawada wrote:
> >>> On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
> >>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
> >>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>>>>>>>> events by adding the new type of wait event such as
> >>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>>>>>>>
> >>>>>>>> Yes, it looks like a improvement rather than bug fix.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Okay, understand.
> >>>>>>>
> >>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
> >>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
> >>>>>>>>> back-backpatching.
> >>>>>>>>
> >>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
> >>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >>>>>>>> fixed even in the back branches.
> >>>>>>>
> >>>>>>> So we need only two patches: one fixes process title issue and another
> >>>>>>> improve wait event. I've attached updated patches.
> >>>>>>
> >>>>>> Thanks for updating the patches! I started reading 0001 patch.
> >>>>>
> >>>>> Thank you for reviewing this patch.
> >>>>>
> >>>>>>
> >>>>>> - /*
> >>>>>> - * Report via ps if we have been waiting for more than 500 msec
> >>>>>> - * (should that be configurable?)
> >>>>>> - */
> >>>>>> - if (update_process_title && new_status == NULL &&
> >>>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >>>>>> - 500))
> >>>>>>
> >>>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
> >>>>>> ResolveRecoveryConflictWithTablespace() so that they always add
> >>>>>> "waiting" into the PS display, whether wait is really necessary or not.
> >>>>>> But isn't it better to display "waiting" in PS basically when wait is
> >>>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> >>>>>> does as the above?
> >>>>>
> >>>>> You're right. Will fix it.
> >>>>>
> >>>>>>
> >>>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
> >>>>>> {
> >>>>>> + char *new_status = NULL;
> >>>>>> +
> >>>>>> + /* Report via ps we are waiting */
> >>>>>> + new_status = set_process_title_waiting();
> >>>>>>
> >>>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> >>>>>> display "waiting" in PS because no wait occurs when recovery conflict
> >>>>>> with database happens.
> >>>>>
> >>>>> Isn't the startup process waiting for other backend to terminate?
> >>>>
> >>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
> >>>>
> >>>> Currently ResolveRecoveryConflictWithLock() and
> >>>> ResolveRecoveryConflictWithBufferPin() don't call
> >>>> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
> >>>> in PS display. You changed them so that they report "waiting". I agree
> >>>> to have this change. But this change is an improvement rather than
> >>>> a bug fix, i.e., we should apply this change only in v13?
> >>>>
> >>>
> >>> Did you mean ResolveRecoveryConflictWithDatabase and
> >>> ResolveRecoveryConflictWithBufferPin?
> >>
> >> Yes! Sorry for my typo.
> >>
> >>> In the current code as far as I
> >>> researched there are two cases where we don't add "waiting" and one
> >>> case where we doubly add "waiting".
> >>>
> >>> ResolveRecoveryConflictWithDatabase and
> >>> ResolveRecoveryConflictWithBufferPin don't update the ps title.
> >>> Although the path where GetCurrentTimestamp() >= ltime is false in
> >>> ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
> >>> already updated in WaitOnLock. On the other hand, the path where
> >>> GetCurrentTimestamp() >= ltime is true in
> >>> ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
> >>> I reported.
> >>>
> >>> I've split the patch into two patches: 0001 patch fixes the issue
> >>> about doubly updating ps title, 0002 patch makes the recovery conflict
> >>> resolution on database and buffer pin update the ps title.
> >>
> >> Thanks for splitting the patches. I think that 0001 patch can be back-patched.
> >>
> >> - /*
> >> - * Report via ps if we have been waiting for more than 500 msec
> >> - * (should that be configurable?)
> >> - */
> >> - if (update_process_title && new_status == NULL &&
> >> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >> - 500))
> >>
> >> Originally, "waiting" is reported in PS if we've been waiting for more than
> >> 500 msec, as the above does. But you got rid of those codes in the patch.
> >> Did you confirm that it's safe to do that? If not, isn't it better to apply
> >> the attached patch?
> >
> > In WaitOnLock() we update the ps title regardless of waiting time. So
> > I thought we can change it to make these behavior consistent. But
> > considering back-patch, your patch looks better than mine.
>
> Yeah, so I pushed the 0001 patch at first!
> I will review the other patches later.
Thank you!
For 0002 patch which makes ResolveRecoveryConflictWithDatabase and
ResolveRecoveryConflictWithBufferPin update the ps title, I think
these are better to wait for 5ms before updating the ps title like
ResolveRecoveryConflictWithVirtualXIDs, for consistency among recovery
conflict resolution functions, but what do you think?
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020/03/10 13:54, Masahiko Sawada wrote:
> On Tue, 10 Mar 2020 at 00:57, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/09 14:10, Masahiko Sawada wrote:
>>> On Mon, 9 Mar 2020 at 13:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2020/03/08 13:52, Masahiko Sawada wrote:
>>>>> On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
>>>>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>>>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>>>>>>>> events by adding the new type of wait event such as
>>>>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>>>>>>>
>>>>>>>>>> Yes, it looks like a improvement rather than bug fix.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Okay, understand.
>>>>>>>>>
>>>>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>>>>>>>> back-backpatching.
>>>>>>>>>>
>>>>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>>>>>>>> fixed even in the back branches.
>>>>>>>>>
>>>>>>>>> So we need only two patches: one fixes process title issue and another
>>>>>>>>> improve wait event. I've attached updated patches.
>>>>>>>>
>>>>>>>> Thanks for updating the patches! I started reading 0001 patch.
>>>>>>>
>>>>>>> Thank you for reviewing this patch.
>>>>>>>
>>>>>>>>
>>>>>>>> - /*
>>>>>>>> - * Report via ps if we have been waiting for more than 500 msec
>>>>>>>> - * (should that be configurable?)
>>>>>>>> - */
>>>>>>>> - if (update_process_title && new_status == NULL &&
>>>>>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>>>>>>> - 500))
>>>>>>>>
>>>>>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>>>>>>>> ResolveRecoveryConflictWithTablespace() so that they always add
>>>>>>>> "waiting" into the PS display, whether wait is really necessary or not.
>>>>>>>> But isn't it better to display "waiting" in PS basically when wait is
>>>>>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>>>>>>>> does as the above?
>>>>>>>
>>>>>>> You're right. Will fix it.
>>>>>>>
>>>>>>>>
>>>>>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>>>>>>>> {
>>>>>>>> + char *new_status = NULL;
>>>>>>>> +
>>>>>>>> + /* Report via ps we are waiting */
>>>>>>>> + new_status = set_process_title_waiting();
>>>>>>>>
>>>>>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>>>>>>>> display "waiting" in PS because no wait occurs when recovery conflict
>>>>>>>> with database happens.
>>>>>>>
>>>>>>> Isn't the startup process waiting for other backend to terminate?
>>>>>>
>>>>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
>>>>>>
>>>>>> Currently ResolveRecoveryConflictWithLock() and
>>>>>> ResolveRecoveryConflictWithBufferPin() don't call
>>>>>> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
>>>>>> in PS display. You changed them so that they report "waiting". I agree
>>>>>> to have this change. But this change is an improvement rather than
>>>>>> a bug fix, i.e., we should apply this change only in v13?
>>>>>>
>>>>>
>>>>> Did you mean ResolveRecoveryConflictWithDatabase and
>>>>> ResolveRecoveryConflictWithBufferPin?
>>>>
>>>> Yes! Sorry for my typo.
>>>>
>>>>> In the current code as far as I
>>>>> researched there are two cases where we don't add "waiting" and one
>>>>> case where we doubly add "waiting".
>>>>>
>>>>> ResolveRecoveryConflictWithDatabase and
>>>>> ResolveRecoveryConflictWithBufferPin don't update the ps title.
>>>>> Although the path where GetCurrentTimestamp() >= ltime is false in
>>>>> ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
>>>>> already updated in WaitOnLock. On the other hand, the path where
>>>>> GetCurrentTimestamp() >= ltime is true in
>>>>> ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
>>>>> I reported.
>>>>>
>>>>> I've split the patch into two patches: 0001 patch fixes the issue
>>>>> about doubly updating ps title, 0002 patch makes the recovery conflict
>>>>> resolution on database and buffer pin update the ps title.
>>>>
>>>> Thanks for splitting the patches. I think that 0001 patch can be back-patched.
>>>>
>>>> - /*
>>>> - * Report via ps if we have been waiting for more than 500 msec
>>>> - * (should that be configurable?)
>>>> - */
>>>> - if (update_process_title && new_status == NULL &&
>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>>> - 500))
>>>>
>>>> Originally, "waiting" is reported in PS if we've been waiting for more than
>>>> 500 msec, as the above does. But you got rid of those codes in the patch.
>>>> Did you confirm that it's safe to do that? If not, isn't it better to apply
>>>> the attached patch?
>>>
>>> In WaitOnLock() we update the ps title regardless of waiting time. So
>>> I thought we can change it to make these behavior consistent. But
>>> considering back-patch, your patch looks better than mine.
>>
>> Yeah, so I pushed the 0001 patch at first!
>> I will review the other patches later.
>
> Thank you!
>
> For 0002 patch which makes ResolveRecoveryConflictWithDatabase and
> ResolveRecoveryConflictWithBufferPin update the ps title, I think
> these are better to wait for 5ms before updating the ps title like
> ResolveRecoveryConflictWithVirtualXIDs, for consistency among recovery
> conflict resolution functions, but what do you think?
Maybe yes.
As another idea, for consistency, we can change all
ResolveRecoveryConflictWithXXX() so that they don't wait
at all before reporting "waiting". But if we don't do that,
"waiting" can be reported even when we can immediately
cancel or terminate the conflicting transactions (e.g., in
case of max_standby_streaming_delay=0). To avoid this
issue, I think it's better to wait for 500ms.
The 0002 patch changes ResolveRecoveryConflictWithBufferPin()
so that it updates PS every time. But this seems not good
because the update can happen very frequently. Thought?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On Wed, 11 Mar 2020 at 16:42, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/10 13:54, Masahiko Sawada wrote:
> > On Tue, 10 Mar 2020 at 00:57, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/09 14:10, Masahiko Sawada wrote:
> >>> On Mon, 9 Mar 2020 at 13:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2020/03/08 13:52, Masahiko Sawada wrote:
> >>>>> On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
> >>>>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>>>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
> >>>>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>>>>>>>>>> events by adding the new type of wait event such as
> >>>>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>>>>>>>>>
> >>>>>>>>>> Yes, it looks like a improvement rather than bug fix.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Okay, understand.
> >>>>>>>>>
> >>>>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
> >>>>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
> >>>>>>>>>>> back-backpatching.
> >>>>>>>>>>
> >>>>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
> >>>>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >>>>>>>>>> fixed even in the back branches.
> >>>>>>>>>
> >>>>>>>>> So we need only two patches: one fixes process title issue and another
> >>>>>>>>> improve wait event. I've attached updated patches.
> >>>>>>>>
> >>>>>>>> Thanks for updating the patches! I started reading 0001 patch.
> >>>>>>>
> >>>>>>> Thank you for reviewing this patch.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> - /*
> >>>>>>>> - * Report via ps if we have been waiting for more than 500 msec
> >>>>>>>> - * (should that be configurable?)
> >>>>>>>> - */
> >>>>>>>> - if (update_process_title && new_status == NULL &&
> >>>>>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >>>>>>>> - 500))
> >>>>>>>>
> >>>>>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
> >>>>>>>> ResolveRecoveryConflictWithTablespace() so that they always add
> >>>>>>>> "waiting" into the PS display, whether wait is really necessary or not.
> >>>>>>>> But isn't it better to display "waiting" in PS basically when wait is
> >>>>>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> >>>>>>>> does as the above?
> >>>>>>>
> >>>>>>> You're right. Will fix it.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
> >>>>>>>> {
> >>>>>>>> + char *new_status = NULL;
> >>>>>>>> +
> >>>>>>>> + /* Report via ps we are waiting */
> >>>>>>>> + new_status = set_process_title_waiting();
> >>>>>>>>
> >>>>>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> >>>>>>>> display "waiting" in PS because no wait occurs when recovery conflict
> >>>>>>>> with database happens.
> >>>>>>>
> >>>>>>> Isn't the startup process waiting for other backend to terminate?
> >>>>>>
> >>>>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
> >>>>>>
> >>>>>> Currently ResolveRecoveryConflictWithLock() and
> >>>>>> ResolveRecoveryConflictWithBufferPin() don't call
> >>>>>> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
> >>>>>> in PS display. You changed them so that they report "waiting". I agree
> >>>>>> to have this change. But this change is an improvement rather than
> >>>>>> a bug fix, i.e., we should apply this change only in v13?
> >>>>>>
> >>>>>
> >>>>> Did you mean ResolveRecoveryConflictWithDatabase and
> >>>>> ResolveRecoveryConflictWithBufferPin?
> >>>>
> >>>> Yes! Sorry for my typo.
> >>>>
> >>>>> In the current code as far as I
> >>>>> researched there are two cases where we don't add "waiting" and one
> >>>>> case where we doubly add "waiting".
> >>>>>
> >>>>> ResolveRecoveryConflictWithDatabase and
> >>>>> ResolveRecoveryConflictWithBufferPin don't update the ps title.
> >>>>> Although the path where GetCurrentTimestamp() >= ltime is false in
> >>>>> ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
> >>>>> already updated in WaitOnLock. On the other hand, the path where
> >>>>> GetCurrentTimestamp() >= ltime is true in
> >>>>> ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
> >>>>> I reported.
> >>>>>
> >>>>> I've split the patch into two patches: 0001 patch fixes the issue
> >>>>> about doubly updating ps title, 0002 patch makes the recovery conflict
> >>>>> resolution on database and buffer pin update the ps title.
> >>>>
> >>>> Thanks for splitting the patches. I think that 0001 patch can be back-patched.
> >>>>
> >>>> - /*
> >>>> - * Report via ps if we have been waiting for more than 500 msec
> >>>> - * (should that be configurable?)
> >>>> - */
> >>>> - if (update_process_title && new_status == NULL &&
> >>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >>>> - 500))
> >>>>
> >>>> Originally, "waiting" is reported in PS if we've been waiting for more than
> >>>> 500 msec, as the above does. But you got rid of those codes in the patch.
> >>>> Did you confirm that it's safe to do that? If not, isn't it better to apply
> >>>> the attached patch?
> >>>
> >>> In WaitOnLock() we update the ps title regardless of waiting time. So
> >>> I thought we can change it to make these behavior consistent. But
> >>> considering back-patch, your patch looks better than mine.
> >>
> >> Yeah, so I pushed the 0001 patch at first!
> >> I will review the other patches later.
> >
> > Thank you!
> >
> > For 0002 patch which makes ResolveRecoveryConflictWithDatabase and
> > ResolveRecoveryConflictWithBufferPin update the ps title, I think
> > these are better to wait for 5ms before updating the ps title like
> > ResolveRecoveryConflictWithVirtualXIDs, for consistency among recovery
> > conflict resolution functions, but what do you think?
>
> Maybe yes.
>
> As another idea, for consistency, we can change all
> ResolveRecoveryConflictWithXXX() so that they don't wait
> at all before reporting "waiting". But if we don't do that,
> "waiting" can be reported even when we can immediately
> cancel or terminate the conflicting transactions (e.g., in
> case of max_standby_streaming_delay=0). To avoid this
> issue, I think it's better to wait for 500ms.
Agreed.
>
> The 0002 patch changes ResolveRecoveryConflictWithBufferPin()
> so that it updates PS every time. But this seems not good
> because the update can happen very frequently. Thought?
Agreed. In the updated version patch, I update the process title in
LockBufferForCleanup() only once when we've been waiting for more than
500 ms. This change also affects the primary server that is waiting
for buffer cleanup lock. I think it would not be bad but it's
different behaviour from LockBuffer().
I've attached the updated version patch. Please review it.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/03/05 20:16, Fujii Masao wrote:
>
>
> On 2020/03/05 16:58, Masahiko Sawada wrote:
>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>
>>>
>>>
>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>>> events by adding the new type of wait event such as
>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>>
>>>>> Yes, it looks like a improvement rather than bug fix.
>>>>>
>>>>
>>>> Okay, understand.
>>>>
>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>>> back-backpatching.
>>>>>
>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>>> fixed even in the back branches.
>>>>
>>>> So we need only two patches: one fixes process title issue and another
>>>> improve wait event. I've attached updated patches.
>>>
>>> Thanks for updating the patches! I started reading 0001 patch.
>>
>> Thank you for reviewing this patch.
>>
>>>
>>> - /*
>>> - * Report via ps if we have been waiting for more than 500 msec
>>> - * (should that be configurable?)
>>> - */
>>> - if (update_process_title && new_status == NULL &&
>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>> - 500))
>>>
>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>>> ResolveRecoveryConflictWithTablespace() so that they always add
>>> "waiting" into the PS display, whether wait is really necessary or not.
>>> But isn't it better to display "waiting" in PS basically when wait is
>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>>> does as the above?
>>
>> You're right. Will fix it.
>>
>>>
>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>>> {
>>> + char *new_status = NULL;
>>> +
>>> + /* Report via ps we are waiting */
>>> + new_status = set_process_title_waiting();
>>>
>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>>> display "waiting" in PS because no wait occurs when recovery conflict
>>> with database happens.
>>
>> Isn't the startup process waiting for other backend to terminate?
>
> Yeah, you're right. I agree that "waiting" should be reported in this case.
On second thought, in recovery conflict case, "waiting" should be reported
while waiting for the specified delay (e.g., by max_standby_streaming_delay)
until the conflict is resolved. So IMO reporting "waiting" in the case of
recovery conflict with buffer pin, snapshot, lock and tablespace seems valid,
because they are user-visible "expected" wait time.
However, in the case of recovery conflict with database, the recovery
basically doesn't wait at all and just terminates the conflicting sessions
immediately. Then the recovery waits for all those sessions to be terminated,
but that wait time is basically small and should not be the user-visible.
If that wait time becomes very long because of unresponsive backend, ISTM
that LOG or WARNING should be logged instead of reporting something in
PS display. I'm not sure if that logging is really necessary now, though.
Therefore, I'm thinking that "waiting" doesn't need to be reported in the case
of recovery conflict with database. Thought?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On Tue, 24 Mar 2020 at 17:04, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/05 20:16, Fujii Masao wrote:
> >
> >
> > On 2020/03/05 16:58, Masahiko Sawada wrote:
> >> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>
> >>>
> >>>
> >>> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 2020/03/04 13:27, Michael Paquier wrote:
> >>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>>>>> events by adding the new type of wait event such as
> >>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>>>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>>>>
> >>>>> Yes, it looks like a improvement rather than bug fix.
> >>>>>
> >>>>
> >>>> Okay, understand.
> >>>>
> >>>>>> I got my eyes on this patch set. The full patch set is in my opinion
> >>>>>> just a set of improvements, and not bug fixes, so I would refrain from
> >>>>>> back-backpatching.
> >>>>>
> >>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
> >>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >>>>> fixed even in the back branches.
> >>>>
> >>>> So we need only two patches: one fixes process title issue and another
> >>>> improve wait event. I've attached updated patches.
> >>>
> >>> Thanks for updating the patches! I started reading 0001 patch.
> >>
> >> Thank you for reviewing this patch.
> >>
> >>>
> >>> - /*
> >>> - * Report via ps if we have been waiting for more than 500 msec
> >>> - * (should that be configurable?)
> >>> - */
> >>> - if (update_process_title && new_status == NULL &&
> >>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >>> - 500))
> >>>
> >>> The patch changes ResolveRecoveryConflictWithSnapshot() and
> >>> ResolveRecoveryConflictWithTablespace() so that they always add
> >>> "waiting" into the PS display, whether wait is really necessary or not.
> >>> But isn't it better to display "waiting" in PS basically when wait is
> >>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> >>> does as the above?
> >>
> >> You're right. Will fix it.
> >>
> >>>
> >>> ResolveRecoveryConflictWithDatabase(Oid dbid)
> >>> {
> >>> + char *new_status = NULL;
> >>> +
> >>> + /* Report via ps we are waiting */
> >>> + new_status = set_process_title_waiting();
> >>>
> >>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> >>> display "waiting" in PS because no wait occurs when recovery conflict
> >>> with database happens.
> >>
> >> Isn't the startup process waiting for other backend to terminate?
> >
> > Yeah, you're right. I agree that "waiting" should be reported in this case.
>
> On second thought, in recovery conflict case, "waiting" should be reported
> while waiting for the specified delay (e.g., by max_standby_streaming_delay)
> until the conflict is resolved. So IMO reporting "waiting" in the case of
> recovery conflict with buffer pin, snapshot, lock and tablespace seems valid,
> because they are user-visible "expected" wait time.
>
> However, in the case of recovery conflict with database, the recovery
> basically doesn't wait at all and just terminates the conflicting sessions
> immediately. Then the recovery waits for all those sessions to be terminated,
> but that wait time is basically small and should not be the user-visible.
> If that wait time becomes very long because of unresponsive backend, ISTM
> that LOG or WARNING should be logged instead of reporting something in
> PS display. I'm not sure if that logging is really necessary now, though.
> Therefore, I'm thinking that "waiting" doesn't need to be reported in the case
> of recovery conflict with database. Thought?
Fair enough. The longer wait time of conflicts with database is not
user-expected behaviour so logging would be better. I'd like to just
drop the change around ResolveRecoveryConflictWithDatabase() from the
patch. Maybe logging LOG or WARNING for recovery conflict on database
would be a separate patch and need more discussion.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020/03/26 14:33, Masahiko Sawada wrote:
> On Tue, 24 Mar 2020 at 17:04, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/05 20:16, Fujii Masao wrote:
>>>
>>>
>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>>>>> events by adding the new type of wait event such as
>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>>>>
>>>>>>> Yes, it looks like a improvement rather than bug fix.
>>>>>>>
>>>>>>
>>>>>> Okay, understand.
>>>>>>
>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>>>>> back-backpatching.
>>>>>>>
>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>>>>> fixed even in the back branches.
>>>>>>
>>>>>> So we need only two patches: one fixes process title issue and another
>>>>>> improve wait event. I've attached updated patches.
>>>>>
>>>>> Thanks for updating the patches! I started reading 0001 patch.
>>>>
>>>> Thank you for reviewing this patch.
>>>>
>>>>>
>>>>> - /*
>>>>> - * Report via ps if we have been waiting for more than 500 msec
>>>>> - * (should that be configurable?)
>>>>> - */
>>>>> - if (update_process_title && new_status == NULL &&
>>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>>>> - 500))
>>>>>
>>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>>>>> ResolveRecoveryConflictWithTablespace() so that they always add
>>>>> "waiting" into the PS display, whether wait is really necessary or not.
>>>>> But isn't it better to display "waiting" in PS basically when wait is
>>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>>>>> does as the above?
>>>>
>>>> You're right. Will fix it.
>>>>
>>>>>
>>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>>>>> {
>>>>> + char *new_status = NULL;
>>>>> +
>>>>> + /* Report via ps we are waiting */
>>>>> + new_status = set_process_title_waiting();
>>>>>
>>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>>>>> display "waiting" in PS because no wait occurs when recovery conflict
>>>>> with database happens.
>>>>
>>>> Isn't the startup process waiting for other backend to terminate?
>>>
>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
>>
>> On second thought, in recovery conflict case, "waiting" should be reported
>> while waiting for the specified delay (e.g., by max_standby_streaming_delay)
>> until the conflict is resolved. So IMO reporting "waiting" in the case of
>> recovery conflict with buffer pin, snapshot, lock and tablespace seems valid,
>> because they are user-visible "expected" wait time.
>>
>> However, in the case of recovery conflict with database, the recovery
>> basically doesn't wait at all and just terminates the conflicting sessions
>> immediately. Then the recovery waits for all those sessions to be terminated,
>> but that wait time is basically small and should not be the user-visible.
>> If that wait time becomes very long because of unresponsive backend, ISTM
>> that LOG or WARNING should be logged instead of reporting something in
>> PS display. I'm not sure if that logging is really necessary now, though.
>> Therefore, I'm thinking that "waiting" doesn't need to be reported in the case
>> of recovery conflict with database. Thought?
>
> Fair enough. The longer wait time of conflicts with database is not
> user-expected behaviour so logging would be better. I'd like to just
> drop the change around ResolveRecoveryConflictWithDatabase() from the
> patch.
Make sense.
+ if (update_process_title)
+ waitStart = GetCurrentTimestamp();
Since LockBufferForCleanup() can be called very frequently,
I don't think that it's good thing to call GetCurrentTimestamp()
every time when LockBufferForCleanup() is called.
+ /* Report via ps if we have been waiting for more than 500 msec */
+ if (update_process_title && new_status == NULL &&
+ TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
+ 500))
Do we really want to see "waiting" in PS even in non hot standby mode?
If max_standby_streaming_delay and deadlock_timeout are set to large value,
ResolveRecoveryConflictWithBufferPin() can wait for a long time, e.g.,
more than 500ms. In that case, I'm afraid that "report if we've been
waiting for more than 500ms" logic doesn't work.
So I'm now thinking that it's better to report "waiting" immdiately before
ResolveRecoveryConflictWithBufferPin(). Of course, we can still use
"report if we've been waiting for more than 500ms" logic by teaching 500ms
to ResolveRecoveryConflictWithBufferPin() as the minimum wait time.
But this looks overkill. Thought?
Based on the above comments, I updated the patch. Attached. Right now
the patch looks very simple. Could you review this patch?
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
Вложения
On Fri, 27 Mar 2020 at 10:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/26 14:33, Masahiko Sawada wrote:
> > On Tue, 24 Mar 2020 at 17:04, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/05 20:16, Fujii Masao wrote:
> >>>
> >>>
> >>> On 2020/03/05 16:58, Masahiko Sawada wrote:
> >>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
> >>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
> >>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
> >>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
> >>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
> >>>>>>>>> events by adding the new type of wait event such as
> >>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
> >>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
> >>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
> >>>>>>>
> >>>>>>> Yes, it looks like a improvement rather than bug fix.
> >>>>>>>
> >>>>>>
> >>>>>> Okay, understand.
> >>>>>>
> >>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
> >>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
> >>>>>>>> back-backpatching.
> >>>>>>>
> >>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
> >>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
> >>>>>>> fixed even in the back branches.
> >>>>>>
> >>>>>> So we need only two patches: one fixes process title issue and another
> >>>>>> improve wait event. I've attached updated patches.
> >>>>>
> >>>>> Thanks for updating the patches! I started reading 0001 patch.
> >>>>
> >>>> Thank you for reviewing this patch.
> >>>>
> >>>>>
> >>>>> - /*
> >>>>> - * Report via ps if we have been waiting for more than 500 msec
> >>>>> - * (should that be configurable?)
> >>>>> - */
> >>>>> - if (update_process_title && new_status == NULL &&
> >>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> >>>>> - 500))
> >>>>>
> >>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
> >>>>> ResolveRecoveryConflictWithTablespace() so that they always add
> >>>>> "waiting" into the PS display, whether wait is really necessary or not.
> >>>>> But isn't it better to display "waiting" in PS basically when wait is
> >>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
> >>>>> does as the above?
> >>>>
> >>>> You're right. Will fix it.
> >>>>
> >>>>>
> >>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
> >>>>> {
> >>>>> + char *new_status = NULL;
> >>>>> +
> >>>>> + /* Report via ps we are waiting */
> >>>>> + new_status = set_process_title_waiting();
> >>>>>
> >>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
> >>>>> display "waiting" in PS because no wait occurs when recovery conflict
> >>>>> with database happens.
> >>>>
> >>>> Isn't the startup process waiting for other backend to terminate?
> >>>
> >>> Yeah, you're right. I agree that "waiting" should be reported in this case.
> >>
> >> On second thought, in recovery conflict case, "waiting" should be reported
> >> while waiting for the specified delay (e.g., by max_standby_streaming_delay)
> >> until the conflict is resolved. So IMO reporting "waiting" in the case of
> >> recovery conflict with buffer pin, snapshot, lock and tablespace seems valid,
> >> because they are user-visible "expected" wait time.
> >>
> >> However, in the case of recovery conflict with database, the recovery
> >> basically doesn't wait at all and just terminates the conflicting sessions
> >> immediately. Then the recovery waits for all those sessions to be terminated,
> >> but that wait time is basically small and should not be the user-visible.
> >> If that wait time becomes very long because of unresponsive backend, ISTM
> >> that LOG or WARNING should be logged instead of reporting something in
> >> PS display. I'm not sure if that logging is really necessary now, though.
> >> Therefore, I'm thinking that "waiting" doesn't need to be reported in the case
> >> of recovery conflict with database. Thought?
> >
> > Fair enough. The longer wait time of conflicts with database is not
> > user-expected behaviour so logging would be better. I'd like to just
> > drop the change around ResolveRecoveryConflictWithDatabase() from the
> > patch.
>
> Make sense.
>
> + if (update_process_title)
> + waitStart = GetCurrentTimestamp();
>
> Since LockBufferForCleanup() can be called very frequently,
> I don't think that it's good thing to call GetCurrentTimestamp()
> every time when LockBufferForCleanup() is called.
>
> + /* Report via ps if we have been waiting for more than 500 msec */
> + if (update_process_title && new_status == NULL &&
> + TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
> + 500))
>
> Do we really want to see "waiting" in PS even in non hot standby mode?
>
> If max_standby_streaming_delay and deadlock_timeout are set to large value,
> ResolveRecoveryConflictWithBufferPin() can wait for a long time, e.g.,
> more than 500ms. In that case, I'm afraid that "report if we've been
> waiting for more than 500ms" logic doesn't work.
>
> So I'm now thinking that it's better to report "waiting" immdiately before
> ResolveRecoveryConflictWithBufferPin(). Of course, we can still use
> "report if we've been waiting for more than 500ms" logic by teaching 500ms
> to ResolveRecoveryConflictWithBufferPin() as the minimum wait time.
> But this looks overkill. Thought?
>
> Based on the above comments, I updated the patch. Attached. Right now
> the patch looks very simple. Could you review this patch?
Thank you for the patch. I agree with you for all the points. Your
patch looks good to me.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020/03/27 15:39, Masahiko Sawada wrote:
> On Fri, 27 Mar 2020 at 10:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/03/26 14:33, Masahiko Sawada wrote:
>>> On Tue, 24 Mar 2020 at 17:04, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2020/03/05 20:16, Fujii Masao wrote:
>>>>>
>>>>>
>>>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
>>>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
>>>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote:
>>>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>>>>>>>>> events by adding the new type of wait event such as
>>>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>>>>>>>>
>>>>>>>>> Yes, it looks like a improvement rather than bug fix.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Okay, understand.
>>>>>>>>
>>>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion
>>>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from
>>>>>>>>>> back-backpatching.
>>>>>>>>>
>>>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly
>>>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>>>>>>>>> fixed even in the back branches.
>>>>>>>>
>>>>>>>> So we need only two patches: one fixes process title issue and another
>>>>>>>> improve wait event. I've attached updated patches.
>>>>>>>
>>>>>>> Thanks for updating the patches! I started reading 0001 patch.
>>>>>>
>>>>>> Thank you for reviewing this patch.
>>>>>>
>>>>>>>
>>>>>>> - /*
>>>>>>> - * Report via ps if we have been waiting for more than 500 msec
>>>>>>> - * (should that be configurable?)
>>>>>>> - */
>>>>>>> - if (update_process_title && new_status == NULL &&
>>>>>>> - TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>>>>>>> - 500))
>>>>>>>
>>>>>>> The patch changes ResolveRecoveryConflictWithSnapshot() and
>>>>>>> ResolveRecoveryConflictWithTablespace() so that they always add
>>>>>>> "waiting" into the PS display, whether wait is really necessary or not.
>>>>>>> But isn't it better to display "waiting" in PS basically when wait is
>>>>>>> necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
>>>>>>> does as the above?
>>>>>>
>>>>>> You're right. Will fix it.
>>>>>>
>>>>>>>
>>>>>>> ResolveRecoveryConflictWithDatabase(Oid dbid)
>>>>>>> {
>>>>>>> + char *new_status = NULL;
>>>>>>> +
>>>>>>> + /* Report via ps we are waiting */
>>>>>>> + new_status = set_process_title_waiting();
>>>>>>>
>>>>>>> In ResolveRecoveryConflictWithDatabase(), there seems no need to
>>>>>>> display "waiting" in PS because no wait occurs when recovery conflict
>>>>>>> with database happens.
>>>>>>
>>>>>> Isn't the startup process waiting for other backend to terminate?
>>>>>
>>>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
>>>>
>>>> On second thought, in recovery conflict case, "waiting" should be reported
>>>> while waiting for the specified delay (e.g., by max_standby_streaming_delay)
>>>> until the conflict is resolved. So IMO reporting "waiting" in the case of
>>>> recovery conflict with buffer pin, snapshot, lock and tablespace seems valid,
>>>> because they are user-visible "expected" wait time.
>>>>
>>>> However, in the case of recovery conflict with database, the recovery
>>>> basically doesn't wait at all and just terminates the conflicting sessions
>>>> immediately. Then the recovery waits for all those sessions to be terminated,
>>>> but that wait time is basically small and should not be the user-visible.
>>>> If that wait time becomes very long because of unresponsive backend, ISTM
>>>> that LOG or WARNING should be logged instead of reporting something in
>>>> PS display. I'm not sure if that logging is really necessary now, though.
>>>> Therefore, I'm thinking that "waiting" doesn't need to be reported in the case
>>>> of recovery conflict with database. Thought?
>>>
>>> Fair enough. The longer wait time of conflicts with database is not
>>> user-expected behaviour so logging would be better. I'd like to just
>>> drop the change around ResolveRecoveryConflictWithDatabase() from the
>>> patch.
>>
>> Make sense.
>>
>> + if (update_process_title)
>> + waitStart = GetCurrentTimestamp();
>>
>> Since LockBufferForCleanup() can be called very frequently,
>> I don't think that it's good thing to call GetCurrentTimestamp()
>> every time when LockBufferForCleanup() is called.
>>
>> + /* Report via ps if we have been waiting for more than 500 msec */
>> + if (update_process_title && new_status == NULL &&
>> + TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
>> + 500))
>>
>> Do we really want to see "waiting" in PS even in non hot standby mode?
>>
>> If max_standby_streaming_delay and deadlock_timeout are set to large value,
>> ResolveRecoveryConflictWithBufferPin() can wait for a long time, e.g.,
>> more than 500ms. In that case, I'm afraid that "report if we've been
>> waiting for more than 500ms" logic doesn't work.
>>
>> So I'm now thinking that it's better to report "waiting" immdiately before
>> ResolveRecoveryConflictWithBufferPin(). Of course, we can still use
>> "report if we've been waiting for more than 500ms" logic by teaching 500ms
>> to ResolveRecoveryConflictWithBufferPin() as the minimum wait time.
>> But this looks overkill. Thought?
>>
>> Based on the above comments, I updated the patch. Attached. Right now
>> the patch looks very simple. Could you review this patch?
>
> Thank you for the patch. I agree with you for all the points. Your
> patch looks good to me.
Thanks for the review! Barring any objections, I will commit the latest patch.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
On 2020/03/04 14:31, Masahiko Sawada wrote: > On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> >> >> >> On 2020/03/04 13:27, Michael Paquier wrote: >>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: >>>> Yeah, so 0001 patch sets existing wait events to recovery conflict >>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) >>>> to the recovery conflict on a snapshot. 0003 patch improves these wait >>>> events by adding the new type of wait event such as >>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch >>>> is the fix for existing versions and 0003 patch is an improvement for >>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? >> >> Yes, it looks like a improvement rather than bug fix. >> > > Okay, understand. > >>> I got my eyes on this patch set. The full patch set is in my opinion >>> just a set of improvements, and not bug fixes, so I would refrain from >>> back-backpatching. >> >> I think that the issue (i.e., "waiting" is reported twice needlessly >> in PS display) that 0002 patch tries to fix is a bug. So it should be >> fixed even in the back branches. > > So we need only two patches: one fixes process title issue and another > improve wait event. I've attached updated patches. I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); Currently the wait event indicating the wait for buffer pin has already been reported. But the above change in the patch changes the name of wait event for buffer pin only in the startup process. Is this really useful? Isn't the existing wait event for buffer pin enough? - /* Wait to be signaled by the release of the Relation Lock */ - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); + /* Wait to be signaled by the release of the Relation Lock */ + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); Same as above. Isn't the existing wait event enough? - /* - * Progressively increase the sleep times, but not to more than 1s, since - * pg_usleep isn't interruptible on some platforms. - */ - standbyWait_us *= 2; - if (standbyWait_us > 1000000) - standbyWait_us = 1000000; + WaitLatch(MyLatch, + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, + STANDBY_WAIT_MS, + wait_event_info); + ResetLatch(MyLatch); ResetLatch() should be called before WaitLatch()? Could you tell me why you dropped the "increase-sleep-times" logic? Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/03/04 14:31, Masahiko Sawada wrote: > > On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >> > >> > >> > >> On 2020/03/04 13:27, Michael Paquier wrote: > >>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: > >>>> Yeah, so 0001 patch sets existing wait events to recovery conflict > >>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) > >>>> to the recovery conflict on a snapshot. 0003 patch improves these wait > >>>> events by adding the new type of wait event such as > >>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch > >>>> is the fix for existing versions and 0003 patch is an improvement for > >>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? > >> > >> Yes, it looks like a improvement rather than bug fix. > >> > > > > Okay, understand. > > > >>> I got my eyes on this patch set. The full patch set is in my opinion > >>> just a set of improvements, and not bug fixes, so I would refrain from > >>> back-backpatching. > >> > >> I think that the issue (i.e., "waiting" is reported twice needlessly > >> in PS display) that 0002 patch tries to fix is a bug. So it should be > >> fixed even in the back branches. > > > > So we need only two patches: one fixes process title issue and another > > improve wait event. I've attached updated patches. > > I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. > > - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); > + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); > > Currently the wait event indicating the wait for buffer pin has already > been reported. But the above change in the patch changes the name of > wait event for buffer pin only in the startup process. Is this really useful? > Isn't the existing wait event for buffer pin enough? > > - /* Wait to be signaled by the release of the Relation Lock */ > - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > + /* Wait to be signaled by the release of the Relation Lock */ > + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); > > Same as above. Isn't the existing wait event enough? Yeah, we can use the existing wait events for buffer pin and lock. > > - /* > - * Progressively increase the sleep times, but not to more than 1s, since > - * pg_usleep isn't interruptible on some platforms. > - */ > - standbyWait_us *= 2; > - if (standbyWait_us > 1000000) > - standbyWait_us = 1000000; > + WaitLatch(MyLatch, > + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, > + STANDBY_WAIT_MS, > + wait_event_info); > + ResetLatch(MyLatch); > > ResetLatch() should be called before WaitLatch()? Fixed. > > Could you tell me why you dropped the "increase-sleep-times" logic? I thought we can remove it because WaitLatch is interruptible but my observation was not correct. The waiting startup process is not necessarily woken up by signal. I think it's still better to not wait more than 1 sec even if it's an interruptible wait. Attached patch fixes the above and introduces only two wait events of conflict resolution: snapshot and tablespace. I also removed the wait event of conflict resolution of database since it's unlikely to become a user-visible and a long sleep as we discussed before. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/03/30 20:10, Masahiko Sawada wrote: > On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> >> >> >> On 2020/03/04 14:31, Masahiko Sawada wrote: >>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>> >>>> >>>> >>>> On 2020/03/04 13:27, Michael Paquier wrote: >>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: >>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict >>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) >>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait >>>>>> events by adding the new type of wait event such as >>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch >>>>>> is the fix for existing versions and 0003 patch is an improvement for >>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? >>>> >>>> Yes, it looks like a improvement rather than bug fix. >>>> >>> >>> Okay, understand. >>> >>>>> I got my eyes on this patch set. The full patch set is in my opinion >>>>> just a set of improvements, and not bug fixes, so I would refrain from >>>>> back-backpatching. >>>> >>>> I think that the issue (i.e., "waiting" is reported twice needlessly >>>> in PS display) that 0002 patch tries to fix is a bug. So it should be >>>> fixed even in the back branches. >>> >>> So we need only two patches: one fixes process title issue and another >>> improve wait event. I've attached updated patches. >> >> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. >> >> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); >> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); >> >> Currently the wait event indicating the wait for buffer pin has already >> been reported. But the above change in the patch changes the name of >> wait event for buffer pin only in the startup process. Is this really useful? >> Isn't the existing wait event for buffer pin enough? >> >> - /* Wait to be signaled by the release of the Relation Lock */ >> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >> + /* Wait to be signaled by the release of the Relation Lock */ >> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); >> >> Same as above. Isn't the existing wait event enough? > > Yeah, we can use the existing wait events for buffer pin and lock. > >> >> - /* >> - * Progressively increase the sleep times, but not to more than 1s, since >> - * pg_usleep isn't interruptible on some platforms. >> - */ >> - standbyWait_us *= 2; >> - if (standbyWait_us > 1000000) >> - standbyWait_us = 1000000; >> + WaitLatch(MyLatch, >> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, >> + STANDBY_WAIT_MS, >> + wait_event_info); >> + ResetLatch(MyLatch); >> >> ResetLatch() should be called before WaitLatch()? > > Fixed. > >> >> Could you tell me why you dropped the "increase-sleep-times" logic? > > I thought we can remove it because WaitLatch is interruptible but my > observation was not correct. The waiting startup process is not > necessarily woken up by signal. I think it's still better to not wait > more than 1 sec even if it's an interruptible wait. So we don't need to use WaitLatch() there, i.e., it's ok to keep using pg_usleep()? > Attached patch fixes the above and introduces only two wait events of > conflict resolution: snapshot and tablespace. Many thanks for updating the patch! - /* Wait to be signaled by the release of the Relation Lock */ - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); + /* Wait to be signaled by the release of the Relation Lock */ + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); + } Is this change really valid? What happens if the latch is set during ResolveRecoveryConflictWithVirtualXIDs()? ResolveRecoveryConflictWithVirtualXIDs() can return after the latch is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. + default: + event_name = "unknown wait event"; + break; Seems this default case should be removed. Please see other pgstat_get_wait_xxx() function, so there is no such code. > I also removed the wait > event of conflict resolution of database since it's unlikely to become > a user-visible and a long sleep as we discussed before. Is it worth defining new wait event type RecoveryConflict only for those two events? ISTM that it's ok to use IPC event type here. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Wed, 1 Apr 2020 at 22:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/03/30 20:10, Masahiko Sawada wrote: > > On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >> > >> > >> > >> On 2020/03/04 14:31, Masahiko Sawada wrote: > >>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2020/03/04 13:27, Michael Paquier wrote: > >>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: > >>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict > >>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) > >>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait > >>>>>> events by adding the new type of wait event such as > >>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch > >>>>>> is the fix for existing versions and 0003 patch is an improvement for > >>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? > >>>> > >>>> Yes, it looks like a improvement rather than bug fix. > >>>> > >>> > >>> Okay, understand. > >>> > >>>>> I got my eyes on this patch set. The full patch set is in my opinion > >>>>> just a set of improvements, and not bug fixes, so I would refrain from > >>>>> back-backpatching. > >>>> > >>>> I think that the issue (i.e., "waiting" is reported twice needlessly > >>>> in PS display) that 0002 patch tries to fix is a bug. So it should be > >>>> fixed even in the back branches. > >>> > >>> So we need only two patches: one fixes process title issue and another > >>> improve wait event. I've attached updated patches. > >> > >> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. > >> > >> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); > >> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); > >> > >> Currently the wait event indicating the wait for buffer pin has already > >> been reported. But the above change in the patch changes the name of > >> wait event for buffer pin only in the startup process. Is this really useful? > >> Isn't the existing wait event for buffer pin enough? > >> > >> - /* Wait to be signaled by the release of the Relation Lock */ > >> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >> + /* Wait to be signaled by the release of the Relation Lock */ > >> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); > >> > >> Same as above. Isn't the existing wait event enough? > > > > Yeah, we can use the existing wait events for buffer pin and lock. > > > >> > >> - /* > >> - * Progressively increase the sleep times, but not to more than 1s, since > >> - * pg_usleep isn't interruptible on some platforms. > >> - */ > >> - standbyWait_us *= 2; > >> - if (standbyWait_us > 1000000) > >> - standbyWait_us = 1000000; > >> + WaitLatch(MyLatch, > >> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, > >> + STANDBY_WAIT_MS, > >> + wait_event_info); > >> + ResetLatch(MyLatch); > >> > >> ResetLatch() should be called before WaitLatch()? > > > > Fixed. > > > >> > >> Could you tell me why you dropped the "increase-sleep-times" logic? > > > > I thought we can remove it because WaitLatch is interruptible but my > > observation was not correct. The waiting startup process is not > > necessarily woken up by signal. I think it's still better to not wait > > more than 1 sec even if it's an interruptible wait. > > So we don't need to use WaitLatch() there, i.e., it's ok to keep using > pg_usleep()? > > > Attached patch fixes the above and introduces only two wait events of > > conflict resolution: snapshot and tablespace. > > Many thanks for updating the patch! > > - /* Wait to be signaled by the release of the Relation Lock */ > - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > + /* Wait to be signaled by the release of the Relation Lock */ > + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > + } > > Is this change really valid? What happens if the latch is set during > ResolveRecoveryConflictWithVirtualXIDs()? > ResolveRecoveryConflictWithVirtualXIDs() can return after the latch > is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. Thank you for reviewing the patch! You're right. It's better to keep using pg_usleep() and set the wait event by pgstat_report_wait_start(). > > + default: > + event_name = "unknown wait event"; > + break; > > Seems this default case should be removed. Please see other > pgstat_get_wait_xxx() function, so there is no such code. > > > I also removed the wait > > event of conflict resolution of database since it's unlikely to become > > a user-visible and a long sleep as we discussed before. > > Is it worth defining new wait event type RecoveryConflict only for > those two events? ISTM that it's ok to use IPC event type here. > I dropped a new wait even type and added them to IPC wait event type. I've attached the new version patch. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/04/02 14:25, Masahiko Sawada wrote: > On Wed, 1 Apr 2020 at 22:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> >> >> >> On 2020/03/30 20:10, Masahiko Sawada wrote: >>> On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>> >>>> >>>> >>>> On 2020/03/04 14:31, Masahiko Sawada wrote: >>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2020/03/04 13:27, Michael Paquier wrote: >>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: >>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict >>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) >>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait >>>>>>>> events by adding the new type of wait event such as >>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch >>>>>>>> is the fix for existing versions and 0003 patch is an improvement for >>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? >>>>>> >>>>>> Yes, it looks like a improvement rather than bug fix. >>>>>> >>>>> >>>>> Okay, understand. >>>>> >>>>>>> I got my eyes on this patch set. The full patch set is in my opinion >>>>>>> just a set of improvements, and not bug fixes, so I would refrain from >>>>>>> back-backpatching. >>>>>> >>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly >>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be >>>>>> fixed even in the back branches. >>>>> >>>>> So we need only two patches: one fixes process title issue and another >>>>> improve wait event. I've attached updated patches. >>>> >>>> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. >>>> >>>> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); >>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); >>>> >>>> Currently the wait event indicating the wait for buffer pin has already >>>> been reported. But the above change in the patch changes the name of >>>> wait event for buffer pin only in the startup process. Is this really useful? >>>> Isn't the existing wait event for buffer pin enough? >>>> >>>> - /* Wait to be signaled by the release of the Relation Lock */ >>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>> + /* Wait to be signaled by the release of the Relation Lock */ >>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); >>>> >>>> Same as above. Isn't the existing wait event enough? >>> >>> Yeah, we can use the existing wait events for buffer pin and lock. >>> >>>> >>>> - /* >>>> - * Progressively increase the sleep times, but not to more than 1s, since >>>> - * pg_usleep isn't interruptible on some platforms. >>>> - */ >>>> - standbyWait_us *= 2; >>>> - if (standbyWait_us > 1000000) >>>> - standbyWait_us = 1000000; >>>> + WaitLatch(MyLatch, >>>> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, >>>> + STANDBY_WAIT_MS, >>>> + wait_event_info); >>>> + ResetLatch(MyLatch); >>>> >>>> ResetLatch() should be called before WaitLatch()? >>> >>> Fixed. >>> >>>> >>>> Could you tell me why you dropped the "increase-sleep-times" logic? >>> >>> I thought we can remove it because WaitLatch is interruptible but my >>> observation was not correct. The waiting startup process is not >>> necessarily woken up by signal. I think it's still better to not wait >>> more than 1 sec even if it's an interruptible wait. >> >> So we don't need to use WaitLatch() there, i.e., it's ok to keep using >> pg_usleep()? >> >>> Attached patch fixes the above and introduces only two wait events of >>> conflict resolution: snapshot and tablespace. >> >> Many thanks for updating the patch! >> >> - /* Wait to be signaled by the release of the Relation Lock */ >> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >> + /* Wait to be signaled by the release of the Relation Lock */ >> + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >> + } >> >> Is this change really valid? What happens if the latch is set during >> ResolveRecoveryConflictWithVirtualXIDs()? >> ResolveRecoveryConflictWithVirtualXIDs() can return after the latch >> is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. > > Thank you for reviewing the patch! > > You're right. It's better to keep using pg_usleep() and set the wait > event by pgstat_report_wait_start(). > >> >> + default: >> + event_name = "unknown wait event"; >> + break; >> >> Seems this default case should be removed. Please see other >> pgstat_get_wait_xxx() function, so there is no such code. >> >>> I also removed the wait >>> event of conflict resolution of database since it's unlikely to become >>> a user-visible and a long sleep as we discussed before. >> >> Is it worth defining new wait event type RecoveryConflict only for >> those two events? ISTM that it's ok to use IPC event type here. >> > > I dropped a new wait even type and added them to IPC wait event type. > > I've attached the new version patch. Thanks for updating the patch! The patch looks good to me except the following mior things. + <row> + <entry><literal>RecoveryConflictSnapshot</literal></entry> + <entry>Waiting for recovery conflict resolution on a physical cleanup.</entry> + </row> + <row> + <entry><literal>RecoveryConflictTablespace</literal></entry> + <entry>Waiting for recovery conflict resolution on dropping tablespace.</entry> + </row> You need to increment the value of "morerows" in "<entry morerows="38"><literal>IPC</literal></entry>". The descriptions of those two events should be placed in alphabetical order for event name. That is, they should be placed above RecoveryPause. "vacuum cleanup" is better than "physical cleanup"? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Thu, 2 Apr 2020 at 15:34, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/04/02 14:25, Masahiko Sawada wrote: > > On Wed, 1 Apr 2020 at 22:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >> > >> > >> > >> On 2020/03/30 20:10, Masahiko Sawada wrote: > >>> On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2020/03/04 14:31, Masahiko Sawada wrote: > >>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 2020/03/04 13:27, Michael Paquier wrote: > >>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: > >>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict > >>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) > >>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait > >>>>>>>> events by adding the new type of wait event such as > >>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch > >>>>>>>> is the fix for existing versions and 0003 patch is an improvement for > >>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? > >>>>>> > >>>>>> Yes, it looks like a improvement rather than bug fix. > >>>>>> > >>>>> > >>>>> Okay, understand. > >>>>> > >>>>>>> I got my eyes on this patch set. The full patch set is in my opinion > >>>>>>> just a set of improvements, and not bug fixes, so I would refrain from > >>>>>>> back-backpatching. > >>>>>> > >>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly > >>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be > >>>>>> fixed even in the back branches. > >>>>> > >>>>> So we need only two patches: one fixes process title issue and another > >>>>> improve wait event. I've attached updated patches. > >>>> > >>>> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. > >>>> > >>>> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); > >>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); > >>>> > >>>> Currently the wait event indicating the wait for buffer pin has already > >>>> been reported. But the above change in the patch changes the name of > >>>> wait event for buffer pin only in the startup process. Is this really useful? > >>>> Isn't the existing wait event for buffer pin enough? > >>>> > >>>> - /* Wait to be signaled by the release of the Relation Lock */ > >>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >>>> + /* Wait to be signaled by the release of the Relation Lock */ > >>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); > >>>> > >>>> Same as above. Isn't the existing wait event enough? > >>> > >>> Yeah, we can use the existing wait events for buffer pin and lock. > >>> > >>>> > >>>> - /* > >>>> - * Progressively increase the sleep times, but not to more than 1s, since > >>>> - * pg_usleep isn't interruptible on some platforms. > >>>> - */ > >>>> - standbyWait_us *= 2; > >>>> - if (standbyWait_us > 1000000) > >>>> - standbyWait_us = 1000000; > >>>> + WaitLatch(MyLatch, > >>>> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, > >>>> + STANDBY_WAIT_MS, > >>>> + wait_event_info); > >>>> + ResetLatch(MyLatch); > >>>> > >>>> ResetLatch() should be called before WaitLatch()? > >>> > >>> Fixed. > >>> > >>>> > >>>> Could you tell me why you dropped the "increase-sleep-times" logic? > >>> > >>> I thought we can remove it because WaitLatch is interruptible but my > >>> observation was not correct. The waiting startup process is not > >>> necessarily woken up by signal. I think it's still better to not wait > >>> more than 1 sec even if it's an interruptible wait. > >> > >> So we don't need to use WaitLatch() there, i.e., it's ok to keep using > >> pg_usleep()? > >> > >>> Attached patch fixes the above and introduces only two wait events of > >>> conflict resolution: snapshot and tablespace. > >> > >> Many thanks for updating the patch! > >> > >> - /* Wait to be signaled by the release of the Relation Lock */ > >> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >> + /* Wait to be signaled by the release of the Relation Lock */ > >> + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >> + } > >> > >> Is this change really valid? What happens if the latch is set during > >> ResolveRecoveryConflictWithVirtualXIDs()? > >> ResolveRecoveryConflictWithVirtualXIDs() can return after the latch > >> is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. > > > > Thank you for reviewing the patch! > > > > You're right. It's better to keep using pg_usleep() and set the wait > > event by pgstat_report_wait_start(). > > > >> > >> + default: > >> + event_name = "unknown wait event"; > >> + break; > >> > >> Seems this default case should be removed. Please see other > >> pgstat_get_wait_xxx() function, so there is no such code. > >> > >>> I also removed the wait > >>> event of conflict resolution of database since it's unlikely to become > >>> a user-visible and a long sleep as we discussed before. > >> > >> Is it worth defining new wait event type RecoveryConflict only for > >> those two events? ISTM that it's ok to use IPC event type here. > >> > > > > I dropped a new wait even type and added them to IPC wait event type. > > > > I've attached the new version patch. > > Thanks for updating the patch! The patch looks good to me except > the following mior things. > > + <row> > + <entry><literal>RecoveryConflictSnapshot</literal></entry> > + <entry>Waiting for recovery conflict resolution on a physical cleanup.</entry> > + </row> > + <row> > + <entry><literal>RecoveryConflictTablespace</literal></entry> > + <entry>Waiting for recovery conflict resolution on dropping tablespace.</entry> > + </row> > > You need to increment the value of "morerows" in > "<entry morerows="38"><literal>IPC</literal></entry>". > > The descriptions of those two events should be placed in alphabetical order > for event name. That is, they should be placed above RecoveryPause. > > "vacuum cleanup" is better than "physical cleanup"? Agreed. I've attached the updated version patch. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2020/04/02 15:54, Masahiko Sawada wrote: > On Thu, 2 Apr 2020 at 15:34, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> >> >> >> On 2020/04/02 14:25, Masahiko Sawada wrote: >>> On Wed, 1 Apr 2020 at 22:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>> >>>> >>>> >>>> On 2020/03/30 20:10, Masahiko Sawada wrote: >>>>> On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote: >>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote: >>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: >>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict >>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) >>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait >>>>>>>>>> events by adding the new type of wait event such as >>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch >>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for >>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? >>>>>>>> >>>>>>>> Yes, it looks like a improvement rather than bug fix. >>>>>>>> >>>>>>> >>>>>>> Okay, understand. >>>>>>> >>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion >>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from >>>>>>>>> back-backpatching. >>>>>>>> >>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly >>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be >>>>>>>> fixed even in the back branches. >>>>>>> >>>>>>> So we need only two patches: one fixes process title issue and another >>>>>>> improve wait event. I've attached updated patches. >>>>>> >>>>>> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. >>>>>> >>>>>> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); >>>>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); >>>>>> >>>>>> Currently the wait event indicating the wait for buffer pin has already >>>>>> been reported. But the above change in the patch changes the name of >>>>>> wait event for buffer pin only in the startup process. Is this really useful? >>>>>> Isn't the existing wait event for buffer pin enough? >>>>>> >>>>>> - /* Wait to be signaled by the release of the Relation Lock */ >>>>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>>>> + /* Wait to be signaled by the release of the Relation Lock */ >>>>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); >>>>>> >>>>>> Same as above. Isn't the existing wait event enough? >>>>> >>>>> Yeah, we can use the existing wait events for buffer pin and lock. >>>>> >>>>>> >>>>>> - /* >>>>>> - * Progressively increase the sleep times, but not to more than 1s, since >>>>>> - * pg_usleep isn't interruptible on some platforms. >>>>>> - */ >>>>>> - standbyWait_us *= 2; >>>>>> - if (standbyWait_us > 1000000) >>>>>> - standbyWait_us = 1000000; >>>>>> + WaitLatch(MyLatch, >>>>>> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, >>>>>> + STANDBY_WAIT_MS, >>>>>> + wait_event_info); >>>>>> + ResetLatch(MyLatch); >>>>>> >>>>>> ResetLatch() should be called before WaitLatch()? >>>>> >>>>> Fixed. >>>>> >>>>>> >>>>>> Could you tell me why you dropped the "increase-sleep-times" logic? >>>>> >>>>> I thought we can remove it because WaitLatch is interruptible but my >>>>> observation was not correct. The waiting startup process is not >>>>> necessarily woken up by signal. I think it's still better to not wait >>>>> more than 1 sec even if it's an interruptible wait. >>>> >>>> So we don't need to use WaitLatch() there, i.e., it's ok to keep using >>>> pg_usleep()? >>>> >>>>> Attached patch fixes the above and introduces only two wait events of >>>>> conflict resolution: snapshot and tablespace. >>>> >>>> Many thanks for updating the patch! >>>> >>>> - /* Wait to be signaled by the release of the Relation Lock */ >>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>> + /* Wait to be signaled by the release of the Relation Lock */ >>>> + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>> + } >>>> >>>> Is this change really valid? What happens if the latch is set during >>>> ResolveRecoveryConflictWithVirtualXIDs()? >>>> ResolveRecoveryConflictWithVirtualXIDs() can return after the latch >>>> is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. >>> >>> Thank you for reviewing the patch! >>> >>> You're right. It's better to keep using pg_usleep() and set the wait >>> event by pgstat_report_wait_start(). >>> >>>> >>>> + default: >>>> + event_name = "unknown wait event"; >>>> + break; >>>> >>>> Seems this default case should be removed. Please see other >>>> pgstat_get_wait_xxx() function, so there is no such code. >>>> >>>>> I also removed the wait >>>>> event of conflict resolution of database since it's unlikely to become >>>>> a user-visible and a long sleep as we discussed before. >>>> >>>> Is it worth defining new wait event type RecoveryConflict only for >>>> those two events? ISTM that it's ok to use IPC event type here. >>>> >>> >>> I dropped a new wait even type and added them to IPC wait event type. >>> >>> I've attached the new version patch. >> >> Thanks for updating the patch! The patch looks good to me except >> the following mior things. >> >> + <row> >> + <entry><literal>RecoveryConflictSnapshot</literal></entry> >> + <entry>Waiting for recovery conflict resolution on a physical cleanup.</entry> >> + </row> >> + <row> >> + <entry><literal>RecoveryConflictTablespace</literal></entry> >> + <entry>Waiting for recovery conflict resolution on dropping tablespace.</entry> >> + </row> >> >> You need to increment the value of "morerows" in >> "<entry morerows="38"><literal>IPC</literal></entry>". >> >> The descriptions of those two events should be placed in alphabetical order >> for event name. That is, they should be placed above RecoveryPause. >> >> "vacuum cleanup" is better than "physical cleanup"? > > Agreed. > > I've attached the updated version patch. Thanks! Looks good to me. Barring any objection, I will commit this patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2020/04/02 16:12, Fujii Masao wrote: > > > On 2020/04/02 15:54, Masahiko Sawada wrote: >> On Thu, 2 Apr 2020 at 15:34, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>> >>> >>> >>> On 2020/04/02 14:25, Masahiko Sawada wrote: >>>> On Wed, 1 Apr 2020 at 22:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>>> >>>>> >>>>> >>>>> On 2020/03/30 20:10, Masahiko Sawada wrote: >>>>>> On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote: >>>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote: >>>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: >>>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict >>>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) >>>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait >>>>>>>>>>> events by adding the new type of wait event such as >>>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch >>>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for >>>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? >>>>>>>>> >>>>>>>>> Yes, it looks like a improvement rather than bug fix. >>>>>>>>> >>>>>>>> >>>>>>>> Okay, understand. >>>>>>>> >>>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion >>>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from >>>>>>>>>> back-backpatching. >>>>>>>>> >>>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly >>>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be >>>>>>>>> fixed even in the back branches. >>>>>>>> >>>>>>>> So we need only two patches: one fixes process title issue and another >>>>>>>> improve wait event. I've attached updated patches. >>>>>>> >>>>>>> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. >>>>>>> >>>>>>> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); >>>>>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); >>>>>>> >>>>>>> Currently the wait event indicating the wait for buffer pin has already >>>>>>> been reported. But the above change in the patch changes the name of >>>>>>> wait event for buffer pin only in the startup process. Is this really useful? >>>>>>> Isn't the existing wait event for buffer pin enough? >>>>>>> >>>>>>> - /* Wait to be signaled by the release of the Relation Lock */ >>>>>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>>>>> + /* Wait to be signaled by the release of the Relation Lock */ >>>>>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); >>>>>>> >>>>>>> Same as above. Isn't the existing wait event enough? >>>>>> >>>>>> Yeah, we can use the existing wait events for buffer pin and lock. >>>>>> >>>>>>> >>>>>>> - /* >>>>>>> - * Progressively increase the sleep times, but not to more than 1s, since >>>>>>> - * pg_usleep isn't interruptible on some platforms. >>>>>>> - */ >>>>>>> - standbyWait_us *= 2; >>>>>>> - if (standbyWait_us > 1000000) >>>>>>> - standbyWait_us = 1000000; >>>>>>> + WaitLatch(MyLatch, >>>>>>> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, >>>>>>> + STANDBY_WAIT_MS, >>>>>>> + wait_event_info); >>>>>>> + ResetLatch(MyLatch); >>>>>>> >>>>>>> ResetLatch() should be called before WaitLatch()? >>>>>> >>>>>> Fixed. >>>>>> >>>>>>> >>>>>>> Could you tell me why you dropped the "increase-sleep-times" logic? >>>>>> >>>>>> I thought we can remove it because WaitLatch is interruptible but my >>>>>> observation was not correct. The waiting startup process is not >>>>>> necessarily woken up by signal. I think it's still better to not wait >>>>>> more than 1 sec even if it's an interruptible wait. >>>>> >>>>> So we don't need to use WaitLatch() there, i.e., it's ok to keep using >>>>> pg_usleep()? >>>>> >>>>>> Attached patch fixes the above and introduces only two wait events of >>>>>> conflict resolution: snapshot and tablespace. >>>>> >>>>> Many thanks for updating the patch! >>>>> >>>>> - /* Wait to be signaled by the release of the Relation Lock */ >>>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>>> + /* Wait to be signaled by the release of the Relation Lock */ >>>>> + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); >>>>> + } >>>>> >>>>> Is this change really valid? What happens if the latch is set during >>>>> ResolveRecoveryConflictWithVirtualXIDs()? >>>>> ResolveRecoveryConflictWithVirtualXIDs() can return after the latch >>>>> is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. >>>> >>>> Thank you for reviewing the patch! >>>> >>>> You're right. It's better to keep using pg_usleep() and set the wait >>>> event by pgstat_report_wait_start(). >>>> >>>>> >>>>> + default: >>>>> + event_name = "unknown wait event"; >>>>> + break; >>>>> >>>>> Seems this default case should be removed. Please see other >>>>> pgstat_get_wait_xxx() function, so there is no such code. >>>>> >>>>>> I also removed the wait >>>>>> event of conflict resolution of database since it's unlikely to become >>>>>> a user-visible and a long sleep as we discussed before. >>>>> >>>>> Is it worth defining new wait event type RecoveryConflict only for >>>>> those two events? ISTM that it's ok to use IPC event type here. >>>>> >>>> >>>> I dropped a new wait even type and added them to IPC wait event type. >>>> >>>> I've attached the new version patch. >>> >>> Thanks for updating the patch! The patch looks good to me except >>> the following mior things. >>> >>> + <row> >>> + <entry><literal>RecoveryConflictSnapshot</literal></entry> >>> + <entry>Waiting for recovery conflict resolution on a physical cleanup.</entry> >>> + </row> >>> + <row> >>> + <entry><literal>RecoveryConflictTablespace</literal></entry> >>> + <entry>Waiting for recovery conflict resolution on dropping tablespace.</entry> >>> + </row> >>> >>> You need to increment the value of "morerows" in >>> "<entry morerows="38"><literal>IPC</literal></entry>". >>> >>> The descriptions of those two events should be placed in alphabetical order >>> for event name. That is, they should be placed above RecoveryPause. >>> >>> "vacuum cleanup" is better than "physical cleanup"? >> >> Agreed. >> >> I've attached the updated version patch. > > Thanks! Looks good to me. Barring any objection, I will commit this patch. Pushed! Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Fri, 3 Apr 2020 at 12:28, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/04/02 16:12, Fujii Masao wrote: > > > > > > On 2020/04/02 15:54, Masahiko Sawada wrote: > >> On Thu, 2 Apr 2020 at 15:34, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>> > >>> > >>> > >>> On 2020/04/02 14:25, Masahiko Sawada wrote: > >>>> On Wed, 1 Apr 2020 at 22:32, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>>> > >>>>> > >>>>> > >>>>> On 2020/03/30 20:10, Masahiko Sawada wrote: > >>>>>> On Fri, 27 Mar 2020 at 17:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote: > >>>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 2020/03/04 13:27, Michael Paquier wrote: > >>>>>>>>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote: > >>>>>>>>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict > >>>>>>>>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION) > >>>>>>>>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait > >>>>>>>>>>> events by adding the new type of wait event such as > >>>>>>>>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch > >>>>>>>>>>> is the fix for existing versions and 0003 patch is an improvement for > >>>>>>>>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching? > >>>>>>>>> > >>>>>>>>> Yes, it looks like a improvement rather than bug fix. > >>>>>>>>> > >>>>>>>> > >>>>>>>> Okay, understand. > >>>>>>>> > >>>>>>>>>> I got my eyes on this patch set. The full patch set is in my opinion > >>>>>>>>>> just a set of improvements, and not bug fixes, so I would refrain from > >>>>>>>>>> back-backpatching. > >>>>>>>>> > >>>>>>>>> I think that the issue (i.e., "waiting" is reported twice needlessly > >>>>>>>>> in PS display) that 0002 patch tries to fix is a bug. So it should be > >>>>>>>>> fixed even in the back branches. > >>>>>>>> > >>>>>>>> So we need only two patches: one fixes process title issue and another > >>>>>>>> improve wait event. I've attached updated patches. > >>>>>>> > >>>>>>> I started reading v2-0002-Improve-wait-events-for-recovery-conflict-resolut.patch. > >>>>>>> > >>>>>>> - ProcWaitForSignal(PG_WAIT_BUFFER_PIN); > >>>>>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_BUFFER_PIN); > >>>>>>> > >>>>>>> Currently the wait event indicating the wait for buffer pin has already > >>>>>>> been reported. But the above change in the patch changes the name of > >>>>>>> wait event for buffer pin only in the startup process. Is this really useful? > >>>>>>> Isn't the existing wait event for buffer pin enough? > >>>>>>> > >>>>>>> - /* Wait to be signaled by the release of the Relation Lock */ > >>>>>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >>>>>>> + /* Wait to be signaled by the release of the Relation Lock */ > >>>>>>> + ProcWaitForSignal(WAIT_EVENT_RECOVERY_CONFLICT_LOCK); > >>>>>>> > >>>>>>> Same as above. Isn't the existing wait event enough? > >>>>>> > >>>>>> Yeah, we can use the existing wait events for buffer pin and lock. > >>>>>> > >>>>>>> > >>>>>>> - /* > >>>>>>> - * Progressively increase the sleep times, but not to more than 1s, since > >>>>>>> - * pg_usleep isn't interruptible on some platforms. > >>>>>>> - */ > >>>>>>> - standbyWait_us *= 2; > >>>>>>> - if (standbyWait_us > 1000000) > >>>>>>> - standbyWait_us = 1000000; > >>>>>>> + WaitLatch(MyLatch, > >>>>>>> + WL_LATCH_SET | WL_POSTMASTER_DEATH | WL_TIMEOUT, > >>>>>>> + STANDBY_WAIT_MS, > >>>>>>> + wait_event_info); > >>>>>>> + ResetLatch(MyLatch); > >>>>>>> > >>>>>>> ResetLatch() should be called before WaitLatch()? > >>>>>> > >>>>>> Fixed. > >>>>>> > >>>>>>> > >>>>>>> Could you tell me why you dropped the "increase-sleep-times" logic? > >>>>>> > >>>>>> I thought we can remove it because WaitLatch is interruptible but my > >>>>>> observation was not correct. The waiting startup process is not > >>>>>> necessarily woken up by signal. I think it's still better to not wait > >>>>>> more than 1 sec even if it's an interruptible wait. > >>>>> > >>>>> So we don't need to use WaitLatch() there, i.e., it's ok to keep using > >>>>> pg_usleep()? > >>>>> > >>>>>> Attached patch fixes the above and introduces only two wait events of > >>>>>> conflict resolution: snapshot and tablespace. > >>>>> > >>>>> Many thanks for updating the patch! > >>>>> > >>>>> - /* Wait to be signaled by the release of the Relation Lock */ > >>>>> - ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >>>>> + /* Wait to be signaled by the release of the Relation Lock */ > >>>>> + ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type); > >>>>> + } > >>>>> > >>>>> Is this change really valid? What happens if the latch is set during > >>>>> ResolveRecoveryConflictWithVirtualXIDs()? > >>>>> ResolveRecoveryConflictWithVirtualXIDs() can return after the latch > >>>>> is set but before WaitLatch() in WaitExceedsMaxStandbyDelay() is reached. > >>>> > >>>> Thank you for reviewing the patch! > >>>> > >>>> You're right. It's better to keep using pg_usleep() and set the wait > >>>> event by pgstat_report_wait_start(). > >>>> > >>>>> > >>>>> + default: > >>>>> + event_name = "unknown wait event"; > >>>>> + break; > >>>>> > >>>>> Seems this default case should be removed. Please see other > >>>>> pgstat_get_wait_xxx() function, so there is no such code. > >>>>> > >>>>>> I also removed the wait > >>>>>> event of conflict resolution of database since it's unlikely to become > >>>>>> a user-visible and a long sleep as we discussed before. > >>>>> > >>>>> Is it worth defining new wait event type RecoveryConflict only for > >>>>> those two events? ISTM that it's ok to use IPC event type here. > >>>>> > >>>> > >>>> I dropped a new wait even type and added them to IPC wait event type. > >>>> > >>>> I've attached the new version patch. > >>> > >>> Thanks for updating the patch! The patch looks good to me except > >>> the following mior things. > >>> > >>> + <row> > >>> + <entry><literal>RecoveryConflictSnapshot</literal></entry> > >>> + <entry>Waiting for recovery conflict resolution on a physical cleanup.</entry> > >>> + </row> > >>> + <row> > >>> + <entry><literal>RecoveryConflictTablespace</literal></entry> > >>> + <entry>Waiting for recovery conflict resolution on dropping tablespace.</entry> > >>> + </row> > >>> > >>> You need to increment the value of "morerows" in > >>> "<entry morerows="38"><literal>IPC</literal></entry>". > >>> > >>> The descriptions of those two events should be placed in alphabetical order > >>> for event name. That is, they should be placed above RecoveryPause. > >>> > >>> "vacuum cleanup" is better than "physical cleanup"? > >> > >> Agreed. > >> > >> I've attached the updated version patch. > > > > Thanks! Looks good to me. Barring any objection, I will commit this patch. > > Pushed! Thanks! Thank you so much! Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services