Re: Skipping schema changes in publication

On Mon, Aug 4, 2025 at 2:07 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
...
> > 10b.
> > How does "ALTER PUBLICATION tap_pub_col SET
> > (publish_generated_columns)" even work? I thought the
> > "pubish_generated_columns" is an enum but you did not specify any enum
> > value here (???)
> >
> > ~~~
> Yes, it works. It works equivalent to publish_generated_columns = stored.
> Eg:
> postgres=# CREATE PUBLICATION pub1 FOR TABLE t1 with
> (publish_generated_columns);
> CREATE PUBLICATION
> postgres=# select * from pg_publication;
> oid | pubname | pubowner | puballtables | pubinsert | pubupdate |
> pubdelete | pubtruncate | pubviaroot | pubgencols
> -------+---------+----------+--------------+-----------+-----------+-----------+-------------+------------+------------
> 16395 | pub1 | 10 | f | t | t | t
> | t | f | s
> (1 row)
>

Hmm -- it's not documented to behave like that, so I've created
another thread for getting to the bottom of this topic.

~~~

Meanwhile, here are my review comments for patch v18-0003

======
src/backend/catalog/pg_publication.c

pg_get_publication_tables:

1.
if (nattnums > 0)
{
values[2] = PointerGetDatum(buildint2vector(attnums, nattnums));
nulls[2] = false;
}
else
nulls[2] = true;

Is there any possibility that values[2] might not be null, but then
nattrnums skips some cols so remains 0? Then the final values[2] would
conflict with nulls[2], which seems strange. Maybe it is safer to also
assign values[2] = null in the else.

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

fetch_remote_table_info:

2.
static void
fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation *lrel,
- List **qual, bool *gencol_published)
+ List **qual, bool *gencol_published,
+ bool *no_cols_published)

This new parameter should be documented in the function comment.

~~~

3.
+ if (server_version >= 190000)
+ *no_cols_published = DatumGetBool(slot_getattr(tslot, 2, &isnull));
+

It seems that *no_cols_published (and *gencol_published) are assigned
false by the caller. I had to go looking for that, so IMO it would be
better to put Assert at the top of here so it is self-documenting

Assert(*gencol_published == false);
Assert(*no_cols_published == false);

======
src/backend/replication/pgoutput/pgoutput.c

4.
+ /*
+ * Indicates whether no columns are published for a given relation. With
+ * the introduction of the EXCEPT clause in column lists, it is now
+ * possible to define a publication that excludes all columns of a table.
+ * However, the 'columns' attribute cannot represent this case, since a
+ * NULL value implies that all columns are published. To distinguish this
+ * scenario, the 'no_cols_published' flag is introduced.
+ */
+ bool no_cols_published;

The wording of the comment seems a bit strange -- EXCEPT is not a clause.

BEFORE:
the introduction of the EXCEPT clause in column lists, ...

SUGGESTION
the introduction of the EXCEPT qualifier for column lists, ....

~~~

5.
Bitmapset *cols = NULL;
+ bool except_columns = false;
+ bool no_col_published = false;

There are multiple places in this patch that say:

'no_col_published'
or 'no_cols_published'

I felt this var name can be misunderstood because it is easy to read
"no" as meaning "no." (aka number), and then misinterpret as
"number_of_cols_published".

Maybe an unambiguous name can be found, like
- 'zero_cols_published' or
- 'nothing_published' or
- really make it 'num_cols_published' and check for 0.

(so this comment applies to multiple places in the patch)

~~

6.
* of the table (including generated columns when
* 'publish_generated_columns' parameter is true).
*/
- if (!cols)
+ if (!no_col_published && !cols)
{

The existing comment above this code fragment also needs to mention
"EXCEPT (column-list)" where all the columns are excluded

======
src/bin/psql/describe.c

describeOneTableDetails:

7.
/* column list (if any) */
if (!PQgetisnull(result, i, 2))
- appendPQExpBuffer(&buf, " (%s)",
- PQgetvalue(result, i, 2));
+ {
+ if (strcmp(PQgetvalue(result, i, 3), "t") == 0)
+ appendPQExpBuffer(&buf, " EXCEPT (%s)",
+ PQgetvalue(result, i, 2));
+ else
+ appendPQExpBuffer(&buf, " (%s)",
+ PQgetvalue(result, i, 2));
+ }

Isn't this code fragment (and also surrounding code) using the same
logic as what is already encapsulated in the function
addFooterToPublicationDesc()?
Superficially, it seems like a large chunk can all be replaced with a
single call to the existing function.

======
src/test/regress/expected/publication.out

8.
+-- Syntax error EXCEPT without a col-list
+CREATE PUBLICATION testpub_except2 FOR TABLE pub_test_except1 EXCEPT;
+ERROR: EXCEPT clause not allowed for table without column list
+LINE 1: CREATE PUBLICATION testpub_except2 FOR TABLE pub_test_except...
+ ^

Is that a bad syntax position marker (^)? e.g. Why is it pointed at
the word "TABLE" instead of "EXCEPT"?

======
.../t/037_rep_changes_except_collist.pl

9.
+# Test initial sync
+my $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab1");
+is($result, qq(|2|3),
+ 'check that initial sync for EXCEPT (column-list) publication');
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM sch1.tab1");
+is($result, qq(1||),
+ 'check that initial sync for EXCEPT (column-list) publication');

These messages still seem to have missing or extra words: "check that
initial sync" (??). Maybe just remove the word 'that'?

~~~

10.
# Test for update
$node_subscriber->safe_psql(
'postgres', qq(
CREATE UNIQUE INDEX b_idx ON tab1 (b);
ALTER TABLE tab1 REPLICA IDENTITY USING INDEX b_idx;
));
$node_publisher->safe_psql(
'postgres', qq(
CREATE UNIQUE INDEX b_idx ON tab1 (b);
ALTER TABLE tab1 REPLICA IDENTITY USING INDEX b_idx;
UPDATE tab1 SET a = 3, b = 4, c = 5 WHERE a = 1;
));
$node_publisher->wait_for_catchup('tap_sub_col');
$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab1");
is( $result, qq(|5|6
|4|5),
'check update for EXCEPT (column-list) publication');

~

10a.
I think the test is OK, but your chosen numbers like 1,2,3, then 4,5,6
and then updating to 1,2,3 to 3,4,5 make it quite hard to review.
Maybe use easier numbers that are more identifiable, e.g. update 1,2,3
=> 991,992,993 or something like that.

~

10b.
You may need to put some ORDER BY in all these queries just to make
sure they are always reproducible, giving rows in the expected order.

======
Kind Regards,
Peter Smith.
Fujitsu Australia