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
