Re: Replica Identity check of partition table on subscriber

On Fri, Jun 10, 2022 at 2:26 PM Amit Langote <amitlangote09(at)gmail(dot)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? If so, why do you think that is a
better place?

I think it is important to have this check to avoid giving error via
check_relation_updatable() when partitioned tables don't have RI but
not clear which is the right place. I think check_relation_updatable()
is better place than logicalrep_rel_mark_updatable() but may be there
is a reason why that is not a good idea.

--
With Regards,
Amit Kapila.