Re: On login trigger: take three
| От | Mikhail Gribkov |
|---|---|
| Тема | Re: On login trigger: take three |
| Дата | |
| Msg-id | CAMEv5_ujyNwCHU_5fHPxxRAqJ7HOPxOwYGMCD-GyB4zi7y8ijQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: On login trigger: take three (Ted Yu <yuzhihong@gmail.com>) |
| Ответы |
Re: On login trigger: take three
Re: On login trigger: take three |
| Список | pgsql-hackers |
Hi Ted,
> bq. in to the system
> 'in to' -> into
> bq. Any bugs in a trigger procedure
> Any bugs -> Any bug
Thanks! Fixed typos in the attached v35.
> + if (event == EVT_Login)
> + dbgtag = CMDTAG_LOGIN;
> + else
> + dbgtag = CreateCommandTag(parsetree);
> + 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.
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.
--
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 по дате отправления: