Hi
po 19. 12. 2022 v 10:40 odesÃlatel Mikhail Gribkov <youzh...@gmail.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: youzh...@gmail.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 <yuzhih...@gmail.com> wrote: > >> >> >> On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov <youzh...@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 >> >
