On Wed, 15 Jan 2025 at 14:41, Shlok Kyal <shlok.kyal....@gmail.com> wrote: > > I have reviewed the patch and have following comments: > > In file: create_publication.sgml > > 1. > + <para> > + If set to <literal>stored</literal>, the generated columns present > in > + the tables associated with publication will be replicated. > </para> > > Instead of 'generated columns' we should use 'stored generated columns'
Modified > ====== > In file: pg_publication.c > > 2. I feel this condition can be more specific. Since a new macro will > be introduced for upcoming Virtual Generated columns. > > - if (att->attisdropped || (att->attgenerated && !include_gencols)) > + if (att->attisdropped || > + (att->attgenerated && (gencols_type == PUBLISH_GENCOL_NONE))) > continue; > > Something like: > if (att->attisdropped) > continue; > if (att->attgenerated == ATTRIBUTE_GENERATED_STORED && > gencols_type != PUBLISH_GENCOL_STORED) > continue; > > Thoughs? Modified > 3. Similarly this can be updated here as well: > > - if (att->attisdropped || (att->attgenerated && !pub->pubgencols)) > + if (att->attisdropped || > + (att->attgenerated && pub->pubgencols_type == PUBLISH_GENCOL_NONE)) > continue; Modified > ======= > In file proto.c > > 4. I feel this condition should also be more specific: > > /* All non-generated columns are always published. */ > - return att->attgenerated ? include_gencols : true; > + return att->attgenerated ? (gencols_type == PUBLISH_GENCOL_STORED) : true; > > We should return 'true' for 'gencols_type == PUBLISH_GENCOL_STORED' > only if 'att->attgenerated = ATTRIBUTE_GENERATED_STORED' Modified > ======= > In file publicationcmds.c > > 5. > /* > * As we don't allow a column list with REPLICA IDENTITY FULL, the > - * publish_generated_columns option must be set to true if the table > - * has any stored generated columns. > + * publish_generated_columns option must be set to 's'(stored) if the > + * table has any stored generated columns. > */ > - if (!pubgencols && > + if (gencols_type == PUBLISH_GENCOL_NONE && > relation->rd_att->constr && > > To be consistent with the comment, I think we should check if > 'gencols_type != PUBLISH_GENCOL_STORED' instead of 'gencols_type == > PUBLISH_GENCOL_NONE'. > Thoughts? Modified The v52 version patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm3OcXdY0EzDEKAfaK9gq2B67Mfsgxu93%2B_249ohyts%3D0g%40mail.gmail.com Regards, Vignesh
