On Thu, Aug 9, 2018 at 11:28 PM, Peter Geoghegan <pg@bowt.ie> wrote:
> On Thu, Aug 9, 2018 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I took a quick look at these. The v11 patch seems OK as far as it goes,
>> but I wonder if you shouldn't also include the RelationMapUpdateMap
>> hunk from the master patch, ie adding rejection of map changes in parallel
>> mode.
>
> Good idea. I'll do that.
>
>> I don't have any objection to the master patch, but it'd be good to get
>> a +1 from someone who's spent more time with the parallelism
>> infrastructure than I have.
>
> That would be my preference too, but commit 29d58fd3 is very analogous
> to the proposed master branch fix, so I feel that it's reasonable to
> proceed without final approval from someone like Robert or Amit.
>
I haven't studied the complete problem, but the way you are
propagating the information to parallel workers looks correct to me.
Few minor comments:
1.
+void
+RestoreRelationMap(char *startAddress)
+{
+ SerializedActiveRelMaps *relmaps;
+
+ if (active_shared_updates.num_mappings != 0 ||
+ active_local_updates.num_mappings != 0 ||
+ pending_shared_updates.num_mappings != 0 ||
+ pending_local_updates.num_mappings != 0)
+ elog(ERROR, "parallel worker has existing mappings");
..
Shouldn't above be Assert?
2.
+void
+SerializeRelationMap(Size maxSize, char *startAddress)
+{
+ SerializedActiveRelMaps *relmaps;
+
+ relmaps = (SerializedActiveRelMaps *) startAddress;
+ relmaps->active_shared_updates = active_shared_updates;
+ relmaps->active_local_updates = active_local_updates;
..
}
Some of the other serialize functions use maxSize for Asserts. See
SerializeComboCIDState. I think we can do without that as well, but
it makes code consistent.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com