Thread: some ri_triggers.c cleanup
ri_triggers.c is endlessly long and repetitive. I want to clean it up a bit (more). I looked into all these switch cases for the unimplemented MATCH PARTIAL option. I toyed around with how a MATCH PARTIAL implementation would actually look like, and it likely wouldn't use the existing code structure anyway, so let's just simplify this for now. First, have ri_FetchConstraintInfo() check that riinfo->confmatchtype is valid. Then we don't have to repeat that everywhere. In the various referential action functions, we don't need to pay attention to the match type at all right now, so remove all that code. A future MATCH PARTIAL implementation would probably have some conditions added to the present code, but it won't need an entirely separate switch branch in each case. In RI_FKey_fk_upd_check_required(), reorganize the code to make it much simpler. Separately, the comment style is also very generous and wasteful with vertical space. That can be shrunk a bit. Attached are some patches. Final score: branch wc -l REL9_6_STABLE 3671 REL_10_STABLE 3668 REL_11_STABLE 3179 master 3034 patch 2695 -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Fri, Feb 22, 2019 at 11:05 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
ri_triggers.c is endlessly long and repetitive. I want to clean it up a
bit (more).
Having just been down this road, I agree that a lot of cleanup is needed and possible.
I looked into all these switch cases for the unimplemented MATCH PARTIAL
option. I toyed around with how a MATCH PARTIAL implementation would
actually look like, and it likely wouldn't use the existing code
structure anyway, so let's just simplify this for now.
+1
Attached are some patches.
I intend to look this over in much greater detail, but I did skim the code and it seems like you left the SET DEFAULT and SET NULL paths separate. In my attempt at statement level triggers I realized that they only differed by the one literal value, and parameterized the function.
On Fri, Feb 22, 2019 at 1:12 PM Corey Huinker <corey.huinker@gmail.com> wrote:
On Fri, Feb 22, 2019 at 11:05 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:ri_triggers.c is endlessly long and repetitive. I want to clean it up a
bit (more).Having just been down this road, I agree that a lot of cleanup is needed and possible.I looked into all these switch cases for the unimplemented MATCH PARTIAL
option. I toyed around with how a MATCH PARTIAL implementation would
actually look like, and it likely wouldn't use the existing code
structure anyway, so let's just simplify this for now.+1
Attached are some patches.I intend to look this over in much greater detail, but I did skim the code and it seems like you left the SET DEFAULT and SET NULL paths separate. In my attempt at statement level triggers I realized that they only differed by the one literal value, and parameterized the function.
I've looked it over more closely now and I think that it's a nice improvement.
As I suspected, the code for SET NULL and SET DEFAULT are highly similar (see .diff), the major difference being two constants, the order of some variable declarations, and the recheck in the set-default case.
The changes were so simple that I felt remiss not adding the patch for you (see .patch).
The changes were so simple that I felt remiss not adding the patch for you (see .patch).
Passes make check.
Attachment
On 2019-02-24 00:34, Corey Huinker wrote: > As I suspected, the code for SET NULL and SET DEFAULT are highly similar > (see .diff), the major difference being two constants, the order of some > variable declarations, and the recheck in the set-default case. > > The changes were so simple that I felt remiss not adding the patch for > you (see .patch). Right, this makes a lot of sense, similar to how ri_restrict() combines RESTRICT and NO ACTION. I'll take a closer look at your patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Right, this makes a lot of sense, similar to how ri_restrict() combines
RESTRICT and NO ACTION.
I'm pretty sure that's where I got the idea, yes.
On 2019-02-25 17:17, Corey Huinker wrote: > Right, this makes a lot of sense, similar to how ri_restrict() combines > RESTRICT and NO ACTION. > > > I'm pretty sure that's where I got the idea, yes. Committed, including your patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services