On Tue, Sep 10, 2024 at 2:51 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Mon, Sep 9, 2024 at 2:38 AM Shubham Khanna > <khannashubham1...@gmail.com> wrote: > > > > Thank you for updating the patches. I have some comments: > > Do we really need to add this option to test_decoding? >
I don't see any reason to have such an option in test_decoding, otherwise, we need a separate option for each publication option. I guess this is leftover of the previous subscriber-side approach. > I think it > would be good if this improves the test coverage. Otherwise, I'm not > sure we need this part. If we want to add it, I think it would be > better to have it in a separate patch. > Right. > --- > + <para> > + If the publisher-side column is also a generated column > then this option > + has no effect; the publisher column will be filled as normal with > the > + publisher-side computed or default data. > + </para> > > I don't understand this description. Why does this option have no > effect if the publisher-side column is a generated column? > Shouldn't it be subscriber-side? I have one additional comment: /* - * If the publication is FOR ALL TABLES then it is treated the same as - * if there are no column lists (even if other publications have a - * list). + * If the publication is FOR ALL TABLES and include generated columns + * then it is treated the same as if there are no column lists (even + * if other publications have a list). */ - if (!pub->alltables) + if (!pub->alltables || !pub->pubgencolumns) Why do we treat pubgencolumns at the same level as the FOR ALL TABLES case? I thought that if the user has provided a column list, we only need to publish the specified columns even when the publish_generated_columns option is set. -- With Regards, Amit Kapila.
