Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
Date: 2017-11-26 22:56:32
Message-ID: 14028.1511736992@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> [ fix-rewrite-tlist-v4.patch ]

I started reviewing this patch. I did not much like the fact that it
effectively moved rewriteTargetListUD to a different file and renamed it.
That seems like unnecessary code churn, plus it breaks the analogy with
rewriteTargetListIU, plus it will make back-patching harder (since that
code isn't exactly the same in back branches). I see little reason why
we can't leave it where it is and just make it non-static. It's not like
there's no other parts of the rewriter that the planner calls.

I revised the patch along that line, and while at it, refactored
preptlist.c a bit to eliminate repeated heap_opens of the target
relation. I've not really reviewed any other aspects of the patch
yet, but in the meantime, does anyone object to proceeding this way?

regards, tom lane

Attachment Content-Type Size
fix-rewrite-tlist-v5.patch text/x-diff 25.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2017-11-27 01:02:22 Re: [HACKERS] More stats about skipped vacuums
Previous Message Mark Dilger 2017-11-26 22:31:08 Re: Memory error in src/backend/replication/logical/origin.c