Re: [HACKERS] make async slave to wait for lsn to be replayed

On 2020-04-07 12:58, Amit Kapila wrote:
> 
> Review comments:
> 1.
> +static void
> +DeleteEvent(void)
> I don't see how this is implemented or called to handle any errors.
> 
> 2.
> + if (InterruptPending)
> + {
> + DeleteEvent();
> + ProcessInterrupts();
> + }
> We generally do this type of handling via CHECK_FOR_INTERRUPTS.
> 
> 3.
> +int
> +WaitUtility(XLogRecPtr target_lsn, const float8 secs)
> Isn't it better to have a return value as bool?
> 
> 4.
> +#define GetNowFloat() ((float8) GetCurrentTimestamp() / 1000000.0)
> This same define is used elsewhere in the code as well, may be we can
> define it in some central place and use it.

Thank you for your review!
Ivan and I have worked on the patch and tried to address your comments:

0. Now we wake up at least every 100ms to check for interrupts.
1. Now we call DeleteWaitedLSN() from 
ProcessInterrupts()=>LockErrorCleanup(). It seems that we can only exit 
the WAIT cycle improperly due to interrupts, so this should be enough 
(?)
2. Now we use CHECK_FOR_INTERRUPTS() instead of ProcessInterrupts()
3. Now WaitUtility() returns bool rather than int
4. Now GetNowFloat() is only defined at one place in the code

What we changed additionally:
- Prohibited using WAIT FOR LSN on master
- Added more tests
- Checked the code with pgindent and adjusted pgindent/typedefs.list
- Changed min_lsn's type to pg_atomic_uint64 + fixed how we work with 
mutex
- Code cleanup in wait.[c|h]: cleaned up #include-s, gave better names 
to functions, changed elog() to ereport()

-- 
Anna Akenteva
Postgres Professional:
The Russian Postgres Company
http://www.postgrespro.com