Also i think this comment about time may be wrong: /* Unix time at which the query was executed. */

/* This structure defines an entry inside the slow log list */
typedef struct slowlogEntry {
    robj **argv;
    int argc;
    long long id;       /* Unique entry identifier. */
    long long duration; /* Time spent by the query, in microseconds. */
    time_t time;        /* Unix time at which the query was executed. */
    sds cname;          /* Client name. */
    sds peerid;         /* Client network address. */
} slowlogEntry;

In slowlogCreateEntry, look like the entry create time. Or the query logged time?

slowlogEntry *slowlogCreateEntry(client *c, robj **argv, int argc, long long duration) {
    ....
    se->time = time(NULL);  // here
    se->duration = duration;
    se->id = server.slowlog_entry_id++;
    se->peerid = sdsnew(getClientPeerId(c));
    se->cname = c->name ? sdsnew(c->name->ptr) : sdsempty();
    return se;
}

@enjoy-binbin The record time of slowlogCreateEntry is actually very close to the time when the command is run.
I think this comment is ok.

Yes i agree with that. Just for the record. Btw, if the command duration is long. Like is a really slow command.
The se->time = (command execute time + se->duration) ? Is that right? @sundb

Read doc: https://redis.io/commands/slowlog.
The comment is correct, the time calculation should be as you say, but I'm not sure if it needs to be changed.

it looks like a bug, but maybe it can be considered a feature.
i.e. a negative count means to fetch the entire slowlog (which is not that big, and usually capped to some 128 records).
so instead of someone passing a value like 1000000 to fetch the entire slowlog, he can just pass -1.
i don't think it really causes harm.
however looking at the code, it's certainly looks like a bug, so maybe we wanna code it differently and preserve the behavior?
@yossigo WDYT?

regarding the comment or doc, from the perspective of the execution time record, IMHO it doesn't really matter if it's the beginning of the command or it's end. the typical use case is to see that some command took 20ms, and that it happened around 8am, i.e. see if it happened today, or yesterday, or match it to some maintainable operation, or a configuration change etc.

Right. Sounds like a good idea. I like it. And for the comment or doc, i just thought about it when I saw it. Not need to fix it

I agree, it really looks like a bug :). I would vote to make -1 a special value that means fetch the entire slowlog, and then make that clearer in the code.

I agree with @madolson.

@oranagra No problem. Doc pr: redis/redis-doc#1585 I alerady made but seems to forget metion in here

@redis/core-team, we already have a majority, and this is in fact not a behavior change, but please respond if you feel documenting this behavior is wrong.

@oranagra @madolson This idea came about suddenly. What do you think we add a lazyfree about the slowlog reset. Maybe like #8258

/* Remove all the entries from the current slow log. */
void slowlogReset(void) {
    while (listLength(server.slowlog) > 0)
        listDelNode(server.slowlog,listLast(server.slowlog));
}

Sound silly, forget about that... No one would do it like me

127.0.0.1:6379> slowlog len
(integer) 2966648
127.0.0.1:6379> slowlog reset
OK
(0.78s)
127.0.0.1:6379>

@enjoy-binbin Lol, lazyfree all of the things. Given that the default is 128, it's an anti-pattern to let it grown, I'm going to agree and say let's not make it lazyfree.