> On Thu, Jan 14, 2021 at 5:36 PM Li Japin <japinli@hotmail.com> wrote
> > Do we really need to access PUBLICATIONRELMAP in this patch? What if
> > we just set it to false in the else condition of (if (publish &&
> > (relkind != RELKIND_PARTITIONED_TABLE || pub->pubviaroot)))
> >
> > Thank for you review. I agree with you, it doesn’t need to access
> > PUBLICATIONRELMAP, since We already get the publication oid in
> > GetRelationPublications(relid) [1], which also access
> > PUBLICATIONRELMAP. If the current pub->oid does not in pubids, the
> publish is false, so we do not need publish the table.
>
> +1. This is enough.
>
> > I have another question, the data->publications is a list, when did it
> has more then one items?
>
> IIUC, when the single table is associated with multiple publications, then
> data->publications will have multiple entries. Though I have not tried,
> we can try having two or three publications for the same table and verify
> that.
>
> > 0001 - change as you suggest.
> > 0002 - does not change.
Hi
In 0001 patch
The comments says " Relation is not associated with the publication anymore "
That comment was accurate when check is_relation_part_of_publication() in the last patch.
But when put the code in the else branch, not only the case in the comments(Relation is dropped),
Some other case such as "partitioned tables" will hit else branch too.
Do you think we need fix the comment a little to make it accurate?
And it seems we can add a "continue;" in else branch.
Of course this it not necessary.
Best regards,
houzj
