Re: Pgoutput not capturing the generated columns

On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 24, 2024 at 8:50 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > The v42 version patch attached at [1] has the changes for the same.
> >
>
> Some more comments:
> 1.
> @@ -1017,7 +1089,31 @@ pgoutput_column_list_init(PGOutputData *data,
> List *publications,
>  {
>   ListCell   *lc;
>   bool first = true;
> + Bitmapset  *relcols = NULL;
>   Relation relation = RelationIdGetRelation(entry->publish_as_relid);
> + TupleDesc desc = RelationGetDescr(relation);
> + MemoryContext oldcxt = NULL;
> + bool collistpubexist = false;
> +
> + pgoutput_ensure_entry_cxt(data, entry);
> +
> + oldcxt = MemoryContextSwitchTo(entry->entry_cxt);
> +
> + /*
> + * Prepare the columns that will be published for FOR ALL TABLES and
> + * FOR TABLES IN SCHEMA publication.
> + */
> + for (int i = 0; i < desc->natts; i++)
> + {
> + Form_pg_attribute att = TupleDescAttr(desc, i);
> +
> + if (att->attisdropped || (att->attgenerated && !entry->pubgencols))
> + continue;
> +
> + relcols = bms_add_member(relcols, att->attnum);
> + }
> +
> + MemoryContextSwitchTo(oldcxt);
>
> This code is unnecessary for cases when the table's publication has a
> column list. So, I suggest to form this list only when required. Also,
> have an assertion that pubgencols value for entry and publication
> matches.
>

Modified and also included Assertion.

> 2.
> @@ -1115,10 +1186,17 @@ pgoutput_column_list_init(PGOutputData *data,
> List *publications,
>   ereport(ERROR,
>   errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>   errmsg("cannot use different column lists for table \"%s.%s\" in
> different publications",
> -    get_namespace_name(RelationGetNamespace(relation)),
> -    RelationGetRelationName(relation)));
> + get_namespace_name(RelationGetNamespace(relation)),
> + RelationGetRelationName(relation)));
>
> Is there a reason to make the above change? It appears to be a spurious change.
>

Change is not required, removed now.

> 3.
> + /* Check if there is any generated column present */
> + for (int i = 0; i < desc->natts; i++)
> + {
> + Form_pg_attribute att = TupleDescAttr(desc, i);
> + if (att->attgenerated)
>
> Add one empty line between the above two lines.
>

Added.

> 4.
> + else if (entry->pubgencols != pub->pubgencols)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot use different values of publish_generated_columns for
> table \"%s.%s\" in different publications",
> + get_namespace_name(RelationGetNamespace(relation)),
> + RelationGetRelationName(relation)));
>
> The last two lines are not aligned.
>

Modified.

The updated v43 version of patches contain the changes for the same.

Thanks and Regards,
Shubham Khanna.