Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uiphg-00GNMi-PK for pgsql-hackers@arkaria.postgresql.org; Mon, 04 Aug 2025 07:33:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uiphf-00Gu2f-L6 for pgsql-hackers@arkaria.postgresql.org; Mon, 04 Aug 2025 07:33:51 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uiphf-00Gu2W-60 for pgsql-hackers@lists.postgresql.org; Mon, 04 Aug 2025 07:33:51 +0000 Received: from mail-qt1-x829.google.com ([2607:f8b0:4864:20::829]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1uiphc-000eDE-0S for pgsql-hackers@lists.postgresql.org; Mon, 04 Aug 2025 07:33:50 +0000 Received: by mail-qt1-x829.google.com with SMTP id d75a77b69052e-4aeea691687so34505451cf.1 for ; Mon, 04 Aug 2025 00:33:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1754292828; x=1754897628; darn=lists.postgresql.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=kKOxipTfw0HyVX8fFlnrtJf96XQnk+015F7fLOYF1Dw=; b=eS7kEUPAP/HMWYefDp1TJt41CihFGQ8KZArHINvtbHB63HtqR2Mn7Z0+INCJgJ/ud/ hRxDPiwwJn379mBOpgvTVjY9PUvDHAstcJyyzrUFcZn55N1QUewL46pFgcrj7phfJlKn 1lBBOMszA9evaoUo2XROfilmrCMbv3T0rERK6GwPGQ2dDss+AZLNgPk6pHZOKofopBg4 txnIbGWg8BsIepn4/eRGxgUQrxNrem2jXOZmRf6C/Ezr8YMge3znZBCnqJ7HVSEl6UNA Pqjw6wEFYhUDFhGwFdmEnMAn9v+t8H2/d5+puUAm4stTttzaX5zeFInX1eXpJeB1piUJ AF9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1754292828; x=1754897628; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=kKOxipTfw0HyVX8fFlnrtJf96XQnk+015F7fLOYF1Dw=; b=IWdpA21TvCekVh7h5RWth/5Vso0MekKJWhO3S0Aj+TKRRFcqMVjWnp7bUWidd6kgLC 4V7druoTY1hArsxMRjZPW5gNMXUxB+/8YAzjul+EMRu1cFB3kRaEAdDHlrYVPWkUtkZV D4jlTBlN2ZaBfYS2IbJZ3SY+uLMML0drx41OJlEnDgf8DuwswwZJi/l9Klmx8ZQF/o1T ynCY4ru4BU1nwHtlJWFmJqr2Ac8bnRYz1LL3llSAj90bt6cBmb73N6/k9LYUzsuIc8L3 U+dm6nJP2bMFd+J9xRyhAFATWPzZ/0JEI3nXxlDjjD6xW0RVNjtZDBhXgG0XZh3RRBRD MD8w== X-Forwarded-Encrypted: i=1; AJvYcCWdmU02zUKYIalokbCQGM3tLDec6DF1C2Zj8Vnj3onjGs0Y5ip6jnMVGvK6HkIbZ7JkDcePYdW0yrPJQkUt@lists.postgresql.org X-Gm-Message-State: AOJu0YyzGZzCDcQMQvbAmVakwidAPEDOdbhlygSP42FAFbpxMFzoENd3 li2b0zBLlefE/75awo//qsdbkiprWf0DaLbxNIx6MCkslX44Wo1KFMevjTMbGSqyVHxxLBCimlJ /tGwYmDFOG+KB+q75wEql5ZE6q0d4BBTFSRBQ X-Gm-Gg: ASbGncsp4WRAXHdcqO6mOM7CK9+12H8ttPoOIhJySkjy/6xQEJ8xMVzwtKZ1IlcYy3A svcmaJtCZBuGBZ7E+94tVaDlM9LD6lgKFOJHqsdrTw+QttaLlRkCZECzt/vvsbqyW5xPVk7yC3u wNRp428yCiEF3C30zSAb9w7Lcvw2eNE4HKmQ0SGXg+GluUjVRjVXwpJBNI+lI+YJjjIdENJc/Vp 9Du9MWKGI2I66nkQoAi9GmrYblg X-Google-Smtp-Source: AGHT+IGuv3djUw816KCBC1CrreIAeB4osZ9/rKGCL0EpQsQd01+IrnHYJ2lVQr3flakmiUacS15/2UTaWSGztCxueBI= X-Received: by 2002:a05:622a:1a21:b0:4ab:730d:c17e with SMTP id d75a77b69052e-4af10a4a4edmr120451391cf.39.1754292827903; Mon, 04 Aug 2025 00:33:47 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Peter Smith Date: Mon, 4 Aug 2025 17:33:21 +1000 X-Gm-Features: Ac12FXzCcTzwe7q3ZIgaks39_ctzy2egqxmxhAGdivDVNUiQ_7WFlDBnjQdXwxg Message-ID: Subject: Re: Skipping schema changes in publication To: Shlok Kyal Cc: Amit Kapila , "Zhijie Hou (Fujitsu)" , vignesh C , YeXiu <1518981153@qq.com>, Ian Lawrence Barwick , Bharath Rupireddy , PostgreSQL Hackers Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk On Mon, Aug 4, 2025 at 2:07=E2=80=AFAM Shlok Kyal 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 =3D store= d. > Eg: > postgres=3D# CREATE PUBLICATION pub1 FOR TABLE t1 with > (publish_generated_columns); > CREATE PUBLICATION > postgres=3D# 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 =3D=3D=3D=3D=3D=3D src/backend/catalog/pg_publication.c pg_get_publication_tables: 1. if (nattnums > 0) { values[2] =3D PointerGetDatum(buildint2vector(attnums, nattnums)); nulls[2] =3D false; } else nulls[2] =3D 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] =3D null in the else. =3D=3D=3D=3D=3D=3D 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 >=3D 190000) + *no_cols_published =3D 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 =3D=3D false); Assert(*no_cols_published =3D=3D false); =3D=3D=3D=3D=3D=3D 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 =3D NULL; + bool except_columns =3D false; + bool no_col_published =3D 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 =3D=3D=3D=3D=3D=3D 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") =3D=3D 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. =3D=3D=3D=3D=3D=3D 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"? =3D=3D=3D=3D=3D=3D .../t/037_rep_changes_except_collist.pl 9. +# Test initial sync +my $result =3D $node_subscriber->safe_psql('postgres', "SELECT * FROM tab1= "); +is($result, qq(|2|3), + 'check that initial sync for EXCEPT (column-list) publication'); +$result =3D $node_subscriber->safe_psql('postgres', "SELECT * FROM sch1.ta= b1"); +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 =3D 3, b =3D 4, c =3D 5 WHERE a =3D 1; )); $node_publisher->wait_for_catchup('tap_sub_col'); $result =3D $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 =3D> 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. =3D=3D=3D=3D=3D=3D Kind Regards, Peter Smith. Fujitsu Australia