On Tue, Oct 29, 2024 at 7:44 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> ======
> src/backend/replication/logical/proto.c
>
> 2.
> +bool
> +logicalrep_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 (!columns && att->attgenerated)
> + return false;
> +
> + /*
> + * Check if a column is covered by a column list.
> + */
> + if (columns && !bms_is_member(att->attnum, columns))
> + return false;
> +
> + return true;
> +}
>
> I thought this could be more simply written as:
>
> {
> if (att->attisdropped)
> return false;
>
> /* If a column list was specified only publish the specified columns. */
> if (columns)
> return bms_is_member(att->attnum, columns);
>
> /* If a column list was not specified publish everything except
> generated columns. */
> return !att->attgenerated;
> }
>
Your version is difficult to follow compared to what is proposed in
the current patch. It is a matter of personal choice, so I leave it to
the author (or others) which one they prefer. However, I suggest that
we add extra comments in the current patch where we return true at the
end of the function and also at the top of the function.
>
> ======
> src/test/regress/sql/publication.sql
>
> 4.
> ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
>
> +-- ok: generated column "d" can be in the list too
> +ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (d);
> +ALTER PUBLICATION testpub_fortable DROP TABLE testpub_tbl5;
>
> Maybe you can change this test to do "SET TABLE testpub_tbl5 (a,d);"
> instead of ADD TABLE, so then you can remove the earlier DROP and DROP
> the table only once.
>
Yeah, we can do that if we want, but let's not add the dependency of
the previous test. Separate tests make it easier to extend the tests
in the future. Now, if it would have saved a noticeable amount of
time, then we could have considered it. Having said that, we can keep
both columns a and d in the column list.
> ======
> src/test/subscription/t/031_column_list.pl
>
> 5.
> +# TEST: Dropped columns are not considered for the column list, and generated
> +# columns are not replicated if they are not explicitly included in the
> column
> +# list. So, the publication having a column list except for those columns
> and a
> +# publication without any column (aka all columns as part of the columns
> list)
> +# are considered to have the same column list.
>
> Hmm. I don't think this wording is quite right "without any column".
> AFAIK the original intent of this test was to prove only that
> dropped/generated columns were ignored for the NULL column list logic.
>
> That last sentence maybe should say more like:
>
> So a publication with a column list specifying all table columns
> (excluding only dropped and generated columns) is considered to be the
> same as a publication that has no column list at all for that table.
>
I think you are saying the same thing in slightly different words.
Both of those sound correct to me. So not sure if we get any advantage
by changing it.
--
With Regards,
Amit Kapila.
