On Wed, Oct 5, 2022 at 23:05 PM Osumi, Takamichi/大墨 昂道 <osumi.takamichi@fujitsu.com> wrote:
> Hi, thank you for the updated patches!
>
>
> Here are my minor review comments for HEAD v12.
Thanks for your comments.
> (1) typo & suggestion to reword one comment
>
>
> + * Publications support partitioned tables. If
> + * publish_via_partition_root is false, all changes are replicated
> + * using leaf partition identity and schema, so we only need
> + * those. Otherwise, If publish_via_partition_root is true, get
> + * the partitioned table itself.
>
>
> The last sentence has "If" in the middle of the sentence.
> We can use the lower letter for it. Or, I think "Otherwise" by itself means
> "If publish_via_partition_root is true". So, I'll suggest a below change.
>
>
> FROM:
> Otherwise, If publish_via_partition_root is true, get the partitioned table itself.
> TO:
> Otherwise, get the partitioned table itself.
Improved.
> (2) Do we need to get "attnames" column from the publisher in the
> fetch_table_list() ?
>
> When I was looking at v16 path, I didn't see any codes that utilize
> the "attnames" column information returned from the publisher.
> If we don't need it, could we remove it ?
> I can miss something greatly, but this might be affected by HEAD codes ?
Yes, it is affected by HEAD. I think we need this column to check whether the
same table has multiple column lists. (see commit fd0b9dc)
The new patch set were attached in [1].
[1] -
https://www.postgresql.org/message-id/OS3PR01MB6275843B2BBE92870F7881C19E299%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Regards,
Wang wei
