Thread: [HACKERS] Repetitive code in RI triggers
Hi all, I was looking through the RI triggers code recently and noticed a few almost identical functions, e.g. ri_restrict_upd() and ri_restrict_del(). The following patch is an attempt to reduce some of repetitive code. Yet there is still room for improvement. Thanks, -- Ildar Musin i.musin@postgrespro.ru -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 4/10/17 11:55, Ildar Musin wrote: > I was looking through the RI triggers code recently and noticed a few > almost identical functions, e.g. ri_restrict_upd() and > ri_restrict_del(). The following patch is an attempt to reduce some of > repetitive code. That looks like something worth pursuing. Please add it to the next commit fest. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 11 Apr 2017, at 03:41, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 4/10/17 11:55, Ildar Musin wrote: >> I was looking through the RI triggers code recently and noticed a few >> almost identical functions, e.g. ri_restrict_upd() and >> ri_restrict_del(). The following patch is an attempt to reduce some of >> repetitive code. > > That looks like something worth pursuing. Please add it to the next > commit fest. Removing reviewer Maksim Milyutin from patch entry due to inactivity and community account email bouncing. Maksim: if you are indeed reviewing this patch, then please update the community account and re-add to the patch entry. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 19.09.2017 11:09, Daniel Gustafsson wrote: > Removing reviewer Maksim Milyutin from patch entry due to inactivity and > community account email bouncing. Maksim: if you are indeed reviewing this > patch, then please update the community account and re-add to the patch entry. > > cheers ./daniel Daniel, thanks for noticing. I have updated my account and re-added to the patch entry. Ildar, your patch is conflicting with the current HEAD of master branch, please update it. -- Regards, Maksim Milyutin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
> On 26 Sep 2017, at 10:51, Maksim Milyutin <milyutinma@gmail.com> wrote: > > On 19.09.2017 11:09, Daniel Gustafsson wrote: > >> Removing reviewer Maksim Milyutin from patch entry due to inactivity and >> community account email bouncing. Maksim: if you are indeed reviewing this >> patch, then please update the community account and re-add to the patch entry. >> >> cheers ./daniel > > Daniel, thanks for noticing. I have updated my account and re-added to the patch entry. Great, thanks! > Ildar, your patch is conflicting with the current HEAD of master branch, please update it. I’ve changed status to Waiting on Author based on this. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Hi Maksim, On 26.09.2017 11:51, Maksim Milyutin wrote: > On 19.09.2017 11:09, Daniel Gustafsson wrote: > >> Removing reviewer Maksim Milyutin from patch entry due to inactivity and >> community account email bouncing. Maksim: if you are indeed reviewing >> this >> patch, then please update the community account and re-add to the >> patch entry. >> >> cheers ./daniel > > Daniel, thanks for noticing. I have updated my account and re-added to > the patch entry. > > Ildar, your patch is conflicting with the current HEAD of master branch, > please update it. > Thank you for checking the patch out. Yes, it seems that original code was reformatted and this led to merging conflicts. I've fixed that and also introduced some minor improvements. The new version is in attachment. Thanks! -- Ildar Musin i.musin@postgrespro.ru -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: tested, failed Documentation: tested, failed The patch looks good. It just removes repetitive code and I think it's ready to commit. The new status of this patch is: Ready for Committer
On Fri, 17 Nov 2017 15:05:31 +0000 Ildus Kurbangaliev <i.kurbangaliev@gmail.com> wrote: > The following review has been posted through the commitfest > application: make installcheck-world: tested, failed > Implements feature: tested, failed > Spec compliant: tested, failed > Documentation: tested, failed > > The patch looks good. It just removes repetitive code and I think > it's ready to commit. > > The new status of this patch is: Ready for Committer "tested, failed" should be read as "tested, passed". Forgot to check them. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Ildar Musin <i.musin@postgrespro.ru> writes:
> [ ri_triggers_v2.patch ]
Pushed with two minor improvements. I noticed that ri_setdefault could
just go directly to ri_restrict rather than call the two separate triggers
that would end up there anyway; that lets its argument be "TriggerData
*trigdata" for more consistency with the other cases. Also, this patch
made it very obvious that we were caching identical queries under hash
keys RI_PLAN_RESTRICT_DEL_CHECKREF and RI_PLAN_RESTRICT_UPD_CHECKREF,
so we might as well just use one hash entry for both cases, saving a few
lines of code as well as a lot of cycles. Likewise in the other two
functions.
regards, tom lane
On 19.11.2017 00:31, Tom Lane wrote: > Ildar Musin <i.musin@postgrespro.ru> writes: >> [ ri_triggers_v2.patch ] > > Pushed with two minor improvements. I noticed that ri_setdefault could > just go directly to ri_restrict rather than call the two separate triggers > that would end up there anyway; that lets its argument be "TriggerData > *trigdata" for more consistency with the other cases. Also, this patch > made it very obvious that we were caching identical queries under hash > keys RI_PLAN_RESTRICT_DEL_CHECKREF and RI_PLAN_RESTRICT_UPD_CHECKREF, > so we might as well just use one hash entry for both cases, saving a few > lines of code as well as a lot of cycles. Likewise in the other two > functions. > > regards, tom lane > Great, thanks -- Ildar Musin i.musin@postgrespro.ru