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(dot)musin(at)postgrespro(dot)ru

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(dot)eisentraut(at)2ndquadrant(dot)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

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

> On 26 Sep 2017, at 10:51, Maksim Milyutin <milyutinma(at)gmail(dot)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

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(dot)musin(at)postgrespro(dot)ru

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(dot)kurbangaliev(at)gmail(dot)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(dot)musin(at)postgrespro(dot)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(dot)musin(at)postgrespro(dot)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(dot)musin(at)postgrespro(dot)ru