On Wed, Nov 4, 2026 at 9:06 PM Ajin Cherian <[email protected]> wrote:
...
> To make the tests pass, I've included the v1 patch from [1], unchanged, as
> 0001.
>
> 0002 adds tests to 011_generated.pl for a table whose only column is
> VIRTUAL, and one whose only column is STORED with
> publish_generated_columns = none, in both text and binary mode.
>
> [1]
> https://www.postgresql.org/message-id/CAL5_ZS1bnYkK_GrA4N4SVKYgYsMMJxL9416m%3DvECAjrcwmQkkg%40mail.gmail.com
>
Hi Ajin.
Thanks for the 0002 patch!
Basically, it LGTM, but here are a couple of very minor review comments.
======
src/backend/replication/logical/tablesync.c
1.
"SELECT DISTINCT"
" (CASE WHEN (array_length(gpt.attrs, 1) = c.relnatts)"
- " THEN NULL ELSE gpt.attrs END)"
+ " THEN NULL ELSE gpt.attrs END),"
+ " gpt.attrs IS NULL"
" FROM pg_get_publication_tables(ARRAY[%s], %u) gpt,"
" pg_class c"
" WHERE c.oid = gpt.relid",
I wondered if it might be helpful for the new result column to have an
alias like:
gpt.attrs IS NULL AS no_cols_published
It's not used anywhere, so it doesn't actually do anything, but OTOH I
think it makes the SQL a bit more self-documenting. What do you think?
======
src/test/subscription/t/011_generated.pl
2.
pub:
+ CREATE TABLE tab_virt_only_text (a int GENERATED ALWAYS AS (99) VIRTUAL);
+ CREATE TABLE tab_virt_only_bin (a int GENERATED ALWAYS AS (99) VIRTUAL);
+ CREATE TABLE tab_stored_only_text (b int GENERATED ALWAYS AS (77) STORED);
+ CREATE TABLE tab_stored_only_bin (b int GENERATED ALWAYS AS (77) STORED);
~
sub:
+ CREATE TABLE tab_virt_only_text (c text DEFAULT 'sub-default');
+ CREATE TABLE tab_virt_only_bin (c text DEFAULT 'sub-default');
+ CREATE TABLE tab_stored_only_text (b int, c text DEFAULT 'sub-default');
+ CREATE TABLE tab_stored_only_bin (b int, c text DEFAULT 'sub-default');
These are all different tables, so it's not necessary to have 3
different column names for them.
Wouldn't it be more intuitive to just call them "a" and "b" (or "a"
and "local_col")?
e.g.
pub:
CREATE TABLE tab_virt_only_text (a int GENERATED ALWAYS AS (99) VIRTUAL);
CREATE TABLE tab_virt_only_bin (a int GENERATED ALWAYS AS (99) VIRTUAL);
CREATE TABLE tab_stored_only_text (a int GENERATED ALWAYS AS (77) STORED);
CREATE TABLE tab_stored_only_bin (a int GENERATED ALWAYS AS (77) STORED);
~
sub:
CREATE TABLE tab_virt_only_text (local_col text DEFAULT 'sub-default');
CREATE TABLE tab_virt_only_bin (local_col text DEFAULT 'sub-default');
CREATE TABLE tab_stored_only_text (a int, local_col text DEFAULT 'sub-default');
CREATE TABLE tab_stored_only_bin (a int, local_col text DEFAULT 'sub-default');
Of course, this comment also needs to change accordingly:
+# The subscriber tables for tab_virt_only_* have no column named "a", and the
+# ones for tab_stored_only_* have a regular column "b".
~~~
3.
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_sub_gen_only_text CONNECTION
'$publisher_connstr' PUBLICATION regress_pub_gen_only_text WITH
(binary = false)"
+);
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_sub_gen_only_bin CONNECTION
'$publisher_connstr' PUBLICATION regress_pub_gen_only_bin WITH (binary
= true)"
+);
Those can be combined into a single safe_sql, right?
~~~
4.
+$node_subscriber->safe_psql('postgres',
+ "DROP SUBSCRIPTION regress_sub_gen_only_text");
+$node_subscriber->safe_psql('postgres',
+ "DROP SUBSCRIPTION regress_sub_gen_only_bin");
Those could be combined into a single safe_sql, right?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
