Re: On login trigger: take three - Mailing list pgsql-hackers

From Mikhail Gribkov
Subject Re: On login trigger: take three
Date
Msg-id CAMEv5_ujyNwCHU_5fHPxxRAqJ7HOPxOwYGMCD-GyB4zi7y8ijQ@mail.gmail.com
Whole thread Raw
In response to Re: On login trigger: take three  (Ted Yu <yuzhihong@gmail.com>)
Responses Re: On login trigger: take three
Re: On login trigger: take three
List 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);
> 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.

--
 best regards,
    Mikhail A. Gribkov

e-mail: youzhick@gmail.com
http://www.strava.com/athletes/5085772
phone: +7(916)604-71-12
Telegram: @youzhick



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. Gribkov
Hi,

bq. in to the system

'in to' -> into

bq. Any bugs in a trigger procedure

Any 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
Attachment

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: appendBinaryStringInfo stuff
Next
From: Alvaro Herrera
Date:
Subject: Re: Common function for percent placeholder replacement