Re: Pgoutput not capturing the generated columns

On Fri, Oct 18, 2024 at 5:42 PM Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
> > >
> > > I have fixed all the given comments. The attached patches contain the
> > > required changes.

Review comments:
===============
1.
>
B. when generated columns are not published

* Publisher not-generated column => subscriber not-generated column:
This is just normal logical replication (not changed by this patch).

* Publisher not-generated column => subscriber generated column:
This will give ERROR.
>

Is the second behavior introduced by the patch? If so, why?

2.
@@ -1213,7 +1207,10 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
{
...
- if (att->attisdropped || att->attgenerated)
+ if (att->attisdropped)
+ continue;
+
+ if (att->attgenerated && !pub->pubgencols)
continue;

It is better to combine the above conditions and write a comment on it.

3.
@@ -368,18 +379,50 @@ pub_collist_contains_invalid_column(Oid pubid,
Relation relation, List *ancestor
Anum_pg_publication_rel_prattrs,
&isnull);

- if (!isnull)
+ if (!isnull || !pubgencols)
{
int x;
Bitmapset *idattrs;
Bitmapset *columns = NULL;

- /* With REPLICA IDENTITY FULL, no column list is allowed. */
- if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
- result = true;
+ if (!isnull)
+ {
+ /* With REPLICA IDENTITY FULL, no column list is allowed. */
+ if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
+ result = true;
+
+ /* Transform the column list datum to a bitmapset. */
+ columns = pub_collist_to_bitmapset(NULL, datum, NULL);
+ }
+ else
+ {
+ TupleDesc desc = RelationGetDescr(relation);
+ int nliveatts = 0;
+
+ for (int i = 0; i < desc->natts; i++)
+ {
+ Form_pg_attribute att = TupleDescAttr(desc, i);
+
+ /* Skip if the attribute is dropped or generated */
+ if (att->attisdropped)
+ continue;
+
+ nliveatts++;
+
+ if (att->attgenerated)
+ continue;
+
+ columns = bms_add_member(columns, i + 1);
+ }

- /* Transform the column list datum to a bitmapset. */
- columns = pub_collist_to_bitmapset(NULL, datum, NULL);
+ /* Return if all columns of the table will be replicated */
+ if (bms_num_members(columns) == nliveatts)
+ {
+ bms_free(columns);
+ ReleaseSysCache(tuple);
+ return false;
+ }

Won't this lead to traversing the entire column list for default cases
where publish_generated_columns would be false which could hurt the
update/delete's performance? Irrespective of that, it is better to
write some comments to explain this logic.

4. Some minimum parts of 0002 like the changes in
/doc/src/sgml/ref/create_publication.sgml should be part of 0001
patch. We can always add examples or more details in the docs as a
later patch.

--
With Regards,
Amit Kapila.