On 18 August 2017 at 10:55, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > On 2017/08/18 4:54, Robert Haas wrote: >> On Thu, Aug 17, 2017 at 8:39 AM, Ashutosh Bapat >> <ashutosh.ba...@enterprisedb.com> wrote: >>> [2] had a patch with some changes to the original patch you posted. I >>> didn't describe those changes in my mail, since they rearranged the >>> comments. Those changes are not part of this patch and you haven't >>> comments about those changes as well. If you have intentionally >>> excluded those changes, it's fine. In case, you haven't reviewed them, >>> please see if they are good to be incorporated. >> >> I took a quick look at your version but I think I like Amit's fine the >> way it is, so committed that and back-patched it to v10. > > Thanks for committing. > >> I find 0002 pretty ugly as things stand. We get a bunch of tuple maps >> that we don't really need, only to turn around and free them. We get >> a bunch of tuple slots that we don't need, only to turn around and >> drop them. We don't really need the PartitionDispatch objects either, >> except for the OIDs they contain. There's a lot of extra stuff being >> computed here that is really irrelevant for this purpose. I think we >> should try to clean that up somehow. > > One way to do that might be to reverse the order of the remaining patches > and put the patch to refactor RelationGetPartitionDispatchInfo() first. > With that refactoring, PartitionDispatch itself has become much simpler in > that it does not contain a relcache reference to be closed eventually by > the caller, the tuple map, and the tuple table slot. Since those things > are required for tuple-routing, the refactoring makes > ExecSetupPartitionTupleRouting itself create them from the (minimal) > information returned by RelationGetPartitionDispatchInfo and ultimately > destroy when done using it. I kept the indexes field in > PartitionDispatchData though, because it's essentially free to create > while we are walking the partition tree in > RelationGetPartitionDispatchInfo() and it seems undesirable to make the > caller compute that information (indexes) by traversing the partition tree > all over again, if it doesn't otherwise have to. I am still considering > some counter-arguments raised by Amit Khandekar about this last assertion. > > Thoughts?
One another approach (that I have used in update-partition-key patch) is to *not* generate the oids beforehand, and instead, call a partition_walker_next() function to traverse through the tree. Each next() function would return a ChildInfo that includes child oid, parent oid, etc. All users of this would guarantee a fixed order of oids. In the update-partition-key patch, I am opening and closing each of the children, which of course, we need to avoid. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
