Re: Speedup generation of command completion tags
От | David Rowley |
---|---|
Тема | Re: Speedup generation of command completion tags |
Дата | |
Msg-id | CAApHDvp5b8X1jJQvmHCGD010BOHORgpzJOLa7kGp-TcLa+HUTQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Speedup generation of command completion tags (Andres Freund <andres@anarazel.de>) |
Ответы |
Re: Speedup generation of command completion tags
(David Rowley <dgrowleyml@gmail.com>)
|
Список | pgsql-hackers |
Thanks for having a look at this. On Sun, 11 Dec 2022 at 11:05, Andres Freund <andres@anarazel.de> wrote: > > On 2022-12-10 20:32:06 +1300, David Rowley wrote: > > @@ -20,13 +20,14 @@ > > typedef struct CommandTagBehavior > > { > > const char *name; > > + const uint8 namelen; > > Perhaps worth adding a comment noting that namelen is the length without the > null byte? I've added that now plus a few other fields that could be easily documented. I left out commenting all of the remaining fields as documenting table_rewrite_ok seemed slightly more than this patch should be doing. There's commands that rewrite tables, e.g. VACUUM FULL that have that as false. Looks like this is only used for commands that *might* rewrite the table. I didn't want to tackle that in this patch. > What about moving make_completion_tag() to cmdtag.c? Then we could just get > the entire CommandTagBehaviour struct at once. It's not super pretty to pass > QueryCompletion to a routine in cmdtag.c, but it's not awful. And if we deem > it problematic, we could just pass qc->commandTag, qc->nprocessed as a > separate arguments. That seems like a good idea. I've renamed and moved the function in the attached. I also adjusted how the trailing NUL char is appended to avoid having to calculate len + 1 and append the NUL char twice for the most commonly taken path. > I wonder if any of the other GetCommandTagName() would benefit noticably from > not having to compute the length. I guess the calls > set_ps_display(GetCommandTagName()) calls in exec_simple_query() and > exec_execute_message() might, although set_ps_display() isn't exactly zero > overhead. But I do see it show up as a few percent in profiles, with the > biggest contributor being the call to strlen. I think that could be improved for sure. It does seem like we'd need to add set_ps_display_with_len() to make what you said work. There's probably lower hanging fruit in that function that could be fixed without having to do that, however. For example: strlcpy(ps_buffer + ps_buffer_fixed_size, activity, ps_buffer_size - ps_buffer_fixed_size); ps_buffer_cur_len = strlen(ps_buffer); could be written as: strlcpy(ps_buffer + ps_buffer_fixed_size, activity, ps_buffer_size - ps_buffer_fixed_size); ps_buffer_cur_len = ps_buffer_fixed_size + Min(strlen(activity), ps_buffer_size - ps_buffer_fixed_size - 1); That's pretty horrible to read though. This sort of thing also makes me think that our investment in having more usages of strlcpy() and fewer usages of strncpy was partially a mistake. There are exactly 2 usages of the return value of strlcpy in our entire source tree. That's about 1% of all calls. Likely what would be better is a function that returns the number of bytes *actually* copied instead of one that returns the number of bytes that it would have copied if it hadn't run out of space. Such a function could be defined as: size_t strdcpy(char * const dst, const char *src, ptrdiff_t len) { char *dstp = dst; while (len-- > 0) { if ((*dstp = *src++) == '\0') { *dstp = '\0'; break; } dstp++; } return (dstp - dst); } Then we could append to strings like: char buffer[STRING_SIZE]; char *bufp = buffer; bufp += strdcpy(bufp, "01", STRING_SIZE - (bufp - buffer)); bufp += strdcpy(bufp, "23", STRING_SIZE - (bufp - buffer)); bufp += strdcpy(bufp, "45", STRING_SIZE - (bufp - buffer)); which allows transformation of the set_ps_display() code to: pg_buffer_cur_len = ps_buffer_fixed_size; pg_buffer_cur_len += strdcpy(ps_buffer + ps_buffer_fixed_size, activity, ps_buffer_size - ps_buffer_fixed_size); (Assume the name strdcpy as a placeholder name for an actual name that does not conflict with something that it's not.) I'd rather not go into too much detail about that here though. I don't see any places that can make use of the known tag length without going to the trouble of inventing new functions or changing the signature of existing ones. David
Вложения
В списке pgsql-hackers по дате отправления: