On Thu, Jun 16, 2022 at 5:24 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Thu, Jun 16, 2022 at 6:42 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangot...@gmail.com> > > wrote: > > > @@ -1735,6 +1735,13 @@ apply_handle_insert_internal(ApplyExecutionData > > > *edata, > > > static void > > > check_relation_updatable(LogicalRepRelMapEntry *rel) > > > { > > > + /* > > > + * If it is a partitioned table, we don't check it, we will check its > > > + * partition later. > > > + */ > > > + if (rel->localrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) > > > + return; > > > > > > Why do this? I mean why if logicalrep_check_updatable() doesn't care > > > if the relation is partitioned or not -- it does all the work > > > regardless. > > > > > > I suggest we don't add this check in check_relation_updatable(). > > > > I think based on this suggestion patch has moved this check to > > logicalrep_rel_mark_updatable(). For a partitioned table, it won't > > even validate whether it can mark updatable as false which seems odd > > to me even though there might not be any bug due to that. Was your > > suggestion actually intended to move it to > > logicalrep_rel_mark_updatable? > > No, I didn't intend to suggest that we move this check to > logicalrep_rel_mark_updatable(); didn't notice that that's what the > latest patch did. > > What I said is that we shouldn't ignore the updatable flag for a > partitioned table in check_relation_updatable(), because > logicalrep_rel_mark_updatable() would have set the updatable flag > correctly even for partitioned tables. IOW, we should not > special-case partitioned tables anywhere. > > I guess the point of adding the check is to allow the case where a > leaf partition's replica identity can be used to apply an update > originally targeting its ancestor that doesn't itself have one. > > I wonder if it wouldn't be better to move the > check_relation_updatable() call to > apply_handle_{update|delete}_internal()? We know for sure that we > only ever get there for leaf tables. If we do that, we won't need the > relkind check. >
I think this won't work for updates via apply_handle_tuple_routing() unless we call it from some other place(s) as well. It will do FindReplTupleInLocalRel() before doing update/delete for CMD_UPDATE case and will lead to assertion failure. -- With Regards, Amit Kapila.