Re: Replica Identity check of partition table on subscriber

On Thu, Jun 16, 2022 at 9:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jun 16, 2022 at 5:24 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Jun 16, 2022 at 6:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangote09@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.

You're right.  I guess it's fine then to check the relkind in
check_relation_updatable() the way the original patch did, even though
it would've been nice if it didn't need to.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com