Re: On login trigger: take three

Hi

po 19. 12. 2022 v 10:40 odesílatel Mikhail Gribkov <youzhick(at)gmail(dot)com>
napsal:

> 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.
>

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).

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

Pavel

> --
> best regards,
> Mikhail A. Gribkov
>
> e-mail: youzhick(at)gmail(dot)com
> *http://www.flickr.com/photos/youzhick/albums
> <http://www.flickr.com/photos/youzhick/albums>*
> 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(at)gmail(dot)com> wrote:
>
>>
>>
>> On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov <youzhick(at)gmail(dot)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
>>
>