From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Shubham Khanna <khannashubham1197(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Rajendra Kumar Dangwal <dangwalrajendra888(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, euler(at)eulerto(dot)com |
Subject: | Re: Pgoutput not capturing the generated columns |
Date: | 2024-10-21 11:19:39 |
Message-ID: | CAA4eK1Jjrf=R+CBAB-ZD3KffYs8J-2eTeCf521ra=3RDjt_kBg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2024-10-21 11:30:41 | Re: type cache cleanup improvements |
Previous Message | Dagfinn Ilmari Mannsåker | 2024-10-21 10:16:25 | Re: type cache cleanup improvements |