On Tue, Nov 5, 2024 at 12:32 PM Peter Smith <[email protected]> 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.
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().
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 changed comments at a few other places in the patch. See attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/proto.c
b/src/backend/replication/logical/proto.c
index 2c8fbc593a..2c2085b2f9 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -1272,10 +1272,6 @@ logicalrep_should_publish_column(Form_pg_attribute att,
Bitmapset *columns,
if (columns)
return bms_is_member(att->attnum, columns);
- /*
- * 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.
- */
+ /* All non-generated columns are always published. */
return att->attgenerated ? include_gencols : true;
}
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index 386b087f79..014454d67b 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -168,9 +168,8 @@ typedef struct RelationSyncEntry
Bitmapset *columns;
/*
- * Include generated columns for publication is set to true if
- * 'publish_generated_columns' parameter is true, and the relation
- * contains generated columns.
+ * This is set if the 'publish_generated_columns' parameter is true, and
+ * the relation contains generated columns.
*/
bool include_gencols;
@@ -1098,7 +1097,6 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
* fetch_table_list. But one can later change the publication so we
still
* need to check all the given publication-table mappings and report an
* error if any publications have a different column list.
- *
*/
foreach(lc, publications)
{
@@ -1157,11 +1155,8 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
if (!cols)
{
/*
- * For the first publication with no specified column
list, we
- * retrieve and cache the table columns. This allows
comparison
- * with the column lists of other publications to
detect any
- * differences. Columns are retrieved only when there
is more than
- * one publication, as differences can only arise in
that case.
+ * Cache the table columns for the first publication
with no specified
+ * column list to detect publication with a different
column list.
*/
if (!relcols && (list_length(publications) > 1))
{
@@ -1187,8 +1182,8 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
} /* loop all
subscribed publications */
/*
- * If no column list publications exist, columns will be selected later
- * according to the 'publish_generated_columns' parameter.
+ * If no column list publications exist, columns to be published will be
+ * computed later according to the 'publish_generated_columns'
parameter.
*/
if (!found_pub_with_collist)
entry->columns = NULL;
