Re: Pgoutput not capturing the generated columns

On Tue, 5 Nov 2024 at 16:25, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 5, 2024 at 12:32 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > has_column_list_defined:
> >
> > 1.
> > + if (HeapTupleIsValid(cftuple))
> > + {
> > + bool isnull = true;
> > +
> > + /* Lookup the column list attribute. */
> > + (void) SysCacheGetAttr(PUBLICATIONRELMAP, cftuple,
> > + Anum_pg_publication_rel_prattrs,
> > + &isnull);
> >
> > AFAIK it is not necessary to assign a default value to 'isnull' here.
> > e.g. most of the other 100s of calls to SysCacheGetAttr elsewhere in
> > PostgreSQL source don't bother to do this.
> >
>
> Can we try to reuse this new function has_column_list_defined() in
> pgoutput_column_list_init() where we are trying to check and form a
> column list? For that, we need to change this function such that it
> also returns a column list when requested.

Modified

> Some more comments:
> 1.
> extern bool is_schema_publication(Oid pubid);
> +extern bool has_column_list_defined(Publication *pub, Oid relid);
>
> The order of declaration doesn't match with its definition in .c file.
> It would be better to define this function after
> is_schema_publication().

Modified

> 2.
> + * 'include_gencols' flag indicates whether generated columns should be
> + * published when there is no column list. Typically, this will have the same
> + * value as the 'publish_generated_columns' publication parameter.
> + *
> + * Note that generated columns can be published only when present in a
> + * publication column list, or when include_gencols is true.
> */
> bool
> -logicalrep_should_publish_column(Form_pg_attribute att, Bitmapset *columns)
> +logicalrep_should_publish_column(Form_pg_attribute att, Bitmapset *columns,
> + bool include_gencols)
> {
> if (att->attisdropped)
> return false;
>
> - /*
> - * Skip publishing generated columns if they are not included in the
> - * column list.
> - */
> - if (!columns && att->attgenerated)
> - return false;
> + /* If a column list is provided, publish only the cols in that list. */
> + if (columns)
> + return bms_is_member(att->attnum, columns);
>
> /*
> - * Check if a column is covered by a column list.
> + * If no column list is provided, generated columns will be published only
> + * if include_gencols is true, while all non-generated columns will always
> + * be published.
>
> The similar information is repeated multiple times in different words.
> I have adjusted the comments in this part of the code to avoid
> repetition.

I have taken the changes

> I have changed comments at a few other places in the patch. See attached.

I have taken the changes

The attached v49 patch has the changes for the same.

Regards,
Vignesh