Re: On login trigger: take three
От | Pavel Stehule |
---|---|
Тема | Re: On login trigger: take three |
Дата | |
Msg-id | CAFj8pRDHh4HV-v1ergUVncckh0Y4ocsMyWrr=p0B_TQX=xvA6g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: On login trigger: take three (Mikhail Gribkov <youzhick@gmail.com>) |
Ответы |
Re: On login trigger: take three
|
Список | pgsql-hackers |
Hi
I think raising an exception inside should be better tested - not it is only in 001_stream_rep.pl - generally more tests are welcome - there are no tested handling exceptions.
Regards
po 19. 12. 2022 v 10:40 odesílatel Mikhail Gribkov <youzhick@gmail.com> napsal:
Hi Ted,> bq. in to the system> 'in to' -> into> bq. Any bugs in a trigger procedure> Any bugs -> Any bugThanks! Fixed typos in the attached v35.> + if (event == EVT_Login)
> + dbgtag = CMDTAG_LOGIN;
> + else
> + dbgtag = CreateCommandTag(parsetree);> The same snippet appears more than once. It seems CMDTAG_LOGIN handling can be moved into `CreateCommandTag`.It appears twice in fact, both cases are nearby: in the main code and under the assert-checking ifdef.
Moving CMDTAG_LOGIN to CreateCommandTag would change the CreateCommandTag function signature and ideology. CreateCommandTag finds a tag based on the command parse tree, while login event is a specific case when we do not have any command and the parse tree is NULL. Changing CreateCommandTag signature for these two calls looks a little bit overkill as it would lead to changing lots of other places the function is called from.
We could move this snippet to a separate function, but here are the similar concerns I think: it would make sense for a more common or a more complex snippet, but not for two cases of if-else one-liners.
I checked this patch and it looks well. All tests passed. Together with https://commitfest.postgresql.org/41/4013/ it can be a good feature.
I re-tested impact on performance and for the worst case looks like less than 1% (0.8%). I think it is acceptable. Tested pgbench scenario "SELECT 1"
pgbench -f ~/test.sql -C -c 3 -j 5 -T 100 -P10 postgres
733 tps (master), 727 tps (patched).
Pavel
--On Sat, Dec 17, 2022 at 3:29 PM Ted Yu <yuzhihong@gmail.com> wrote:On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov <youzhick@gmail.com> wrote:Hi Nikita,> Mikhail, I've checked the patch and previous discussion,> the condition mentioned earlier is still actual:Thanks for pointing this out! My bad, I forgot to fix the documentation example.Attached v34 has this issue fixed, as well as a couple other problems with the example code.--best regards,
Mikhail A. GribkovHi,bq. in to the system'in to' -> intobq. Any bugs in a trigger procedureAny bugs -> Any bug+ if (event == EVT_Login)
+ dbgtag = CMDTAG_LOGIN;
+ else
+ dbgtag = CreateCommandTag(parsetree);The same snippet appears more than once. It seems CMDTAG_LOGIN handling can be moved into `CreateCommandTag`.Cheers
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Zhang MingliДата:
Сообщение: Re: Fix condition in shm_toc and remove unused function shm_toc_freespace.