On 21 June 2017 at 20:14, Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Jun 21, 2017 at 5:28 AM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote:>> The comment "UPDATE/DELETE > cases are handled above" is referring to >>> the code that initializes the WCOs generated by the planner. You've >>> modified the comment in your patch, but the associated code: your >>> updated comment says that only "DELETEs and local UPDATES are handled >>> above", but in reality, *all* updates are still handled above. And >>> then they are handled again here. Similarly for returning lists. >>> It's certainly not OK for the comment to be inaccurate, but I think >>> it's also bad to redo the work which the planner has already done, >>> even if it makes the patch smaller. >> >> I guess this has to do with the UPDATE turning into DELETE+INSERT. So, it >> seems like WCOs are being initialized for the leaf partitions >> (ResultRelInfos in the mt_partitions array) that are in turn are >> initialized for the aforementioned INSERT. That's why the term "...local >> UPDATEs" in the new comment text. >> >> If that's true, I wonder if it makes sense to apply what would be >> WCO_RLS_UPDATE_CHECK to a leaf partition that the tuple will be moved into >> by calling ExecInsert()? > > I think we probably should apply the insert policy, just as we're > executing the insert trigger.
Yes, the RLS quals should execute during tuple routing according to whether it is a update or whether it has been converted to insert. I think the tests don't quite test the insert part. Will check. > >>> Also, I feel like it's probably not correct to use the first result >>> relation as the nominal relation for building WCOs and returning lists >>> anyway. I mean, if the first result relation has a different column >>> order than the parent relation, isn't this just broken? If it works >>> for some reason, the comments don't explain what that reason is. >> >> Yep, it's more appropriate to use >> ModifyTableState->rootResultRelationInfo->ri_RelationDesc somehow. That >> is, if answer to the question I raised above is positive. >From what I had checked earlier when coding that part, rootResultRelInfo is NULL in case of inserts, unless something has changed in later commits. That's the reason I decided to use the first resultRelInfo. Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers