On Mon, Aug 22, 2016 at 6:46 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Mon, Aug 22, 2016 at 12:09 PM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>>
>> I took a look at your patch. Couple of notes from me.
>>
>>> const char *
>>> GetEventIdentifier(uint16 eventId)
>>> {
>>> const char *res;
>>> switch (eventId)
>>> {
>>> case EVENT_ARCHIVER_MAIN:
>>> res = "ArchiverMain";
>>> break;
>>> ... long long list of events ...
>>> case EVENT_WAL_SENDER_WRITE_DATA:
>>> res = "WalSenderWriteData";
>>> break;
>>> default:
>>> res = "???";
>>> }
>>> return res;
>>> }
>>
>>
>> Would it be better to use an array here?
Okay, I have switched to an array....
>>> typedef enum EventIdentifier
>>> {
>>
>>
>> EventIdentifier seems too general name for me, isn't it? Could we name it
>> WaitEventIdentifier? Or WaitEventId for shortcut?
... And WaitEventIdentifier.
> I'm also not sure about handling of secure_read() and secure_write()
> functions.
> In the current patch we're tracking latch event wait inside them. But we
> setup latch only for blocking sockets and can do it multiple times in loop.
> Would it be better to make separate wait events NETWORK_READ and
> NETWORK_WRITE and setup them for the whole time spent in secure_read() and
> secure_write()?
The whole point is to track a waiting event when we are sure that it
is going to wait, which is why the patch depends on WaitEventSetWait.
If we would set up those flags at the beginning and reset them of
secure_read and secure_write, we may actually track an event that is
not blocking.
--
Michael
