Re: Pgoutput not capturing the generated columns

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: Shubham Khanna <khannashubham1197(at)gmail(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(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-09-24 01:37:49
Message-ID: CAHut+Pu7EcK_JTgWS7GzeStHk6Asb1dmEzCJU2TJf+W1Zy30LQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi. Here are my v32-0002 review comments:

======
src/backend/replication/logical/tablesync.c

1. fetch_remote_table_info

/*
- * Get column lists for each relation.
+ * Get column lists for each relation, and check if any of the
+ * publications have the 'publish_generated_columns' parameter enabled.

I am not 100% sure about this logic anymore. Maybe it is OK, but it
requires careful testing because with Amit's "column lists take
precedence" it is now possible for the publication to say
'publish_generated_columns=false', but the publication can still
publish gencols *anyway* if they were specified in a column list.

~~~

2.
/*
* Fetch info about column lists for the relation (from all the
* publications).
*/
+ StringInfo pub_names = makeStringInfo();
+
+ get_publications_str(MySubscription->publications, pub_names, true);
resetStringInfo(&cmd);
appendStringInfo(&cmd,
~

nit - The comment here seems misplaced.

~~~

3.
+ if (server_version >= 120000)
+ {
+ has_pub_with_pubgencols = server_version >= 180000 && has_pub_with_pubgencols;
+
+ if (!has_pub_with_pubgencols)
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
+ }

My previous review comment about this [1 #10] was:
Can the 'gencols_allowed' var be removed, and the condition just be
replaced with if (!has_pub_with_pubgencols)? It seems equivalent
unless I am mistaken.

nit - So the current v32 code is not what I was expecting. What I
meant was 'has_pub_with_pubgencols' can only be true if server_version
>= 180000, so I thought there was no reason to check it again. For
reference, I've changed it to like I meant in the nitpicks attachment.
Please see if that works the same.

======
[1] my review of v31-0002.
https://www.postgresql.org/message-id/CAHut%2BPusbhvPrL1uN1TKY%3DFd4zu3h63eDebZvsF%3Duy%2BLBKTwgA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment Content-Type Size
PS_NITPICKS_v320002.txt text/plain 1.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Shayon Mukherjee 2024-09-24 01:44:17 Re: Proposal to Enable/Disable Index using ALTER INDEX
Previous Message jian he 2024-09-24 01:00:15 Re: ANALYZE ONLY