On Mon, Oct 28, 2024 at 7:43 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Hi, here are my review comments for patch v43-0001.
>
> ======
> src/backend/replication/logical/proto.c
>
> 2.
> +static bool
> +should_publish_column(Form_pg_attribute att, Bitmapset *columns)
> +{
> + if (att->attisdropped)
> + return false;
> +
> + /*
> + * Skip publishing generated columns if they are not included in the
> + * column list.
> + */
> + if (att->attgenerated && !columns)
> + return false;
> +
> + if (!column_in_column_list(att->attnum, columns))
> + return false;
> +
> + return true;
> +}
>
> Here, I wanted to suggest that the whole "Skip publishing generated
> columns" if-part is unnecessary because the next check
> (!column_in_column_list) is going to return false for the same
> scenario anyhow.
>
> But, unfortunately, the "column_in_column_list" function has some
> special NULL handling logic in it; this means none of this code is
> quite what it seems to be (e.g. the function name
> column_in_column_list is somewhat misleading)
>
> IMO it would be better to change the column_in_column_list signature
> -- add another boolean param to say if a NULL column list is allowed
> or not. That will remove any subtle behaviour and then you can remove
> the "if (att->attgenerated && !columns)" part.
>
The NULL column list still means all columns, so changing the behavior
as you are proposing doesn't make sense and would make the code
difficult to understand.
>
> 4. pgoutput_column_list_init
>
> - if (att->attisdropped || att->attgenerated)
> + if (att->attisdropped)
> continue;
>
> + if (att->attgenerated)
> + {
> + if (bms_is_member(att->attnum, cols))
> + gencolpresent = true;
> +
> + continue;
> + }
> +
> nliveatts++;
> }
>
> /*
> - * If column list includes all the columns of the table,
> - * set it to NULL.
> + * If column list includes all the columns of the table
> + * and there are no generated columns, set it to NULL.
> */
> - if (bms_num_members(cols) == nliveatts)
> + if (bms_num_members(cols) == nliveatts && !gencolpresent)
> {
>
> Something seems not quite right (or maybe redundant) with this logic.
> For example, because you unconditionally 'continue' for generated
> columns, then AFAICT it is just not possible for bms_num_members(cols)
> == nliveatts and at the same time 'gencolpresent' to be true. So you
> could've just Asserted (!gencolpresent) instead of checking it in the
> condition and mentioning it in the comment.
>
It seems part of the logic is redundant. I propose to change something
along the lines of the attached. I haven't tested the attached change
as it shows how we can improve this part of code.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/pgoutput/pgoutput.c
b/src/backend/replication/pgoutput/pgoutput.c
index d59a8f5032..17aeb80637 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1081,7 +1081,7 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
int i;
int nliveatts = 0;
TupleDesc desc =
RelationGetDescr(relation);
- bool gencolpresent = false;
+ bool att_gen_present = false;
pgoutput_ensure_entry_cxt(data, entry);
@@ -1098,20 +1098,19 @@ pgoutput_column_list_init(PGOutputData *data, List
*publications,
if (att->attgenerated)
{
- if
(bms_is_member(att->attnum, cols))
- gencolpresent =
true;
-
- continue;
+ att_gen_present = true;
+ break;
}
nliveatts++;
}
/*
- * If column list includes all the
columns of the table
- * and there are no generated columns,
set it to NULL.
+ * Generated attributes are published
only when they are
+ * present in the column list.
Otherwise, a NULL column
+ * list means publish all columns.
*/
- if (bms_num_members(cols) == nliveatts
&& !gencolpresent)
+ if (!att_gen_present &&
bms_num_members(cols) == nliveatts)
{
bms_free(cols);
cols = NULL;
