Repetitive code in RI triggers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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